<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 17, 2017 at 2:04 AM, Pedro Ferreira via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">PFerreira created this revision.<br>
Herald added a reviewer: dberlin.<br>
<br>
Related to bugzilla 33731 <a href="https://bugs.llvm.org/show_bug.cgi?id=33731" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_<wbr>bug.cgi?id=33731</a><br>
<br>
This is caused by a a PRE insertion when the predecessor block hasn't yet been through Value Numbering (due to a back-edge).<br></blockquote><div><br></div><div>I'm not surprised this is buggy, sadly.</div><div>I suspect there are a bunch of issues around this kind of thing. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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:<br>
<br>
L.LB1_346:                                        ; preds = %L.LB1_423<br>
<br>
  %7 = sext i32 %2 to i64<br>
  %8 = mul i64 %7, 4<br>
<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- %.pre = sext i32 %1 to i64** ; inserted by GVN::performScalarPREInsertion<br>
<br>
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).</blockquote><div>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.</div><div>addToLeaderTable always adds to the end, and so it will hit the PRE leader before any other.</div><div><br></div><div>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.</div><div>Assign all PRE'd instructions numbers that are MAX_INT.<br></div><div><br></div><div>If you have two MAX_INT instructions, they are in-order already (since PRE will only add them in order).</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This is because "GVN::<wbr>performScalarPREInsertion" will number the instruction as it is created (so it is in the leader-list). </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The attached patch moves the "%.pre" instruction up so that the replacement is valid.<br>
<br>
I am not sure this is a valid fix, but I couldn't figure out another solution.<br></blockquote><div>There are a couple other ways to fix this.</div><div>Another would be "always check local dominance using OrderedInstruction in FindLeader ". This is equivalent to the above. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I though about stoping "GVN::<wbr>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).<br>
<br>
The patch does fix the bug though, without adding regressions.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35475" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D35475</a><br>
<br>
Files:<br>
  lib/Transforms/Scalar/GVN.cpp<br>
<br>
<br>
Index: lib/Transforms/Scalar/GVN.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Transforms/Scalar/GVN.cpp<br>
+++ lib/Transforms/Scalar/GVN.cpp<br>
@@ -1807,6 +1807,13 @@<br>
     return false;<br>
   }<br>
<br>
+  // If this was a shortcut PRE, it will have been inserted at the end<br>
+  // of the block; ensure that it dominates all uses of the original.<br>
+  if (Instruction *R = dyn_cast<Instruction>(Repl)) {<br>
+    if (DT->dominates(I, R)) {<br>
+      R->moveBefore(I);<br>
+    }<br>
+  }<br>
   // Remove it!<br>
   patchAndReplaceAllUsesWith(I, Repl);<br>
   if (MD && Repl->getType()-><wbr>isPtrOrPtrVectorTy())<br>
<br>
<br>
</blockquote></div><br></div></div>