[PATCH] D9151: Loop Versioning for LICM

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:120
@@ +119,3 @@
+static cl::opt<bool> UseLoopVersioningLICM(
+    "loop-versioning-licm", cl::init(false), cl::Hidden,
+    cl::desc("Enable the experimental Loop Versioning LICM pass"));
----------------
"loop-versioning-licm" -> "enable-loop-versioning-licm"

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:303
@@ +302,3 @@
+  if (!CurLoop->hasDedicatedExits()) {
+    DEBUG(dbgs() << "    loop does not has dedicated exit blocks\n");
+    return false;
----------------
Okay, please explain this better in the comment. Something like:

  // We need to be able to compute the loop trip count in order to generate the bound checks.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:387
@@ +386,3 @@
+    return false;
+  }
+  return true;
----------------
Why do you care how many pointers there are? I'd think only the number of checks generated matters in terms of cost.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:633
@@ +632,3 @@
+INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(LoopSimplify)
+INITIALIZE_PASS_DEPENDENCY(ScalarEvolutionWrapperPass)
----------------
No, please rename the other option (as I noted by the other option, putting adding enable- as a prefix would be natural). Then make this change.


Repository:
  rL LLVM

http://reviews.llvm.org/D9151