<div dir="ltr">Hi Daniel, Hal,<div><br></div><div>Daniel, thanks for the advice. I think letting the PRE step do PRE on loads too is a good move. I'm swamped at the moment but will send out a new patch in a few days time.</div>
<div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 13 August 2014 17:35, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">----- Original Message -----<br>
> From: "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
</div><div><div class="h5">> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "James Molloy" <<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>><br>
> Sent: Wednesday, August 13, 2014 11:01:22 AM<br>
> Subject: Re: [PATCH] Make GVN more iterative<br>
><br>
> On Tue, Aug 12, 2014 at 10:48 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
> > ----- Original Message -----<br>
> >> From: "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
> >> To: "James Molloy" <<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>><br>
> >> Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> >> Sent: Tuesday, August 12, 2014 11:26:10 AM<br>
> >> Subject: Re: [PATCH] Make GVN more iterative<br>
> >><br>
> >> On Tue, Aug 12, 2014 at 2:25 AM, James Molloy<br>
> >> <<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>><br>
> >> wrote:<br>
> >> > Hi Daniel,<br>
> >> ><br>
> >> > The 5% speedup in compile time is almost certainly entirely<br>
> >> > noise.<br>
> >> > That figure was got from running the LNT suite on a core i7.<br>
> >> ><br>
> >> > You're right that in this testcase only one load is missed<br>
> >> > currently, but that is 50% of the loads in the testcase! The<br>
> >> > problem is chains of partially redundant loads. The reduced<br>
> >> > testcase that inspired this (taken from 450.soplex, where we<br>
> >> > lose<br>
> >> > 15% of our runtime due to it!) is:<br>
> >> ><br>
> >> > double f(int stat, int i, double * restrict * restrict p)<br>
> >> > {<br>
> >> >     double x;<br>
> >> >     switch (stat)<br>
> >> >     {<br>
> >> >         case 0:<br>
> >> >         case 1:<br>
> >> >             x = p[0][i] - 1;<br>
> >> >             if (x < 0)<br>
> >> >                 return x;<br>
> >> >         case 2:<br>
> >> >             return 3 - p[0][i];<br>
> >> >         default:<br>
> >> >             return 0;<br>
> >> >     }<br>
> >> > }<br>
> >> ><br>
> >> > You sound like an expert on the GVN code, which I certainly am<br>
> >> > not.<br>
> >> > I've worked with PRE heavily before, but that was in a different<br>
> >> > compiler that did not use SSA so the algorithm was totally<br>
> >> > different (and GVN didn't exist). Having looked at the LLVM<br>
> >> > algorithm, the first (GVN) stage performs PRE of loads, but the<br>
> >> > second stage performs PRE of non-loads.<br>
> >><br>
> >> Yes.  This is because GVN does not really value number memory, it<br>
> >> uses<br>
> >> memdep to try to get it to tell whether the loads look the same<br>
> >> (which<br>
> >> isn't really the same :P).  As such, PRE of memory is performed at<br>
> >> the<br>
> >> point where it is asking memdep questions (and in fact, is mostly<br>
> >> independent of the rest of GVN, except for one small part).<br>
> >><br>
> >> As such, it will miss things, like you describe, where you end up<br>
> >> with<br>
> >> partially redundant loads that interact with partially redundant<br>
> >> scalars (and other cases, since the Load PRE does not handle<br>
> >> partial<br>
> >> availability)<br>
> >><br>
> >> ><br>
> >> > This is obviously going to result in missing PRE opportunities.<br>
> >> > The<br>
> >> > example above ends up with a chain where you need to spot the<br>
> >> > first load is partially redundant (which GVN duly does), then<br>
> >> > spot<br>
> >> > that the "sext i32 -> i64" afterwards is partially redundant<br>
> >> > (which the second stage PRE duly does), then notice the next<br>
> >> > load<br>
> >> > is now redundant (woops, we never do load PRE again at this<br>
> >> > point!)<br>
> >> ><br>
> >> > I don't see it as "we're missing just one load", I see it as<br>
> >> > "LLVM's implementation of a truly classical compiler<br>
> >> > optimization<br>
> >> > is really weak".<br>
> >><br>
> >> I 100% agree with you, but this is a known issue.  Nobody has had<br>
> >> the<br>
> >> werewithal to actually solve it, and people keep piling on hacks<br>
> >> and<br>
> >> bandaids, which is how GCC got so slow in the end.  There has to<br>
> >> be a<br>
> >> stop loss point or GVN will just get ever slower.<br>
> >><br>
> >> > What do you think? Should we implement a new algorithm, or make<br>
> >> > it<br>
> >> > more iterative?<br>
> >> ><br>
> >><br>
> >> Realistically, we should change the algorithm. But this is a lot<br>
> >> of<br>
> >> work. (if you are interested, i can help, and there is even a<br>
> >> GVN+PRE<br>
> >> implementation in LLVM's sourcetree, if you look at the version<br>
> >> control history of Transforms/Scalar)<br>
> >><br>
> >> However,  you could go halfway for now.<br>
> >> There is nothing, IIRC, that should stop you from updating the<br>
> >> availability tables after the scalar PRE, and then just iterating<br>
> >> that<br>
> >> + load PRE (without the rest of GVN).  The load PRE does not<br>
> >> really<br>
> >> depend on anything interesting, last i looked.<br>
> >><br>
> >> > It only re-iterates if PRE actually does something, so I don't<br>
> >> > think this should slow the compiler down massively.<br>
> >><br>
> >><br>
> >> 3% on an average testcase is a lot to catch essentially 0.1% more<br>
> >> loads ;)<br>
> ><br>
> > This seems like a good example of something to enable in an<br>
> > aggressive optimization mode above what we currently have (a -O4,<br>
> > for example).<br>
><br>
> I'd agree with this :)<br>
><br>
> >  I realize you're joking, but, I think that "0.1% more loads" is<br>
> >  likely unfair, we'd actually need to look at what happened in<br>
> >  this test case.<br>
><br>
> The test case is contrived, of course, so it does 50% there.<br>
><br>
> But I did actually run it on some benchmarks i had, and, on average,<br>
> it removed single digit more loads per file.<br>
> There was no measurable speedup, at least on those benchmarks.  But I<br>
> know, when we added similar support to GCC, we did find some real<br>
> benchmarks that improved, so i have no doubt it'll make some<br>
> difference to people :)<br>
><br>
><br>
> > Nevertheless, if hypothetically I could spend 3% more time to get a<br>
> > 0.1% speedup,  and I can extrapolate that to spending 3x the time<br>
> > to get a 10% speedup, I have many users who would gladly pay that<br>
> > price (at least for production builds).<br>
><br>
> > Generally speaking, we guard our compile time very carefully, and<br>
> > that's a good thing, but we should not exclude useful (and easily<br>
> > maintainable)<br>
> > optimization improvements from our codebase, even if we don't<br>
> > include them in the default -O3 optimization pipeline.<br>
><br>
> I'd agree with this.<br>
> But, i'm not sure it's quite to the maintainable stage yet, IMHO.<br>
> But maybe we differ in opinion on that. I would not, for example,<br>
> consider iterating any particular pass repeatedly to be a good<br>
> solution when we know of better ways to accomplish the same goal (due<br>
> to known deficiencies or whatever).<br>
> In this case, i haven't asked him to actually rewrite GVN, i've asked<br>
> him to iterate only the part actually causing the optimization, and<br>
> maybe at some higher opt level.  I expect this to achieve his goal,<br>
> be<br>
> smaller, better to maintain, and get us a little farther down the<br>
> road<br>
> to sanity.<br>
<br>
</div></div>Sounds good to me :-)<br>
<br>
Thanks again,<br>
Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> ><br>
> >  -Hal<br>
> ><br>
> >> It's only because this load is important that anyone cares.<br>
> >><br>
> >><br>
> >> > I ran GVN over a clang.bc, and got this:<br>
> >> ><br>
> >> > Before my patch (release+asserts):<br>
> >> >   10.1046 ( 47.6%)   0.6320 ( 51.3%)  10.7367 ( 47.8%)  10.7387<br>
> >> >   (<br>
> >> >   47.7%)  Global Value Numbering<br>
> >> ><br>
> >> > After my patch (release+asserts):<br>
> >> >   10.2886 ( 48.2%)   0.7441 ( 55.9%)  11.0327 ( 48.6%)  11.0000<br>
> >> >   (<br>
> >> >   48.4%)  Global Value Numbering<br>
> >> ><br>
> >> > Which is 0.3s (2.8%) worse, which doesn't sound like it should<br>
> >> > be a<br>
> >> > show-stopper to me! Perhaps you could run it on your own<br>
> >> > testcases?<br>
> >> ><br>
> >> > Cheers,<br>
> >> ><br>
> >> > James<br>
> >> ><br>
> >> > -----Original Message-----<br>
> >> > From: Daniel Berlin [mailto:<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>]<br>
> >> > Sent: 11 August 2014 23:49<br>
> >> > To: James Molloy<br>
> >> > Cc: Owen Anderson (<a href="mailto:resistor@mac.com">resistor@mac.com</a>); llvm-commits<br>
> >> > Subject: Re: [PATCH] Make GVN more iterative<br>
> >> ><br>
> >> > On Mon, Aug 11, 2014 at 3:25 PM, Daniel Berlin<br>
> >> > <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> >> >> We have cases where GVN is *really slow* (IE 29 seconds and 80%<br>
> >> >> of<br>
> >> >> compile time). Iterating it again is likely to make that worse.<br>
> >> >>  Happy<br>
> >> >> to give them to you (or test it with this patch)<br>
> >> >><br>
> >> >> You say you saw a 5% speedup in compile time, which seems<br>
> >> >> really<br>
> >> >> odd.<br>
> >> >><br>
> >> >> What exactly did you run it on?<br>
> >> >><br>
> >> >> Additionally, it kind of sounds like you are saying all this<br>
> >> >> does<br>
> >> >> is<br>
> >> >> remove 1 additional load for this testcase. Do you have more<br>
> >> >> general<br>
> >> >> performance numbers?<br>
> >> >> Iterating all of GVN to eliminate a single load seems like a<br>
> >> >> pretty<br>
> >> >> heavy hammer.<br>
> >> ><br>
> >> > To be clear, LLVM used to have a GVNPRE implementation, but it<br>
> >> > was<br>
> >> > decided this wasn't worth the cost of what it got.<br>
> >> > What you are doing is effectively re-adding that, but without an<br>
> >> > integrated algorithm that was O(better time bounds).<br>
> >> > Thus, this patch, at a glance, seems like the wrong approach.<br>
> >> > If we really have a bunch of cases with significant performance<br>
> >> > benefits from GVN + PRE, then that would point towards moving<br>
> >> > back<br>
> >> > towards GVN-PRE, which is why the comment says "   // Actually,<br>
> >> > when<br>
> >> > this happens, we should just fully integrate PRE into GVN."<br>
> >> ><br>
> >> ><br>
> >> ><br>
> >> ><br>
> >> ><br>
> >> >><br>
> >> >><br>
> >> >><br>
> >> >> On Mon, Aug 11, 2014 at 11:29 AM, James Molloy<br>
> >> >> <<a href="mailto:James.Molloy@arm.com">James.Molloy@arm.com</a>> wrote:<br>
> >> >>> Hi all,<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> GVN currently iterates until it can do no more, then performs<br>
> >> >>> PRE.<br>
> >> >>> There’s a FIXME, saying we should try GVN again if PRE made<br>
> >> >>> changes.<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> This patch changes GVN to do this, motivated by the attached<br>
> >> >>> testcase<br>
> >> >>> (reduced from SPEC) where GVN currently leaves a redundant<br>
> >> >>> load.<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> The FIXME mentions memory dependence checking, but it looks to<br>
> >> >>> me<br>
> >> >>> like the memory dependence updating got implemented after the<br>
> >> >>> FIXME<br>
> >> >>> was written, so it’s out of date.<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> I’ve tested this for compile time and there are no non-noise<br>
> >> >>> regressions (in fact, the geomean was a 5% speedup,<br>
> >> >>> interestingly).<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> What do you think?<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> Cheers,<br>
> >> >>><br>
> >> >>><br>
> >> >>><br>
> >> >>> James<br>
> >> >>><br>
> >> >>><br>
> >> >>> -- IMPORTANT NOTICE: The contents of this email and any<br>
> >> >>> attachments<br>
> >> >>> are confidential and may also be privileged. If you are not<br>
> >> >>> the<br>
> >> >>> intended recipient, please notify the sender immediately and<br>
> >> >>> do<br>
> >> >>> not<br>
> >> >>> disclose the contents to any other person, use it for any<br>
> >> >>> purpose, or<br>
> >> >>> store or copy the information in any medium. Thank you.<br>
> >> >>><br>
> >> >>> ARM Limited, Registered office 110 Fulbourn Road, Cambridge<br>
> >> >>> CB1<br>
> >> >>> 9NJ,<br>
> >> >>> Registered in England & Wales, Company No: 2557590 ARM<br>
> >> >>> Holdings<br>
> >> >>> plc,<br>
> >> >>> Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,<br>
> >> >>> Registered in<br>
> >> >>> England & Wales, Company No: 2548782<br>
> >> >>><br>
> >> >>> _______________________________________________<br>
> >> >>> llvm-commits mailing list<br>
> >> >>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> >> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> >> >>><br>
> >> ><br>
> >> ><br>
> >> ><br>
> >> ><br>
> >><br>
> >> _______________________________________________<br>
> >> llvm-commits mailing list<br>
> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> >><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>