Annotate binary operators primarily in the
'SimplifyIndvar::simplifyUsers' to give more context for analyzes and
transformations with users, which are visited before binary operators.
Details
- Reviewers
mkazantsev apilipenko nikic fhahn lebedev.ri
Diff Detail
Event Timeline
llvm/lib/Analysis/ScalarEvolution.cpp | ||
---|---|---|
11118 | Why not isKnownPredicateAt(SignFlippedPred, LHS, RHS, CtxI)? It may also take assumes in current block into consideration. | |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
756 | How is that related to what the patch claims to do? Should it be a separate patch with separate tests? |
llvm/lib/Analysis/ScalarEvolution.cpp | ||
---|---|---|
11115 | Sure this is wrong. start = SINT_MAX - 1 step = 2 RHS = SINT_MAX On first iteration, both checks pass. SINT_MAX - 1 <s SINT_MAX // true SINT_MAX - 1 <u SINT_MAX // true On the 2nd iteration unsigned check will fail. Note that what you did is just duplicated the checks above, but dropped the nuw check that was there for reason. |
llvm/lib/Analysis/ScalarEvolution.cpp | ||
---|---|---|
11115 | Sorry, nsw check. |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
756 | Now there are 2 places where we are doing BO strengthen: before and inside the loop, so I though it would be better to move this code to a separate method to reduce repeating |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
936 | Factoring out this check into strengthenBinaryOp can be split off as a NFC patch. Then, make a functional patch that introduces its new use. |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
936 | Thanks, will do that |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
936 |
Update the patch after split-off & update commit message according to what it's doing now?
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
894 | This solves problem for users of initial IV, but seems that it still exists for users of users. Can we do it for all values? |
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp | ||
---|---|---|
894 | I'll add test for the case with users of users, thanks |
IIRC strengthenBinaryOp() is expensive, it would be great not to call it twice. (It works on double-sized SCEV expressions, so usually 128 bits, and thus takes all the slow paths).
It would be helpful to provide some more context on what the actual problem here is, i.e. in which order are we visiting instructions / performing folds in the example, and what would the right order be. I suspect it's not actually necessary to call it twice, but it's hard to say without understanding the root problem.
I'll try to explain the root probrem in the 'unknown.start' test case.
- Currently we process IV's users in this order:
%signed.cmp = icmp slt i32 %iv, %len %unsigned.cmp = icmp ult i32 %iv, %len %iv.next = add i32 %iv, 1
When we try to make %unsigned.cmp invariant, IV's SCEV doesn't have nsw/nuw flags:
{%start,+,1}<%loop>
Therefore AddRec's MonotonicType, computed in the ScalarEvolution::getLoopInvariantPredicate, is nullopt and we can't make %unsigned.cmp invariant.
- But if we visit bin op firstly and annotate it with nsw/nuw flags, they will be propagated to IV's SCEV:
{%start,+,1}<nuw><nsw><%loop>
And it allows us to make %unsigned.cmp invariant, since MonotonicType has value
Thanks for the explanation.
I suspect that what actually happens is that as part of the getZeroExtendExpr() call, we infer a nowrap flag on the addrec in https://github.com/llvm/llvm-project/blob/7a49d50f22ad577d91cda7904c8a162c2cecd4a8/llvm/lib/Analysis/ScalarEvolution.cpp#L1688, or one of the later bits in the same method. If this is the case, it would be nice to know which one is responsible here, and in particular whether it is the constant range one.
This is a long-standing issue, that nowrap flags are inferred when creating zext/sext expressions, so that creating those expressions can improve "unrelated" analysis results. I tried to address the constant range nowrap inference in D90338, but haven't pursued this in a while.
That reminded me of a patch I had lying around for a while to strengthen no wrap flags when construction AddRecs: D144050
I suspect that what actually happens is that as part of the getZeroExtendExpr() call, we infer a nowrap flag on the addrec in https://github.com/llvm/llvm-project/blob/7a49d50f22ad577d91cda7904c8a162c2cecd4a8/llvm/lib/Analysis/ScalarEvolution.cpp#L1688, or one of the later bits in the same method. If this is the case, it would be nice to know which one is responsible here, and in particular whether it is the constant range one.
We infer a nowrap flag on the addrec from the proveNoUnsignedWrapViaInduction(AR) call: https://github.com/llvm/llvm-project/blob/7a49d50f22ad577d91cda7904c8a162c2cecd4a8/llvm/lib/Analysis/ScalarEvolution.cpp#L1776
This seems to be the reason why patch https://reviews.llvm.org/D144050 from Florian didn't help in the case
I think this sould go, unless we have a better idea how to fix it. After all, it's just one more go of logic that is done in loop anyways. I'll give it couple of days to chill if someone has strong objections.
I still think this is the wrong way to go about it. If it's infeasible to ensure the flags are present in the first place, we should probably expose the methods to explicitly infer the flags on the addrec, without having to go through zext/sext to make this happen as a side effect.
Yeah, the question is, how much does it take and is it possible to do without running into a cyclic dependency...
Sure this is wrong.
On first iteration, both checks pass.
On the 2nd iteration unsigned check will fail.
SINT_MAX + 1 <s SINT_MAX true
SINT_MAX + 1 <u SINT_MAX false
Note that what you did is just duplicated the checks above, but dropped the nuw check that was there for reason.