[PATCH] D68811: [CVP] Remove a masking operation if range information implies it's a noop

[Philip Reames via Phabricator via llvm-commits]

reames marked an inline comment as done.
reames added inline comments.


================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:715-716
+
+  ConstantRange LRange = LVI->getConstantRange(LHS, BB, BinOp);
+  if (!LRange.getUnsignedMax().ule(RHS->getValue()))
+    return false;
----------------
reames wrote:
> lebedev.ri wrote:
> > Do we want to query constanrange, or use `getPredicateAt()`?
> > Different folds here take different routes.
> > Should this be:
> > ```
> >   if (LVI->getPredicateAt(ICmpInst::ICMP_ULE, LHS, RHS, SDI) !=
> >       LazyValueInfo::True)
> >     return false;
> > ```
> > ?  (i don't know)
> In this case, I think they're basically equivalent in practice for this case.  The getPredicateAt form is slightly more powerful as it does predicate pushback through one layer of predecessors, but skipping that is slightly faster compile time wise.
> 
> Hm, having written that, I think I'll switch to the predicate form just for future proofing ad consistency.  Update coming.  
Correction: the getPredicateAt form does predicate pushback, but is weaker on conditions which are implied by edge local facts.  (i.e. I tried to switch and the results are strictly worse.)  This is arguably an LVI bug - one I remember from past occurrences now - but not something I'm going to fix just for this issue.  


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

https://reviews.llvm.org/D68811