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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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:
> 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.  
Right, this is a longstanding LVI limitation.


================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:713
+  ConstantInt *RHS = dyn_cast<ConstantInt>(BinOp->getOperand(1));
+  if (!RHS || !RHS->getValue().isMask())
+    return false;
----------------
The limit to masks seems a bit stronger than strictly necessary: The range needs to be <= than the trailing ones in RHS. That is for `0b11001111`, if the range is `<= 0b00001111` that is sufficient. Not sure if this is worth handling.


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

https://reviews.llvm.org/D68811