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

[Anna Thomas via Phabricator via llvm-commits]

anna accepted this revision.
anna added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:185
+      else
+        return SE.isKnownPredicate(ICmpInst::ICMP_UGE, Begin, End);
+    }
----------------
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?


https://reviews.llvm.org/D39082