<div dir="ltr">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).<div><br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 14, 2016 at 3:31 PM, David Kreitzer <span dir="ltr"><<a href="mailto:david.l.kreitzer@intel.com" target="_blank">david.l.kreitzer@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">DavidKreitzer added a comment.<br>
<br>
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.<br>
<br>
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)".<br>
<br>
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:<br>
<br>
  /// The given values are known to be equal at a certain point, P, in the program.<br>
  /// When DominatesByEdge is true, that point is the CFG edge Root.<br>
  /// When DominatesByEdge is false, that point is the Instruction Instr.<br>
  /// Exploit this, for example by replacing 'LHS' with 'RHS' at all uses dominated by P.<br>
  /// Returns whether a change was made.<br>
  bool GVN::propagateEquality(Value *LHS, Value *RHS, const BasicBlockEdge &Root,<br>
                              Instruction *Instr, bool DominatesByEdge) {<br>
<br>
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.)<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16100" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16100</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>