[PATCH] D11918: Constant propagation after hiting assume(icmp)

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:611-612
@@ -610,2 +610,4 @@
 
+    // Map used to replace values with constants in the same BB.
+    SmallMapVector<llvm::Value*, llvm::Constant*, 4> ReplaceWithConstMap;
     SmallVector<Instruction*, 8> InstrsToErase;
----------------
The description is not wrong, but I would've described it differently:
  // Block-local map of equivalent values to their leader, does not propagate to any successors. Entries added mid-block are applied to the remaining instructions in the block.
  SmallMapVector<llvm::Value*, llvm::Constant*, 4> LocalLeaderMap;
Notably there's nothing *const-y* about it. We often replace X with Y when both are Instructions, or when Y is an Argument, or in this case when Y is a Constant. What's special about this is that it's block-local.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:724
@@ -721,2 +723,3 @@
     BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ);
+    bool replaceOperandsWithConsts(Instruction *instr) const;
     bool propagateEquality(Value *LHS, Value *RHS, const BasicBlockEdge &Root);
----------------
s/instr/I/

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2040
@@ +2039,3 @@
+bool GVN::replaceOperandsWithConsts(Instruction *Instr) const {
+
+  bool Changed = false;
----------------
Extra blank.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2042
@@ +2041,3 @@
+  bool Changed = false;
+  for (unsigned OpNum = 0 ; OpNum < Instr->getNumOperands() ; ++OpNum) {
+    Value *operand = Instr->getOperand(OpNum);
----------------
Extra spaces before semicolons

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2228
@@ +2227,3 @@
+    if (IntrinsicI->getIntrinsicID() == Intrinsic::assume) {
+
+      Value *V = IntrinsicI->getArgOperand(0);
----------------
Extr ablank.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2232
@@ +2231,3 @@
+        if (ICmpI->getSignedPredicate() == ICmpInst::Predicate::ICMP_EQ) {
+
+          bool Changed = false;
----------------
Extra blank.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2234-2235
@@ +2233,4 @@
+          bool Changed = false;
+          Value *Lhs = ICmpI->getOperand(0);
+          Value *Rhs = ICmpI->getOperand(1);
+          Constant *LhsConst = dyn_cast<Constant>(Lhs);
----------------
LHS and RHS. If you're worried about shadowing the existing LHS and RHS, I'd suggest ICmpLHS and ICmpRHS or ICmpOp0 and ICmpOp1.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2239-2240
@@ +2238,4 @@
+
+          // Propagate equality only if only one of operand is const.
+          if ((LhsConst != nullptr) ^ (RhsConst != nullptr)) {
+            for (BasicBlock *successor : successors(I->getParent())) {
----------------
The common idiom is to check which is Constant and then use std::swap.

However, we canonicalize constants to the RHS when possible (ie., not "sub" instructions). Any reason it would be de-canonicalized at this point?

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2244
@@ +2243,3 @@
+
+                // equality propagation can't be done for every
+                // successor, but propagateEquality checks it
----------------
Capitalize.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2245
@@ +2244,3 @@
+                // equality propagation can't be done for every
+                // successor, but propagateEquality checks it
+                Changed |= propagateEquality(Lhs,
----------------
Period.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2246-2248
@@ +2245,5 @@
+                // successor, but propagateEquality checks it
+                Changed |= propagateEquality(Lhs,
+                                             Rhs,
+                                             Edge);
+            }
----------------
Surely this fits on fewer lines?

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2259
@@ +2258,3 @@
+      }
+
+    }
----------------
Extra blank.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2442
@@ -2380,1 +2441,3 @@
+
+    ChangedFunction |= replaceOperandsWithConsts(BI);
     ChangedFunction |= processInstruction(BI);
----------------
I suspect this should have a quick exit for the common case where ReplaceWithConstMap is empty. Putting the test here might make sense, so that we don't even call it at -O0.

We surely don't want to iterate over the operands calling find on an empty map.

================
Comment at: test/Transforms/GVN/assume-ptr-equal-same-bb.ll:1
@@ +1,2 @@
+; RUN: opt < %s -gvn -S | grep " call void @_ZN1A3fooEv("
+
----------------
Please merge the two tests, using FileCheck for both of them. I think you can replace the grep with a CHECK: here.


http://reviews.llvm.org/D11918