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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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:
> 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?
Maybe it's related to the fact that applyLoopGuards rewriting supports zext but not umin? https://github.com/llvm/llvm-project/blob/c05ace1067bd21abf504d75f1efb5cf0e1c3fb51/llvm/lib/Analysis/ScalarEvolution.cpp#L14956

> How much are you concerned by this?

Based on the comment above, it looks like this is a motivating case for the applyLoopGuards() logic, not just artificial test coverage, so it would be nice to not regress it.


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

https://reviews.llvm.org/D141481