[PATCH] D141481: [SCEV] Canonicalize ext(min/max(x, y)) to min/max(ext(x), ext(y))

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested review of this revision.
mkazantsev added inline comments.


================
Comment at: llvm/test/Analysis/ScalarEvolution/max-backedge-taken-count-guard-info-rewrite-expressions.ll:47
 ; Test case from PR40961.
+; TODO: Loop %loop: constant max backedge-taken count is 3
 define i32 @rewrite_zext_min_max(i32 %N, ptr %arr) {
----------------
mkazantsev wrote:
> nikic wrote:
> > Why does this regress?
> Investigating.
I digged as deep as that inside of `howFarToZero`, there is code
```
    if (Exact != getCouldNotCompute()) {
      APInt MaxInt = getUnsignedRangeMax(applyLoopGuards(Exact, L));
      ConstantMax =
          getConstant(APIntOps::umin(MaxInt, getUnsignedRangeMax(Exact)));
    }
```
and here is the numbers are coming from. Good case:
```
Exact = ((-4 + (4 * ((zext i32 (16 umin %N) to i64) /u 4))<nuw><nsw>)<nsw> /u 4), ConstantMax = 3
```
Bad case:
```
Exact = ((-4 + (4 * ((16 umin (zext i32 %N to i64)) /u 4))<nuw><nsw>)<nsw> /u 4), ConstantMax = 4611686018427387903
```
I didn't go deep enough to understand what exactly was missing on range computation. Looks like that one of dozens of ad-hoc ways to compute it has failed. I could theoretically go deeper and try to understand this. I'm pretty sure this SCEV can be constructed with same result even now, just no test that would expose the problem.

Question is: do we care enough to be blocked on this? What the patch did makes perfect sense, it's just another heuristic missing somewhere. I can assign someone to take a look a bit later, unless this is a serious problem.

How much are you concerned by this?


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

https://reviews.llvm.org/D141481