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

[Daniel Berlin via llvm-commits]

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/1a97c3c0/attachment.html>