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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks again for having a look. I'll drop the bail out and add a test before  I commit the change.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:182
+        continue;
+    } else if (isa<SCEVAddRecExpr>(RightSCEV))
+      continue;
----------------
mkazantsev wrote:
> 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.
Ah yes. I removed the check now, as it won't anything wrong. What I meant is that I think we could do better than just dropping the bail out. That's what I plan to do in a follow up patch.


https://reviews.llvm.org/D43876