[PATCH] D102267: [SCEV] Apply guards to max with non-unitary steps.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:9269
+      assert(MaxInt.ule(getUnsignedRangeMax(Exact)) &&
+             "applying loop guards pessimized the found maximum");
+      Max = getConstant(MaxInt);
----------------
nikic wrote:
> Is there any particular reason to believe that this assertion can't be hit? The unit step case takes the minimum and has a test for this (`@guard_pessimizes_analysis`). Or is the assert here so we can find a test case?
> 
> On a related note, I wonder if we shouldn't be limiting the guard insertion to comparisons against constants. That case should be non-pessimizing, and at least none of the existing tests benefit from non-constant guards.
> Is there any particular reason to believe that this assertion can't be hit? The unit step case takes the minimum and has a test for this (@guard_pessimizes_analysis). Or is the assert here so we can find a test case?

I was overly optimistic. But I added a variant of `@guard_pessimizes_analysis` that would trigger the assert. Replaced that with a conditional assignment.

> On a related note, I wonder if we shouldn't be limiting the guard insertion to comparisons against constants. That case should be non-pessimizing, and at least none of the existing tests benefit from non-constant guards.

I think in theory rewriting with non-constants could also help in some cases, but probably not in practice at the moment.  I'm planning to look into that separately, possible to also help with pessimizing results after applying guards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102267