[PATCH] D28312: NewGVN: Fix PR 31501.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Post commit review, the general idea is obviously correct but few comments



================
Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:393-396
+  BasicBlock *PHIBlock = I->getParent();
   auto *PN = cast<PHINode>(I);
-  auto *E = new (ExpressionAllocator)
-      PHIExpression(PN->getNumOperands(), I->getParent());
+  auto *E =
+      new (ExpressionAllocator) PHIExpression(PN->getNumOperands(), PHIBlock);
----------------
The rename is NFC, but, hey, whatever.


================
Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:813-828
+  // We match the semantics of SimplifyPhiNode from InstructionSimplify here.
+
+  // See if all arguaments are the same.
+  // We track if any were undef because they need special handling.
+  bool HasUndef = false;
+  auto Filtered = make_filter_range(E->operands(), [&](const Value *Arg) {
+    if (Arg == I)
----------------
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. 
I wasn't able to find a nice way to share code between GVN and InstSimplify, so this looks good as is.


================
Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:852
+      // Only have to check for instructions
+      if (Instruction *AllSameInst = dyn_cast<Instruction>(AllSameValue))
+        if (!DT->dominates(AllSameInst, I))
----------------
`auto *`


================
Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:1870
 }
-
 bool NewGVN::eliminateInstructions(Function &F) {
----------------
Any reason why you removed this newline?


================
Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:1998-1999
           }
-          if (Member && isa<Constant>(Member))
-            assert(isa<Constant>(CC->RepLeader));
 
----------------
Why this assertion doesn't hold anymore?


Repository:
  rL LLVM

https://reviews.llvm.org/D28312