<div dir="ltr"><div>I don't think NewGVN should do this, instead of a cleanup pass, unless it actually produces better analysis results.</div><div><br></div><div>I'm already tempted to have us stop wasting time checking isInstructionTriviallyDead.</div><div><br></div><div>I know it matches what GVN does, but that doesn't seem a good reason to me, and i don't see how it would improve a PRE that properly does dataflow analysis.<br></div><div>In fact, you can prove it can't :)</div><div><br></div><div>(if the branch is unconditional, and can be merged, it's existence should provably not affect the solution to PRE's dataflow equations).</div><div><br></div><div>Does it actually improve elimination?<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 26, 2016 at 7:33 AM, Davide Italiano 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">davide updated this revision to Diff 82507.<br>
<span class="">davide added a comment.<br>
<br>
fix a typo<br>
<br>
<br>
</span><span class=""><a href="https://reviews.llvm.org/D28117" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28117</a><br>
<br>
Files:<br>
  lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
  test/Transforms/NewGVN/basic.<wbr>ll<br>
<br>
<br>
Index: test/Transforms/NewGVN/basic.<wbr>ll<br>
==============================<wbr>==============================<wbr>=======<br>
--- test/Transforms/NewGVN/basic.<wbr>ll<br>
+++ test/Transforms/NewGVN/basic.<wbr>ll<br>
@@ -1,4 +1,3 @@<br>
-; XFAIL: *<br>
 ; RUN: opt < %s -newgvn -S | FileCheck %s<br>
<br>
 define i32 @main() {<br>
Index: lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
+++ lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
@@ -1378,6 +1378,20 @@<br>
   unsigned ICount = 0;<br>
   SmallPtrSet<BasicBlock *, 16> VisitedBlocks;<br>
<br>
+  // Do a sweep over all the basic blocks to merge unconditional branches.<br>
+  for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE;) {<br>
+    BasicBlock *BB = &*FI++;<br>
+<br>
+    // In theory here we could pass MemSSA if/when MergeBlockIntoPredecessor<br>
</span>+    // will grow a version which accepts the analysis.<br>
<div class="HOEnZb"><div class="h5">+    bool removedBlock = MergeBlockIntoPredecessor(<br>
+        BB, DT, nullptr /* LoopInfo */, nullptr /* MemDep */);<br>
+<br>
+    if (removedBlock)<br>
+      NumGVNBlocksDeleted++;<br>
+    Changed |= removedBlock;<br>
+  }<br>
+<br>
   // Note: We want RPO traversal of the blocks, which is not quite the same as<br>
   // dominator tree order, particularly with regard whether backedges get<br>
   // visited first or second, given a block with multiple successors.<br>
<br>
<br>
</div></div></blockquote></div><br></div>