[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);
+    }
----------------
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.


https://reviews.llvm.org/D39082