[PATCH] D35475: [PATCH] [GVN] Ensure replacement instruction dominates its uses

[Pedro Ferreira via llvm-commits]

On 17/07/17 15:48, Daniel Berlin wrote:
>
>
> On Mon, Jul 17, 2017 at 2:04 AM, Pedro Ferreira via Phabricator
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>
>     PFerreira created this revision.
>     Herald added a reviewer: dberlin.
>
>     Related to bugzilla 33731
>     https://bugs.llvm.org/show_bug.cgi?id=33731
>     <https://bugs.llvm.org/show_bug.cgi?id=33731>
>
>     This is caused by a a PRE insertion when the predecessor block
>     hasn't yet been through Value Numbering (due to a back-edge).
>
>
> I'm not surprised this is buggy, sadly.
> I suspect there are a bunch of issues around this kind of thing. 
>
>     The PRE insertion adds a duplicate instruction at the end of the
>     block, but the already-existing instruction is already used in the
>     predecessor block, like so:
>
>     L.LB1_346:                                        ; preds = %L.LB1_423
>
>       %7 = sext i32 %2 to i64
>       %8 = mul i64 %7, 4
>
>  
>
>     - %.pre = sext i32 %1 to i64** ; inserted by
>     GVN::performScalarPREInsertion
>
>     When we then iterate over this block (later in the pass), GVN
>     chooses to delete the first instance of the instruction as it is
>     duplicated (instead of the later one which was created before).
>
> This sounds like an issue with findLeader/addToLeaderTable not
> checking local dominance if they are in the same block, based on the
> assumption that it will process the block in order.

I agree; there is a comment in "performScalarPREInsertion" stating
exactly that.

> addToLeaderTable always adds to the end, and so it will hit the PRE
> leader before any other.
>
> If so, the fastest fix i can think of is to  number the instructions
> in the block starting at 0 as we process them. An instruction can only
> be a leader when BB = LeaderBB iff the number is < than the current
> instruction's number.
> Assign all PRE'd instructions numbers that are MAX_INT.

I'm not sure I understand - the instruction in particular has already
been numbered on a different block. That is, there are equivalent
instructions in other blocks which DO get numbered, so essentially CSE
will eventually move the instruction to a common dominator (or at least
it should).
That being said, the "%7 = sext i32 %2 to i64" already has a number. The
problem is that this particular instance has not been added to the
leader list and so PREInsertion adds one (since it assumes there isn't one).

>
> If you have two MAX_INT instructions, they are in-order already (since
> PRE will only add them in order).
>
>
>  
>
>     This is because "GVN::performScalarPREInsertion" will number the
>     instruction as it is created (so it is in the leader-list). 
>
>
>     The attached patch moves the "%.pre" instruction up so that the
>     replacement is valid.
>
>     I am not sure this is a valid fix, but I couldn't figure out
>     another solution.
>
> There are a couple other ways to fix this.
> Another would be "always check local dominance using
> OrderedInstruction in FindLeader ". This is equivalent to the above.

But wouldn't that require that the instruction exists in the leader list?
If I do check the dominance, wouldn't I have to scan the incoming block
anyway?

I do have another alternative; just before where I added the instruction
moving, we have:

  Value *Repl = findLeader(I->getParent(), Num);
  if (!Repl) {
    // Failure, just remember this instance for future use.
    addToLeaderTable(Num, I, I->getParent());
    return false;
  } else if (Repl == I) {
    // If I was the result of a shortcut PRE, it might already be in the
table
    // and the best replacement for itself. Nothing to do.
    return false;
  }

we could append to this if-else block:

else if (DT->dominates(I,R)) {
  // Replace the existing leader since this one dominates it.
  replaceOnLeaderTable(Num, Repl, I);
}

This way we'd preserve the existing instruction (instead of replacing it
with the "%.pre" one).


>     I though about stoping "GVN::performScalarPREInsertion" from
>     inserting the instruction by numbering the BasicBlock, but that
>     might lead to infinite loops. I also tried to stop PREInsertion
>     from running if the BasicBlock had not been numbered yet (by
>     keeping a list of numbered blocks) but that wouldn't work since
>     GVN only does one pass over the function (so this case would never
>     be optimised out).
>
>     The patch does fix the bug though, without adding regressions.
>
>
>     https://reviews.llvm.org/D35475 <https://reviews.llvm.org/D35475>
>
>     Files:
>       lib/Transforms/Scalar/GVN.cpp
>
>
>     Index: lib/Transforms/Scalar/GVN.cpp
>     ===================================================================
>     --- lib/Transforms/Scalar/GVN.cpp
>     +++ lib/Transforms/Scalar/GVN.cpp
>     @@ -1807,6 +1807,13 @@
>          return false;
>        }
>
>     +  // If this was a shortcut PRE, it will have been inserted at
>     the end
>     +  // of the block; ensure that it dominates all uses of the original.
>     +  if (Instruction *R = dyn_cast<Instruction>(Repl)) {
>     +    if (DT->dominates(I, R)) {
>     +      R->moveBefore(I);
>     +    }
>     +  }
>        // Remove it!
>        patchAndReplaceAllUsesWith(I, Repl);
>        if (MD && Repl->getType()->isPtrOrPtrVectorTy())
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170717/7d29c9c7/attachment.html>