[PATCH] D39082: [IRCE] Smarter detection of empty ranges using SCEV

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:185
+      else
+        return SE.isKnownPredicate(ICmpInst::ICMP_UGE, Begin, End);
+    }
----------------
anna wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > anna wrote:
> > > > SCEV's isKnownPredicate is actually really good in most cases to identify the empty range (BEGIN >= END). 
> > > > However, there's a stronger version of isKnownPredicate in `DependenceInfo::isKnownPredicate`. 
> > > > 
> > > > Specifically, it first checks SCEV's version of isKnownPredicate and if it fails, it does a further analysis using the Delta between Begin and End and checking the Delta based on the predicate passed in. 
> > > > 
> > > > I'm wondering if SCEV's form maybe weaker and hence we may fire the assert added about empty ranges at line 1756?
> > > Assert on 1756 should never fire. If you look into `IntersectRange`, we check range on `isEmpty()` and return `None` if it is true. We make check before return and check in assert with the same `isEmpty()`, so we just cannot end up with one returning true and another returning false.
> > > 
> > > Thanks for pointing me out of `DependenceInfo::isKnownPredicate`, I will see if I can use it here.
> > I don't really like this idea after looking into its code. The reasons are:
> > 
> > 1) To use it, we need to factor this method out of dependency analysis ( e.g. into an utility function which takes `ScalarEvolution` as a parameter);
> > 2) It does not currently support unsigned comparisons failing with `llvm_unreachable("unexpected predicate in isKnownPredicate");`, and we do need them;
> > 3) If what it does with Delta is really profitable for symbolics, I don't see why it shouldn't be moved to `ScalarEvolution::isKnownPredicate`. This code doesn't need anything but ScalarEvolution.
> > 
> > So I would rather leave it as it is and consider this extra stuff from `DependenceInfo::isKnownPredicate` for factoring into SCEV in case if it is proved to make any sense.
> agreed. I was just curious about how the further Delta checks helps here (if any), and it looks like it won't help us?
In theory, they can help if we iterate from symbilic to symbolic; in practice, I think in most such cases we won't be able to prove anything if we don't have a known range for them, and if we know the range, we can prove `isKnownPredicate` basing on it.


https://reviews.llvm.org/D39082