[PATCH] D89821: [SCEV] Match 'zext (trunc A to iB) to iY' as URem.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D89821#2342783 <https://reviews.llvm.org/D89821#2342783>, @lebedev.ri wrote:

> I would expect that you should be able to showcase the effect with `opt -analyze -scalar-evolution`-based test:
> https://godbolt.org/z/xddz8d (because there's `zext(A % B) --> zext(A) % zext(B)` `matchURem()`-driven fold)

Not sure if that's possible, because the expression is already in the form `zext (trunc ... to iX) to iY` and I think if there was an outer zext, it would be already folded into the inner zext.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12757
 // 4, A / B becomes X / 8).
 bool ScalarEvolution::matchURem(const SCEV *Expr, const SCEV *&LHS,
                                 const SCEV *&RHS) {
----------------
lebedev.ri wrote:
> What if `Expr` isn't an `zext` itself, but it is used by zext (which is the case in the single user of this method)?
Hm, I not sure what to do about that case. I think we need to zext (trunc) combo to match that.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12760-12761
+  // Try to match 'zext (trunc A to iB) to iY', which is used
+  // for URem with constant power-of-2 second operands. Make sure the size of
+  // the operand A matches the size of the whole expressions.
+  if (const auto *ZExt = dyn_cast<SCEVZeroExtendExpr>(Expr)) {
----------------
lebedev.ri wrote:
> What if we had `zext(zext(trunc))`, so we end up with a wider type than A was?
> 
Originally the code had a check to ensure the type of the starting expression matched the type of A, but I now updated things to do a ZExt on demand to make A match the type of Expr.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12768
+      RHS = getConstant(Expr->getType(),
+                        1u << getTypeSizeInBits(Trunc->getType()));
+      return true;
----------------
mkazantsev wrote:
> `1u << X` will overflowan become zero if `X > 32`. Consider using APInt.
Thanks, updated to use APInt and added a test where the LHS is > 1 << 32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89821