[PATCH] Make GVN more iterative

[Daniel Berlin]

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
>>