[PATCH] D14271: [CVP] Fold return values if possible

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:52
@@ +51,3 @@
+    /// dominates.  If no such Constant can be found, return nullptr.
+    Constant* getConstantAt(Value *V, Instruction *At);
+
----------------
"Constant*" -> "Constant *"

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:198
@@ -192,1 +197,3 @@
 
+  // As a policy choice, we chose not waste compile time on anything where the
+  // comparison is testing local values.  While LVI can sometimes reason about
----------------
chose -> choose  (present tense)

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:200
@@ +199,3 @@
+  // comparison is testing local values.  While LVI can sometimes reason about
+  // such cases, it's not it's primary purposes.  We do make sure to do the
+  // block local query for uses from terminator instructions, but that's
----------------
"it's not it's primary purposes" -> "it's not its primary purpose". (it's -> its and purposes -> purpose)

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:333
@@ -318,1 +332,3 @@
 
+Constant* CorrelatedValuePropagation::getConstantAt(Value *V, Instruction *At) {
+  if (Constant *C = LVI->getConstant(V, At->getParent(), At))
----------------
Constant* -> Constant *

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:337
@@ +336,3 @@
+
+  // TODO: The following really should be sunk inside LVI's core algorith, or
+  // at least the outer shims around such.
----------------
algorith -> algorithm

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:339-340
@@ +338,4 @@
+  // at least the outer shims around such.
+  auto *C = dyn_cast<CmpInst>(V);
+  if (!C) return nullptr;
+
----------------
What about a single SelectInst?

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:351-354
@@ +350,6 @@
+  
+  if (Result == LazyValueInfo::True)
+    return ConstantInt::getTrue(C->getContext());
+  else
+    return ConstantInt::getFalse(C->getContext());
+}
----------------
Remove the else. See http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

(Yes I know you just copied it from code above.)

================
Comment at: test/Transforms/CorrelatedValuePropagation/select.ll:74
@@ -73,3 +73,3 @@
   ret i32 %sel
-; CHECK: ret i32 %[[sel]]
 }
----------------
Cool!


http://reviews.llvm.org/D14271