[PATCH] D151394: [LSR] Treat URem as uninteresting

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

In D151394#4377695 <https://reviews.llvm.org/D151394#4377695>, @nikic wrote:

> I think what I still don't really understand is why LSR thinks it is safe to replace the IV here when it isn't. It seems like marking the urem as interesting here may be non-profitable, but also shouldn't be incorrect. I think there has to be some bug on the LSR side to enable this replacement.
>
> Note that `urem` is not the only non-add instruction that can end up producing an add SCEV node, so I'm not sure why we need to treat urem in particular specially.

This truly seems to be like one work around instead of fixing the problem for now. I tested some internal benchmarks and there is no performance regression. I tried to analyze this case in LSR, but everywhere looks like reasonable. I tested the test case with typed pointer, and the bug exists in LLVM 12. I may need more time to dig when and which design missed this scenario in LSR.

`urem` is special since the i32-i64 conversion may cause the value difference and the variable is negative since it converts the signed to unsigned. Another one is `udiv`, but there is SCEV Div Expr.

  signed: (i32 -1) == (i64 -1)
  unsigned: (i32 -1) != (i64 -1)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151394