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

[Zhang, Yuanrui via llvm-commits]

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.

I am wondering if you have any plan to rewrite this function. 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<mailto: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/fe3acf64/attachment.html>