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

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1769
@@ +1768,3 @@
+bool GVN::processAssumeIntrinsic(IntrinsicInst *IntrinsicI) {
+  assert(IntrinsicI->getIntrinsicID() == Intrinsic::assume);
+  Value *V = IntrinsicI->getArgOperand(0);
----------------
Please add a string to this: assert(expression && "Failure message"); See http://llvm.org/docs/CodingStandards.html#assert-liberally

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1771
@@ +1770,3 @@
+  Value *V = IntrinsicI->getArgOperand(0);
+  Constant *True = ConstantInt::getTrue(getGlobalContext());
+  bool Changed = false;
----------------
Wrong context. Use the same context from any Type or Value you have.

Let me explain what llvm contexts are for. They're intended to keep different threads separate, and keep them from accessing each others data, since llvm does not have internal locking. So the context owns the types and constant, and if two threads try to create "i1 true" at the same time then they'll race on updating the map. (Also, it's a verifier failure to have two Type*'s form two contexts involved in the same Module. By default everyone just uses the global context, but there may be a library user that does not.)

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1773
@@ +1772,3 @@
+  bool Changed = false;
+  for (BasicBlock *successor : successors(IntrinsicI->getParent())) {
+    BasicBlockEdge Edge(IntrinsicI->getParent(), successor);
----------------
"Successor". See http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1776-1777
@@ +1775,4 @@
+
+    // Equality propagation can't be done for every
+    // successor, but propagateEquality checks it.
+    Changed |= propagateEquality(V, True, Edge);
----------------
Reflow this comment.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2071-2072
@@ -2033,1 +2070,4 @@
 
+// Tries to replace instruction with const,
+// using information from ReplaceWithConstMap.
+bool GVN::replaceOperandsWithConsts(Instruction *Instr) const {
----------------
Reflow this comment.

================
Comment at: test/Transforms/GVN/assume-ptr-equal.ll:6-22
@@ +5,19 @@
+
+; Checking const propagation across other BBs
+; CHECK-LABEL: define void @_Z1gb(
+; CHECK: call i32 @_ZN1A3fooEv(
+; CHECK: call i32 @_ZN1A3barEv(
+
+; Checking const propagation in the same BB
+; CHECK-LABEL: define i32 @main()
+; CHECK: call i32 @_ZN1A3fooEv(
+
+; CHECK-LABLE: define float @_Z1gf(float %p)
+; CHECK: ret float 3.000000e+00
+
+; CHECK-LABEL: define float @_Z1hf(float %p)
+; CHECK: ret float 3.000000e+00
+
+; CHECK-LABEL: define float @_Z1if(float %p)
+; CHECK-NOT: ret float 3.000000e+00
+
----------------
Please interleave the check statements with the function being tested. This helps show which calls should I expect to become direct calls to foo and bar.

================
Comment at: test/Transforms/GVN/assume-ptr-equal.ll:15
@@ +14,3 @@
+
+; CHECK-LABLE: define float @_Z1gf(float %p)
+; CHECK: ret float 3.000000e+00
----------------
Typo, CHECK-LABLE


http://reviews.llvm.org/D11918