[PATCH] D144050: [SCEV] Strengthen nowrap flags via ranges for ARs on construction.

[Florian Hahn via Phabricator via llvm-commits]

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