<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 6, 2017 at 6:04 PM, 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 added a comment.<br>
<br>
Post commit review, the general idea is obviously correct but few comments<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp:393-396<br>
+  BasicBlock *PHIBlock = I->getParent();<br>
   auto *PN = cast<PHINode>(I);<br>
-  auto *E = new (ExpressionAllocator)<br>
-      PHIExpression(PN-><wbr>getNumOperands(), I->getParent());<br>
+  auto *E =<br>
+      new (ExpressionAllocator) PHIExpression(PN-><wbr>getNumOperands(), PHIBlock);<br>
----------------<br>
The rename is NFC, but, hey, whatever.<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp:813-828<br>
+  // We match the semantics of SimplifyPhiNode from InstructionSimplify here.<br>
+<br>
+  // See if all arguaments are the same.<br>
+  // We track if any were undef because they need special handling.<br>
+  bool HasUndef = false;<br>
+  auto Filtered = make_filter_range(E->operands(<wbr>), [&](const Value *Arg) {<br>
+    if (Arg == I)<br>
----------------<br>
Somehow duplicating the logic here is not ideal. I don't expect the semantic of `SimplifyPHINode` to change anytime soon, but in case it would, we're in a situation where we should keep the two copies in sync.<br>
I wasn't able to find a nice way to share code between GVN and InstSimplify, so this looks good as is.<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp:852<br>
+      // Only have to check for instructions<br>
+      if (Instruction *AllSameInst = dyn_cast<Instruction>(<wbr>AllSameValue))<br>
+        if (!DT->dominates(AllSameInst, I))<br>
----------------<br>
`auto *`<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp:1870<br>
 }<br>
-<br>
 bool NewGVN::eliminateInstructions(<wbr>Function &F) {<br>
----------------<br>
Any reason why you removed this newline?<br>
<br></blockquote><div>No :(</div><div>i'll put it back.</div><div><br></div><div>I have clang-format auto-run on save, and sometimes... </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp:1998-1999<br>
           }<br>
-          if (Member && isa<Constant>(Member))<br>
-            assert(isa<Constant>(CC-><wbr>RepLeader));<br>
<br>
----------------<br>
Why this assertion doesn't hold anymore?<br></blockquote><div><br></div><div>It is verified directly now.</div><div><br></div></div></div></div>