[PATCH] D67408: [IndVars] An implementation of loop predication without a need for speculation

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

> I haven't seen anything in the literature which quite matches this. Given that, I'm not entirely sure that keeping the name "loop predication" is helpful. Anyone have suggestions for a better name? This is analogous to partial redundancy elimination - since we remove the condition flowing around the backedge - and has some parallels to our existing transforms which try to make conditions invariant in loops.

Maybe this is because I don't have context on previous work related to LoopPredication but it  sounds not quite what the transformation is doing. At least it doesn't generate any explicit predicate for the loop. I don't have anything better to suggest at this time though.



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2750
+  // form.  If we have a read-only loop, and we can tell that we must exit down
+  // a path which does not need any of the values computed within the loop, we
+  // can rewrite the loop to exit on the first iteration.  Note that this
----------------
I can't find a place where this prerequisite is actually checked. Am I just missing it?


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2758
+  if (!LoopPredication)
+    return Changed;
+
----------------
Returning at this point means no other code (other than loop predication) can be added bellow this point in future. I think we better structure it in a more independent way. Taking into account this function becomes pretty large I would suggest factoring out new code to a separate function?


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2769
+
+  auto Filter = [&](BasicBlock *ExitingBB) {
+    // If our exitting block exits multiple loops, we can only rewrite the
----------------
Looks like many steps in this filter are common with the existing code above. I think it make sense to unify these two pieces. Ideally ExitingBB better be const to prevent unintended influence but not sure this is doable with out a copy.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2827
+      // TODO:isGuaranteedToTransfer
+      if (I.mayHaveSideEffects() || I.mayThrow())
+        return Changed;
----------------
No need to call mayThrow explicitly since mayHaveSideEffects covers this.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2835
+  //    it for dedicated passes.
+  // 2) We insert the comparison at the branch.  Hoisting introduces additional
+  //    legality constraints and we leave that to dedicated logic.  We want to
----------------
The first sentence sounds a little odd to me. Would you mind rephrasing it. 


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2837
+  //    legality constraints and we leave that to dedicated logic.  We want to
+  //    predicate even if we can't insert a loop invariant expression as
+  //    peeling or unrolling will likely reduce the cost of the otherwise loop
----------------
Could you clarify this sentence for me. Did not catch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67408/new/

https://reviews.llvm.org/D67408