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

[Philip Reames via Phabricator via llvm-commits]

reames marked 5 inline comments as done.
reames added inline comments.


================
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
----------------
ebrevnov wrote:
> I can't find a place where this prerequisite is actually checked. Am I just missing it?
The code is assuming lcssa, so the lack of phis in the exist block is what you're looking for.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2758
+  if (!LoopPredication)
+    return Changed;
+
----------------
ebrevnov wrote:
> 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?
I'm completely fine factoring this out into it's own function, but let's hold off until we decide on pass placement and algorithmic questions.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2769
+
+  auto Filter = [&](BasicBlock *ExitingBB) {
+    // If our exitting block exits multiple loops, we can only rewrite the
----------------
ebrevnov wrote:
> 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.
While there are commonalities, there are enough differences that combining the two is going to end up being extremely confusing.  I'd be willing to play around with code structure to see if this works better than I think it would, but I'd like to land and then refactor in that direction if it's okay.


================
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
----------------
ebrevnov wrote:
> The first sentence sounds a little odd to me. Would you mind rephrasing it. 
The comment is also stale.  I will simply remove 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
----------------
ebrevnov wrote:
> Could you clarify this sentence for me. Did not catch.
Again, stale.

I was originally inserting comparisons at the branch under the logic that the exit count might not be invariant.  I then realized that was non-sensical when I couldn't construct a test for it, and moved to the bail out scheme now in the code.    I forgot to remove the comment.


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

https://reviews.llvm.org/D67408