[PATCH] D144050: [SCEV] Strengthen nowrap flags via ranges for ARs on construction.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 6 08:59:58 PST 2023
fhahn marked 3 inline comments as done.
fhahn added a comment.
What are the current thoughts on the next steps to move forward with this. As for the 2 regressions, I think the one in `llvm/test/Analysis/ScalarEvolution/incorrect-exit-count.ll` won't be easily resolve-able. The other one can be solved by a follow-up (D144753 <https://reviews.llvm.org/D144753>), but it needs a bit more work because currently it triggers an infinite loop in a test case.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5739
+ (SCEV::NoWrapFlags)(AR->getNoWrapFlags() |
+ proveNoWrapViaConstantRanges(AR)));
+ }
----------------
mkazantsev wrote:
> fhahn wrote:
> > mkazantsev wrote:
> > > Should we update `Flags` here to have more optimistic flags for `BEInst`?
> > I tried to do that in D144197, but it seems the only change is a single regression; I've not looked into it in detail so far though.
> It's not exactly what I mean, I mean smth like
> ```
> if (auto *AR = dyn_cast<SCEVAddRecExpr>(PHISCEV)) {
> Flags = Flags | proveNoWrapViaConstantRanges(AR);
> setNoWrapFlags(const_cast<SCEVAddRecExpr *>(AR), Flags);
> }
> // further on, updated Flags is used, see line 5749.
> ```
Ah I see, thanks! I put up D145395 as follow-up. Although I am not sure if it is correct for all cases, as the strengthening with ranges may be independent of BEValue overflowing.
================
Comment at: llvm/test/Analysis/ScalarEvolution/incorrect-exit-count.ll:20
; CHECK-NEXT: %storemerge1921 = phi i32 [ 3, %outer.loop ], [ %dec, %for.end ]
-; CHECK-NEXT: --> {3,+,-1}<nuw><nsw><%for.cond6> U: [3,4) S: [3,4) Exits: <<Unknown>> LoopDispositions: { %for.cond6: Computable, %outer.loop: Variant }
+; CHECK-NEXT: --> {3,+,-1}<nsw><%for.cond6> U: [3,4) S: [3,4) Exits: <<Unknown>> LoopDispositions: { %for.cond6: Computable, %outer.loop: Variant }
; CHECK-NEXT: %idxprom20 = zext i32 %storemerge1921 to i64
----------------
fhahn wrote:
> nikic wrote:
> > Regression
> I still need to investigate this one.
Ok, here's what's going on.
Without this patch, this is strengthened by this code here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ScalarEvolution.cpp#L1651
With the new changes, this code now gets called earlier via the call to `proveNoWrapViaConstantRanges`, which in turn computes the backedge taken-count, which in turn calls `getSCEVAtScope`, which in turn creates the SCEV for `%idxprom20`. Because `getSCEVAtScope` constructs the SCEV for `%idxprom20` while computing the backedge taken count, the backedge taken count will be `SCEVCouldNotCompute` at L1651, hence the flags don't get strengthened.
I don't know if there's a way to avoid this issue, but it seems a small price to pay in this case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144050/new/
https://reviews.llvm.org/D144050
More information about the llvm-commits
mailing list