[PATCH] D144861: [SCEV][IndVars][WIP] Check outer loop invariant when cononicalize comparision

Wed Mar 1 00:10:22 PST 2023

nikic added a comment.

In D144861#4155833 <https://reviews.llvm.org/D144861#4155833>, @wsmoses wrote:

> If it’s possible to not revert that would be preferable as @jdoefert @vchuravy I and others have many applications that would have significant slow down if reverted.

The change also caused significant slowdowns for multiple other people, which were never addressed (e.g. https://github.com/llvm/llvm-project/issues/54191). Combined with this issue, that's enough for a revert for me.

In D144861#4160432 <https://reviews.llvm.org/D144861#4160432>, @StephenFan wrote:

> In D144861#4160179 <https://reviews.llvm.org/D144861#4160179>, @efriedma wrote:
>
>> From my analysis of the testcase, we called "getAddExpr(SCEV::FlagNUW)" while analyzing the first loop, and that broke the analysis of the second loop.  The actual simplification isn't relevant.
>
> Sorry, I don't know what you mean by "The actual simplification isn't relevant". Would you mind explaining a little more?

The getAddExpr() with NUW flag sets this flag globally, for all uses of this add. The icmp simplification itself doesn't make a difference, just the side effect that the (not globally correct) nuw flag is set.

If we don't want to revert the patch, the right fix would be to not set the NUW flag in the ControllingFiniteLoop case.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144861