[PATCH] D21945: [PM] Port ConstantHoisting to the new Pass Manager

[Davide Italiano via llvm-commits]

davide added a comment.

The structure of this patch looks sensible. Some comments.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:180
@@ +179,3 @@
+void ConstantHoistingPass::collectConstantCandidates(
+    ConstCandMapType &ConstCandMap, Instruction *Inst, unsigned Idx,
+    ConstantInt *ConstInt) {
----------------
hmm, is this clang-formatt'ed?

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:196
@@ -195,3 +101,1 @@
 
-  setup(Fn);
-
----------------
Nice that you got rid of this weird `setup()` function.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:290
@@ -386,3 +289,3 @@
 /// constants with respect to the base constant.
-void ConstantHoisting::findAndMakeBaseConstant(ConstCandVecType::iterator S,
-                                               ConstCandVecType::iterator E) {
+void ConstantHoistingPass::findAndMakeBaseConstant(
+    ConstCandVecType::iterator S, ConstCandVecType::iterator E) {
----------------
same (re clang-format), I could be wrong tho

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:500
@@ +499,3 @@
+                                   DominatorTree &RunDT, BasicBlock &RunEntry) {
+  TTI = &RunTTI;
+  DT = &RunDT;
----------------
I prefer the idiom
`this->TTI = &TTI`
so that you don't have two different names for the variables, but, up to you I guess.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:534
@@ +533,3 @@
+      return PreservedAnalyses::all();
+    else {
+      // FIXME: This should also 'preserve the CFG'.
----------------
`else` after `return`


http://reviews.llvm.org/D21945