[PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction

Wed Jan 20 13:21:43 PST 2016

Sorry guys, I had a lot of things to do and I forgot about it.
I don't think it makes lot of sense to refactor GVN right now - Daniel is
working on new GVN, I don't know how close is he. But it's true that the
GVN we have right now is a little awful.

The thing that could fix issue with propageEquality, is that the code for
propagation by the block should be simpler than the general code that we
have here, but I don't know how much code would it have in common.

I don't have much time right now to think about it, so I hope you will find
the way to fix it.
You can also ask Nick Lewycky for help.

Piotr

2016-01-20 22:01 GMT+01:00 Daniel Berlin <dberlin at dberlin.org>:

>
>
> On Wed, Jan 20, 2016 at 12:59 PM, Zhang, Yuanrui <yuanrui.zhang at intel.com>
> wrote:
>
>> Hi Piotr,
>>
>>
>>
>> Did you get a time to look at the fix I uploaded for the second time?
>> Does it look good to you?
>>
>>
>>
>>    Daniel’s suggestion on restructuring the propagateEquality is good and
>> necessary, since many developers find this function confusing.
>>
>>
>>
>> Dave and I tried the idea he mentioned below, i.e., the new interface,
>> but it seems that it does not help much to eliminate the code confusion in
>> that function, and not simpler than the current fix.
>>
>
> Sigh.
>
> Okay, i think then we should just punt on it for now.  Once we push
> post-dominators through the stack, we'll just rewrite it to do this the
> traditional post-dominance way.
>
>
>>
>> I am wondering if you have any plan to rewrite this function.
>>
>
> I believe I am the only person who is trying to clean up GVN, and i'm
> doing it by rewriting it entirely :P
> .
>
>> Are you familiar with the code? If not, do you know who is familiar with
>> this code, and can assist us in doing the rewriting. We can all help if
>> needed, but we need an expert first.
>>
>>
>>
>> Thank you very much!
>>
>>
>>
>> Regards,
>> Ray
>>
>> *From:* Daniel Berlin [mailto:dberlin at dberlin.org]
>> *Sent:* Thursday, January 14, 2016 3:42 PM
>> *To:* reviews+D16100+public+7f8d41cf7877574b at reviews.llvm.org
>> *Cc:* Zhang, Yuanrui <yuanrui.zhang at intel.com>; Nick Lewycky <
>> nlewycky at google.com>; Piotr Padlewski <piotr.padlewski at gmail.com>;
>> Kreitzer, David L <david.l.kreitzer at intel.com>; llvm-commits <
>> llvm-commits at lists.llvm.org>
>> *Subject:* Re: [PATCH] D16100: Fix for two constant propagation problems
>> in GVN with assume intrinsic instruction
>>
>>
>>
>> To start with, I'm fine with committing this patch if we agree to address
>> the other issues, *somehow*, in a followup (either by y'all or by Piotr).
>>
>>
>>
>>
>>
>> On Thu, Jan 14, 2016 at 3:31 PM, David Kreitzer <
>> david.l.kreitzer at intel.com> wrote:
>>
>> DavidKreitzer added a comment.
>>
>> I like Ray's plan of first making the targeted stability fix and then
>> deciding how best to restructure propagateEquality to make it clearer. The
>> bug illustrated by the @_Z1im test case is pretty egregious and ought to be
>> fixed promptly IMO.
>>
>> Daniel, you suggested possibly splitting propagateEquality, but there is
>> a lot of functionality in there that you'd like to share between the assume
>> intrinsic caller and the other callers. For example, the code that infers A
>> == 1 && B == 1 from A && B == 1 applies equally regardless of whether A &&
>> B == 1 came from "assume(A && B)" or "if (A && B)".
>>
>> I think the fundamental difference between assume and the other callers
>> of propagateEquality is that for assume, the equality property becomes
>> known true immediately following the assume instruction while for the other
>> callers, the equality property becomes known true along the outgoing edge
>> of the block ending in the conditional branch or switch. It shouldn't be
>> hard to make that distinction in the interface to propagateEquality.
>> Perhaps something like this:
>>
>>   /// The given values are known to be equal at a certain point, P, in
>> the program.
>>   /// When DominatesByEdge is true, that point is the CFG edge Root.
>>   /// When DominatesByEdge is false, that point is the Instruction Instr.
>>   /// Exploit this, for example by replacing 'LHS' with 'RHS' at all uses
>> dominated by P.
>>   /// Returns whether a change was made.
>>   bool GVN::propagateEquality(Value *LHS, Value *RHS, const
>> BasicBlockEdge &Root,
>>                               Instruction *Instr, bool DominatesByEdge) {
>>
>> When DominatesByEdge is true, Instr would be ignored. And when
>> DominatesByEdge is false, Root would be ignored. Then the dominance check
>> for the assume case becomes one of "does this instruction dominate" rather
>> than "does this block dominate". (Note that this lets us get rid of the
>> "for each successor" loop inside processAssumeIntrinsic, which is pretty
>> artificial. I suspect it was written that way just to make it easier to
>> reuse propagateEquality.)
>>
>>
>> http://reviews.llvm.org/D16100
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160120/52e7ddf3/attachment.html>