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

[Pedro Ferreira via Phabricator via llvm-commits]

PFerreira created this revision.
Herald added a reviewer: dberlin.

Related to bugzilla 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).
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 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.
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

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 --------------
A non-text attachment was scrubbed...
Name: D35475.106840.patch
Type: text/x-patch
Size: 598 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170717/00c29538/attachment.bin>