[llvm-dev] [RFC] NewGVN

[Davide Italiano via llvm-dev]

Hi,
we would like to propose a new Global Value Numbering pass in LLVM.
The ideas/code are from Daniel Berlin (with a minor overhaul/splitting
into submittable patches from me). The code has been around for a
while (2012 or before), and we think it's getting ready to be
committed upstream.

### Motivation

To put things into context: my personal motivation for having a new
GVN/PRE algorithm is LTO.
It's not a secret that LLVM is getting slower and slower release after
release, as Rafael discovered/pointed out in March [1] (and probably
many others found out). I personally took a shot at profiling LTO on
many internal/opensource applications (including clang itself) and
noticed that GVN always show in the top-3 passes (and it's generally
the pass where we spend most of the time in the middle-end). There are
cases (extreme) where 90% of the compile time goes in GVN.

Example:
===-------------------------------------------------------------------------===
... Pass execution timing report ...
===-------------------------------------------------------------------------===
Total Execution Time: 684.7622 seconds (683.7141 wall clock)

---User Time--- --System Time-- --User+System-- ---Wall
Time--- --- Name ---
130.2558 ( 20.0%) 6.3128 ( 18.7%) 136.5686 ( 19.9%) 137.6145 (
20.1%) X86 DAG->DAG Instruction Selection
55.4695 ( 8.5%) 0.5501 ( 1.6%) 56.0196 ( 8.2%) 56.1049 (
8.2%) Function Integration/Inlining
42.3492 ( 6.5%) 0.0364 ( 0.1%) 42.3856 ( 6.2%) 42.8676 (
6.3%) Global Value Numbering

### Problems in the current GVN

There are some issues in the current GVN infrastructure. A
non-exhaustive list (I'm pretty sure people have their list of
complains, these are the ones I care about the most).
* GVN is very slow for large test cases
* GVN doesn't do a real analysis, instead it eliminates as it goes, so
it's hard to reason about it.
* GVN doesn't perform any phi predication, i.e. doesn't know about phi
nodes, so later passes have to do some extra work to clean up
* There are bugs, e.g. [2] which would require a rewrite of PRE to be fixed.

### NewGVN

The new algorithm implements the ideas described in [3] with some
engineering optimizations by Dan (for example the set of touched
instructions is represented using a Bitvector instead of a set because
it's not uncommon for large functions where a predicate change that
thousands of instructions need to be changed, and we both measured 30%
of the whole pass time spent just modifying the set).
The code pretty much maps what the paper describe so I won't try to
repeat it here =)

Some advantages of NewGVN:
* GVN performs a real analysis (which is now part of the pass itself
but potentially could be split and used as an utility by other
passes). For example, Quentin/Dan pointed out that outlining at the IR
level is hard without a proper value numbering analysis.
* On large testcases, It's faster than current GVN, even though didn't
go through a lot of profiling/optimization lately (I found it up to
50% faster in compile time on some internal benchmarks). There are
some places were we can improve. For example, we spend a lot of time
inside Simplify* functions. Another considerable chunk of the time is
spent inside MemorySSA, but this is going to change once we preserve
MemSSA more aggressively.
* The code is easier to understand (at least to me)
[...]

Some current limitations of NewGVN:
* NewGVN doesn't do everything that the current GVN does. There are
plans to implement missing features and more.
* On small testcases NewGVN may be slower as it does some work upfront
that the current GVN doesn't do. NewGVN is probably less lazy than it
should be, but it didn't matter for us so we didn't consider it a
priority. If somebody cares and finds a case where NewGVN
substantially regresses, speak up.

The initial code can be found here https://reviews.llvm.org/D26224
The current patch includes only the "core" GVN algorithm, i.e. the
expression framework/the logic to perform congruence finding. Other
pieces (e.g. PRE will build on top of that).

My rough plan is:
* Try to get this reviewed/tested and in tree.
* Fix miscompiles/bugs (and performance problems as/if they arise).
* Build other pieces of the algorithm on top of what we have. My
immediate concern will be implementing support for llvm.assume and
load coercion. Dan said he will try to find the time to work on the
predicate handling (actually, there's already a branch
https://github.com/dberlin/llvm-gvn-rewrite/tree/newgvn-predicates).

Please let us know what you think. Any feedback/review comment/testing
is very appreciated!

Thanks!

[1] http://lists.llvm.org/pipermail/llvm-dev/2016-March/096488.html
[2] https://llvm.org/bugs/show_bug.cgi?id=30692#c11
[3] http://dl.acm.org/citation.cfm?id=512536

--
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Daniel Berlin via llvm-dev]

First, thanks. This is a very very long time coming :)

Second, for those watching, note that pretty much all of the improvements and missing cases, including load forwarding/coercion, etc, actually are done.

It's more a matter of cleaning them up, breaking it down, and submitting it, than "implementing them".

The main experimentation at this point is "can we do it cleaner" not "can we do it" :)

It's also important to note that this new GVN also treats loads/stores in a unified way with scalars, unlike current GVN (which has no load or store value numbering).

So it will happily discover complex load/store relations (though there is some improvements we can still make here)

For example:

int vnum_test8(int *data)

{

  int i;

  int stop = data[3];

  int m = data[4];

  int n = m;

  int p;

  for (i=0; i<stop; i++) {

    int k = data[2];

    data[k] = 2;

    data[0] = m - n;

    k = data[1];

    m = m + k;

    n = n + k;

    p = data[0];

  }

  return p;

}

LLVM's current GVN will eliminate a single load here[1]

NewGVN will calculate that m and n are equal, that m-n is 0, that p is 0

It's not quite perfect yet, i haven't fixed store handling, so the following is missed:

int a;

int *p;

// LLVM is too smart and if we don't do this, realizes *p is a store to undef

void foo(){

    p = &a;

}

int main(int argc, char **argv) {

  int result;

  foo();

  *p = 2;

  if (argc)

    *p = 2;

  result = *p;

  return result;

}

Here, current LLVM GVN will do nothing, because it can't understand anything really about the stores.

GCC's GVN will determine result is 2.

NewGVN is not quite that smart yet (it require a little work to what we do to stores, and value numbering memory ssa versions)

This issue compounds if you have conditional stores of the same value.

So, for example, if you add:

if (i < 30)

  data[0] = 0;

to the first case.

GCC can still determine p is 0.

Currently, NewGVN cannot.

--Dan

[Davide Italiano via llvm-dev]

On Tue, Nov 15, 2016 at 11:29 PM, Daniel Berlin <dbe...@dberlin.org> wrote:
> First, thanks. This is a very very long time coming :)
> Second, for those watching, note that pretty much all of the improvements
> and missing cases, including load forwarding/coercion, etc, actually are
> done.
> It's more a matter of cleaning them up, breaking it down, and submitting it,
> than "implementing them".

Oh sure, bad wording. For those interested in the other pieces, this
is Dan's branch
https://github.com/dberlin/llvm-gvn-rewrite

--
Davide

[David Chisnall via llvm-dev]

This is really great to see, as I’ve spent far too much of my life over the past two years fighting with undocumented assumptions made by GVN. A couple of quick questions about the new GVN, based on problems I’ve had with the old one:

Does it assume that it’s always safe to widen a load (or store) to a power of two? For our target, this is only sound if you can show that the pointer was used to read all of the bytes that you are loading (we have byte-granularity memory safety). Old GVN has no hooks for targets to specify whether this is safe and so is implicitly assuming a page-based MMU. This optimisation is also unsound for M-profile ARM cores, though will fail occasionally there, whereas it fails deterministically for us.

Does it make any assumptions about the layout of memory in pointers? Old GVN treats pointers as integers and assumes that it’s safe to do partial stores to them. As a prerequisite for memory safety, we must be able to guarantee atomic updates to pointers and we had to hack GVN to disable a bunch of these things. In LLVM IR, pointers are opaque and there is no guarantee that their representation is the same as a same-sized integer, but old GVN makes this assumption.

David

[Daniel Berlin via llvm-dev]

On Wed, Nov 16, 2016 at 2:03 AM, David Chisnall via llvm-dev <llvm...@lists.llvm.org> wrote:

This is really great to see, as I’ve spent far too much of my life over the past two years fighting with undocumented assumptions made by GVN.  A couple of quick questions about the new GVN, based on problems I’ve had with the old one:

Does it assume that it’s always safe to widen a load (or store) to a power of two? 

I don't believe old gvn does widening any more, and new gvn certainly doesn't.

 

For our target, this is only sound if you can show that the pointer was used to read all of the bytes that you are loading (we have byte-granularity memory safety).  Old GVN has no hooks for targets to specify whether this is safe and so is implicitly assuming a page-based MMU.  This optimisation is also unsound for M-profile ARM cores, though will fail occasionally there, whereas it fails deterministically for us.

Does it make any assumptions about the layout of memory in pointers? 

Not that i know of

 

Old GVN treats pointers as integers and assumes that it’s safe to do partial stores to them.

Can you give an example?

Is this store coercion or something?
 

[Davide Italiano via llvm-dev]

On Wed, Nov 16, 2016 at 8:01 AM, Daniel Berlin <dbe...@dberlin.org> wrote:
>
>
> On Wed, Nov 16, 2016 at 2:03 AM, David Chisnall via llvm-dev
> <llvm...@lists.llvm.org> wrote:
>>
>> This is really great to see, as I’ve spent far too much of my life over
>> the past two years fighting with undocumented assumptions made by GVN. A
>> couple of quick questions about the new GVN, based on problems I’ve had with
>> the old one:
>>
>> Does it assume that it’s always safe to widen a load (or store) to a power
>> of two?
>
>
> I don't believe old gvn does widening any more, and new gvn certainly
> doesn't.
>

Yes, as it apparently blocks other optimizations. David, see
https://reviews.llvm.org/D24096 for details.
As an aside, if you're interested in combining loads, you may want to
take a look at Michael Spencer's loadcombine pass.

>>
>> For our target, this is only sound if you can show that the pointer was
>> used to read all of the bytes that you are loading (we have byte-granularity
>> memory safety). Old GVN has no hooks for targets to specify whether this is
>> safe and so is implicitly assuming a page-based MMU. This optimisation is
>> also unsound for M-profile ARM cores, though will fail occasionally there,
>> whereas it fails deterministically for us.
>>
>> Does it make any assumptions about the layout of memory in pointers?
>

Which assumptions are you thinking of?

[David Chisnall via llvm-dev]

On 16 Nov 2016, at 19:03, Davide Italiano <dav...@FreeBSD.org> wrote:
>
>>>
>>> For our target, this is only sound if you can show that the pointer was
>>> used to read all of the bytes that you are loading (we have byte-granularity
>>> memory safety). Old GVN has no hooks for targets to specify whether this is
>>> safe and so is implicitly assuming a page-based MMU. This optimisation is
>>> also unsound for M-profile ARM cores, though will fail occasionally there,
>>> whereas it fails deterministically for us.
>>>
>>> Does it make any assumptions about the layout of memory in pointers?
>>
>
> Which assumptions are you thinking of?

My last merge from upstream was about a year ago (and a new one is long overdue), but there were issues where GVN was assuming that if it did a load of a pointer then a ptrtoint, then a truncation, that it would get the same result as doing a narrower load. This is not the case in any platform where pointers are not simply integers (i.e. where you actually need inttoptr / ptrtoint instead of bitcast).

David

[Daniel Berlin via llvm-dev]

On Wed, Nov 16, 2016 at 11:11 AM, David Chisnall <David.C...@cl.cam.ac.uk> wrote:

On 16 Nov 2016, at 19:03, Davide Italiano <dav...@FreeBSD.org> wrote:
>
>>>
>>> For our target, this is only sound if you can show that the pointer was
>>> used to read all of the bytes that you are loading (we have byte-granularity
>>> memory safety).  Old GVN has no hooks for targets to specify whether this is
>>> safe and so is implicitly assuming a page-based MMU.  This optimisation is
>>> also unsound for M-profile ARM cores, though will fail occasionally there,
>>> whereas it fails deterministically for us.
>>>
>>> Does it make any assumptions about the layout of memory in pointers?
>>
>
> Which assumptions are you thinking of?

My last merge from upstream was about a year ago (and a new one is long overdue), but there were issues where GVN was assuming that if it did a load of a pointer then a ptrtoint, then a truncation, that it would get the same result as doing a narrower load.  This is not the case in any platform where pointers are not simply integers (i.e. where you actually need inttoptr / ptrtoint instead of bitcast).

You keep talking about platforms, but llvm ir itself is not platform dependent.

Can you give a reference in the language reference that says that this is not legal?

IE what loads do *on your platform* is completely irrelevant to whether the IR code is legal or not, only what it codegens to.

LLVM's type semantics (and pointers may not have types, but the load operations produce values that do) are also not defined in terms of platform, but in terms of what datalayout says, etc.

What you want seems to be non-integral pointer types.

Which are experimental:

"LLVM IR optionally allows the frontend to denote pointers in certain address spaces as “non-integral” via the datalayout string. Non-integral pointer types represent pointers that have an unspecified bitwise representation; that is, the integral representation may be target dependent or unstable (not backed by a fixed integer).

inttoptr instructions converting integers to non-integral pointer types are ill-typed, and so are ptrtoint instructions converting values of non-integral pointer types to integers. Vector versions of said instructions are ill-typed as well."

One of the reasons it's experimental is because nobody has made it work in all cases.

I think whoever wants this to work is going to have to drive fixing it and making it work sanely.

David

[Sanjoy Das via llvm-dev]

Hi all,

Daniel Berlin via llvm-dev wrote:
> My last merge from upstream was about a year ago (and a new one is
> long overdue), but there were issues where GVN was assuming that if
> it did a load of a pointer then a ptrtoint, then a truncation, that
> it would get the same result as doing a narrower load. This is not
> the case in any platform where pointers are not simply integers
> (i.e. where you actually need inttoptr / ptrtoint instead of bitcast).
>
>
> You keep talking about platforms, but llvm ir itself is not platform
> dependent.
> Can you give a reference in the language reference that says that this
> is not legal?
>
> IE what loads do *on your platform* is completely irrelevant to whether
> the IR code is legal or not, only what it codegens to.
>
> LLVM's type semantics (and pointers may not have types, but the load
> operations produce values that do) are also not defined in terms of
> platform, but in terms of what datalayout says, etc.
>
> What you want seems to be non-integral pointer types.
>
> Which are experimental:
>
> "LLVM IR optionally allows the frontend to denote pointers in certain
> address spaces as “non-integral” via the datalayout string

> <http://llvm.org/docs/LangRef.html#langref-datalayout>. Non-integral
> pointer types represent pointers that have an /unspecified/ bitwise

> representation; that is, the integral representation may be target
> dependent or unstable (not backed by a fixed integer).
>
> |inttoptr| instructions converting integers to non-integral pointer
> types are ill-typed, and so are |ptrtoint| instructions converting
> values of non-integral pointer types to integers. Vector versions of
> said instructions are ill-typed as well."
>
>
> One of the reasons it's experimental is because nobody has made it work
> in all cases.
>
> I think whoever wants this to work is going to have to drive fixing it
> and making it work sanely.

Hopefully this won't derail this thread -- but I plan to resume work on
non-integral pointers very soon (mid December - early Jan). Right now
I'm busy with some higher priority things.

We have the same problem as David C., btw, that GVN tends to freely
convert between pointers and integers. We have local patches that fix
old GVN to DTRT, and the my plan was to upstream the custom patches
predicated on the pointer types. Same with instcombine (I don't
remember if we have other patches).

I'm fine re-doing the same work on NewGVN (prevent inttoptr / ptrtoint
on certain class of pointers).

-- Sanjoy

[Daniel Berlin via llvm-dev]

Oh, cool.

 

We have the same problem as David C., btw, that GVN tends to freely convert between pointers and integers.  We have local patches that fix old GVN to DTRT, and the my plan was to upstream the custom patches predicated on the pointer types.  Same with instcombine (I don't remember if we have other patches).

Neat.

 

I'm fine re-doing the same work on NewGVN (prevent inttoptr / ptrtoint on certain class of pointers).

Sure, i'm just saying "I don't think it's forbidden except in that case, it sounds like a new feature".

[David Chisnall via llvm-dev]

On 16 Nov 2016, at 21:56, Daniel Berlin <dbe...@dberlin.org> wrote:
>
> You keep talking about platforms, but llvm ir itself is not platform dependent.
> Can you give a reference in the language reference that says that this is not legal?

Nothing in the LangRef (apart from the note about non-integral pointers, which was added recently) makes any claim about the representation of pointers. Pointers in LLVM IR have always been opaque and must explicitly be bitcast or inttoptr / ptrtoint cast to be used as if they were integers.

We have had discussions on the list previously about tightening up the semantics of inttoptr and ptrtoint.

> IE what loads do *on your platform* is completely irrelevant to whether the IR code is legal or not, only what it codegens to.
>
> LLVM's type semantics (and pointers may not have types, but the load operations produce values that do) are also not defined in terms of platform, but in terms of what datalayout says, etc.

GVN is materialising loads that go beyond the bounds of an object. This is undefined behaviour in C and there is nothing in the LangRef that indicates that this should be valid. It is only potentially valid because, on platforms with a page-based MMU as the sole form of memory protection, if you only round up to a power of two then you will still be in the same page (and, likely, cache line) so you will get some unspecified data and can ignore it.

> What you want seems to be non-integral pointer types.
>
> Which are experimental:
> "LLVM IR optionally allows the frontend to denote pointers in certain address spaces as “non-integral” via the datalayout string. Non-integral pointer types represent pointers that have an unspecified bitwise representation; that is, the integral representation may be target dependent or unstable (not backed by a fixed integer).
> inttoptr instructions converting integers to non-integral pointer types are ill-typed, and so are ptrtoint instructions converting values of non-integral pointer types to integers. Vector versions of said instructions are ill-typed as well."
>
> One of the reasons it's experimental is because nobody has made it work in all cases.
> I think whoever wants this to work is going to have to drive fixing it and making it work sanely.

Actually, that isn’t what I want, because we do define inttoptr and ptrtoint for our architecture. You can’t implement C without them (or some equivalent) working and we have a fully working C / Objective-C compiler (C++ in progress) using LLVM. ptrtoint is always valid for us, inttoptr may give null depending on the ABI and environment.

I gave a talk in the LLVM track at FOSDEM a couple of years ago about the things that are needed to make LLVM work correctly for targets where integers are not pointers. We have done most of this work, but it is not helped by people propagating the ‘integers are pointers’ assumption (which the LangRef has always been *very* careful not to state) in passes.

[Piotr Padlewski via llvm-dev]

Very nice to see it!

Piotr

[Davide Italiano via llvm-dev]

Do you happen to have a link for the talk? We'll try to make sure this
works in the new pass.

--
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare

[Arnaud De Grandmaison via llvm-dev]

Davide,

Slides and video available at https://archive.fosdem.org/2015/schedule/event/the_cheri_cpu/

Kind regards,
Arnaud

[David Chisnall via llvm-dev]

On 18 Nov 2016, at 09:42, Arnaud De Grandmaison <Arnaud.DeG...@arm.com> wrote:
>
> Davide,
>
> Slides and video available at https://archive.fosdem.org/2015/schedule/event/the_cheri_cpu/

That gives some background on our architecture. The talk I was thinking of was this one:

http://llvm.org/devmtg/2015-02/slides/chisnall-pointers-not-int.pdf

David

[Philip Reames via llvm-dev]

Glad to see this landing! It's been a long time coming.

Once this is in, please do not turn it on by default immediately. Let's
call for volunteers to find some of the most egregious miscompiles, fix
them, and then turn this on by default.

Philip

_______________________________________________

[Daniel Berlin via llvm-dev]

Agreed.

Davide and I agreed that the right play is to cut it down to the core part, and submit that, and then add all the stuff on top.

It would be really nice to get the core part of it well tested so that when we add the stuff like load coercion, etc, we don't also have to debug too many issues in the core of it.

[Davide Italiano via llvm-dev]

On Fri, Nov 18, 2016 at 8:43 AM, Philip Reames via llvm-dev
<llvm...@lists.llvm.org> wrote:
> Glad to see this landing! It's been a long time coming.
>
> Once this is in, please do not turn it on by default immediately. Let's
> call for volunteers to find some of the most egregious miscompiles, fix
> them, and then turn this on by default.
>

There are no immediate plans to enable NewGVN by default (at least,
not in the near future). In fact, the mail that I originally wrote
doesn't at all mention the switch, neither any follow-ups from me or
Daniel, so, I'm not entirely sure where you got that idea from. If you
take a look more closely (at the mail, or a the patch), you'll realize
that "key" pieces that are in old GVN are still missing. The most
noticeable are PRE and load coercion. In other words, the patch
proposed is not (yet) on par with what the current GVN does (although
all the missing pieces are already implemented out-of-tree).

Also, let me try to clarify one point. This is already a call for
volunteers. If you feel adventurous, you can download the
patch/apply/test/report issues. I can and I will spend time
integrating the rest of the work and fix all the reported
bugs/miscompiles. If there's something that can we do in a cleaner
way, a discussion will happen on the mailing list/on the review thread
and everybody will have a chance to comment, as it's happening for the
initial patch (and as I always try to do).

Once the first patch lands, I'll commit a temporary cl::opt to enable
NewGVN for those interested in testing and send another CFT e-mail.
FWIW, The patch had already a round of light testing internally. Of
course, this is not enough or indicative of its maturity/robustness. I
plan to have it tested more carefully inside my organization in
parallel.

That said, thanks for you input.

--
Davide

[Davide Italiano via llvm-dev]

On Fri, Nov 18, 2016 at 9:05 AM, Daniel Berlin via llvm-dev
<llvm...@lists.llvm.org> wrote:
> Agreed.
> Davide and I agreed that the right play is to cut it down to the core part,
> and submit that, and then add all the stuff on top.
> It would be really nice to get the core part of it well tested so that when
> we add the stuff like load coercion, etc, we don't also have to debug too
> many issues in the core of it.
>

100% agree that the core algorithm should be very well tested before
moving forward with the other pieces.

--
Davide

[Jack Howarth via llvm-dev]

>>On Fri, Nov 18, 2016 at 8:43 AM, Philip Reames via llvm-dev
><llvm-dev at lists.llvm.org> wrote:
>> Glad to see this landing! It's been a long time coming.
>>
>> Once this is in, please do not turn it on by default immediately. Let's
>> call for volunteers to find some of the most egregious miscompiles, fix
>> them, and then turn this on by default.
>>
>There are no immediate plans to enable NewGVN by default (at least,
>not in the near future). In fact, the mail that I originally wrote
>doesn't at all mention the switch, neither any follow-ups from me or
>Daniel, so, I'm not entirely sure where you got that idea from. If you
>take a look more closely (at the mail, or a the patch), you'll realize
>that "key" pieces that are in old GVN are still missing. The most
>noticeable are PRE and load coercion. In other words, the patch
>proposed is not (yet) on par with what the current GVN does (although
>all the missing pieces are already implemented out-of-tree).
>Also, let me try to clarify one point. This is already a call for
>volunteers. If you feel adventurous, you can download the
>patch/apply/test/report issues. I can and I will spend time

i>ntegrating the rest of the work and fix all the reported

>bugs/miscompiles. If there's something that can we do in a cleaner
>way, a discussion will happen on the mailing list/on the review thread
>and everybody will have a chance to comment, as it's happening for the
>initial patch (and as I always try to do).
>Once the first patch lands, I'll commit a temporary cl::opt to enable
>NewGVN for those interested in testing and send another CFT e-mail.
>FWIW, The patch had already a round of light testing internally. Of
>course, this is not enough or indicative of its maturity/robustness. I
>plan to have it tested more carefully inside my organization in
>parallel.
>That said, thanks for you input.

Why even commit the patch to enable the pass into trunk. Just leave it
up on Phabricator for testers to apply locally to their tree?
Jack

[Michael Spencer via llvm-dev]

Because that increase friction to testing it. Asking people to pass -mllvm -newgvn will end up with a lot more testing than "please apply this patch and recompile everything".

- Michael Spencer

[Greg Bedwell via llvm-dev]

Right. On trunk I can have it run through swathes of internal test code with a new flag in a matter of mouse clicks through our CI. Having to apply a patch manually is a more involved affair with a lot more manual steps. That's not to say it can't be done, but it's something I'd rather avoid where a far easier solution exists. At that point it becomes a piece of work that I have to schedule and prioritise vs my other responsibilities (and therefore might get done much later or not at all). If it's just kicking off an automated test run and reporting any bugs that turn up it's probably something I can just squeeze in straight away without any real planning.

-Greg

[Philip Reames via llvm-dev]

Same here.  Committed under an off by default flag is far preferred over "apply this patch".

Also, once it's in tree, it's much easier for multiple people to work on it at once.  Beyond all the algorithmic stuff, there's all the standard code cleanup and refactoring changes going on in tree.  Keeping an out of tree patch up to date w.r.t. API changes can be a real pain.

Philip