[PATCH] D43876: [LoopUnroll] Peel off iterations if it makes conditions true/false.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

LGTM. I would rather prefer removing the restriction on RightSCEV being AR in this patch and then making a follow-up if you need some extra logic for it, but it's up to you.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:182
+        continue;
+    } else if (isa<SCEVAddRecExpr>(RightSCEV))
+      continue;
----------------
fhahn wrote:
> mkazantsev wrote:
> > Why do we bail if both left and right are AddRecs? What is the problem?
> > 
> > If there are no conceptual troubles with handling this case, I'd rather handle it. And if so, please add a corresponding test.
> I don't think there are conceptual problems, although it will make things slightly more complicated; I think we would need some more checks and also evaluate the second AR, if we have one. I have  test case, but I would prefer to add that in a follow up patch, if that is ok with you?
We could just remove this "continue" check without adding extra logic for right AR. I mean, we only care that LeftSCEV is an AR and we don't care what RightSCEV actually is. For example, in a loop:

  int i = 1, j = 1;
  while (true) {
    i++;
    j = (j /s (i + 1));
  }

SCEV of `i`'s Phi will be a SCEVAddRec and SCEV of `j`'s Phi will be SCEVUnknown, and for some reason you allow your optimization when `RightSCEV = j` and explicitly prohibit it when `RightSCEV = i`. I wonder how `i` is more complicated than `j`? :)

I'm OK if you remove this check in a follow-up patch or right in this one, whatever you like more.


https://reviews.llvm.org/D43876