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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Sean and Davide!


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:180
@@ +179,3 @@
+void ConstantHoistingPass::collectConstantCandidates(
+    ConstCandMapType &ConstCandMap, Instruction *Inst, unsigned Idx,
+    ConstantInt *ConstInt) {
----------------
davide wrote:
> hmm, is this clang-formatt'ed?
Yes, it is. Looks like the usual format goes out of the 80-char bounds, and I guess this is the fallback.

================
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) {
----------------
davide wrote:
> same (re clang-format), I could be wrong tho
This one is a bit weirder, since the usual formatting fits (exactly), but this is what clang-format outputs. Possibly a clang-format bug...

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:500
@@ +499,3 @@
+                                   DominatorTree &RunDT, BasicBlock &RunEntry) {
+  TTI = &RunTTI;
+  DT = &RunDT;
----------------
davide wrote:
> 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.
I don't really care either way, and we have both in the codebase. Since you felt strongly enough about it to comment, I don't mind changing. :-)

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


http://reviews.llvm.org/D21945