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