[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 10 01:04:18 PST 2023
mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.
Mostly looks good, but I think I found a bug in `IsMinMaxSCEVWithConstant `. Also please rebase on top of your tests.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15031
+ RHS = MinMax->getOperand(1);
+ return isa<SCEVConstant>(RHS);
+ }
----------------
1. If I remember correctly, SCEV operands are always sorted by kind. It means that constants always go first. `RHS` can't be a constant because then `LHS` is also a constant and the whole thing should be folded away. So it should be trivially `false`, and I guess this code part isn't covered by tests if it wasn't caught.
2. Usually matcher methods aren't supposed to change operands if they return `false`. In your case it doesn't matter, but generally better to follow the standard practice and do all checks before modifying the operands.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15169
+ return true;
+ if (isa<SCEVMinMaxExpr>(Expr)) {
+ auto MinMax = cast<SCEVMinMaxExpr>(Expr);
----------------
`if (auto *MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) { ...`
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15083
+ auto IsMin =
+ isa<SCEVSMinExpr>(MinMaxExpr) || isa<SCEVUMinExpr>(MinMaxExpr);
+ auto DivisibleExpr =
----------------
alonkom wrote:
> mkazantsev wrote:
> > Do you really support `smin` here? Your code is only correct for unsigned values. It interprets all values as non-negative.
> I check if the operands are non-negative inside GetPreviousSCEV/GetNextSCEVD
Ah ok, then it makes sense. Can we assert on that here?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15191
+ case CmpInst::ICMP_SLT: {
+ auto ModifiedRHS = getMinusSCEV(RHS, getOne(RHS->getType()));
+ ModifiedRHS =
----------------
alonkom wrote:
> mkazantsev wrote:
> > How do you account for `RHS` being `SINT_MIN` and similar cases here?
> I wonder how this worked before my patch.
> if we have assume (N < SINT_MIN) then the following SCEV was generated:
> smin(N, SINT_MIN - 1) which would overflow.
>
>
Well, if there is a check somewhere that `RHS` is non-negative, then it makes sense. Otherwise it's possibly broken. :)
================
Comment at: llvm/test/Analysis/ScalarEvolution/trip-multiple-guard-info.ll:397
+define void @test_trip_multiple_4_upper_lower_bounds(i32 %num) {
+; CHECK-LABEL: 'test_trip_multiple_4_upper_lower_bounds'
----------------
alonkom wrote:
> mkazantsev wrote:
> > Please precommit tests as is, I want to see what exactly this patch changes.
> https://reviews.llvm.org/D143337
Rebase patch on top of it?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141850/new/
https://reviews.llvm.org/D141850
More information about the llvm-commits
mailing list