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

[Daniel Berlin via llvm-commits]

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/20160114/d63b7972/attachment.html>