[PATCH] D147897: [ValueTracking] Use maximum shift count in `shl` when determining if `shl` can be zero.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2688
+          computeKnownBits(I->getOperand(1), DemandedElts, Depth, Q);
+      if (!Known.One.shl(KnownCnt.getMaxValue()).isZero())
+        return true;
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > Isn't this going to assert if the max value is wider than the bit width?
> > > Should probably just use `KnownBits::shl()` here.
> > > Should probably just use `KnownBits::shl()` here.
> > 
> > Will do, does using `KnownBits::shl` fix the issue of failing the assert if max value is too wide, or do I need to add a check for that? Adding a few tests for that case and not seeing any assertions.
> > Should probably just use `KnownBits::shl()` here.
> 
> Using `KnownBits::shl(Known, KnownCnt).isNonZero()` causes regressions. How about just adding a checking `KnownCnt.getMaxValue().ult(Known.getBitWidth())`?
> Should probably just use `KnownBits::shl()` here.




================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2688
+          computeKnownBits(I->getOperand(1), DemandedElts, Depth, Q);
+      if (!Known.One.shl(KnownCnt.getMaxValue()).isZero())
+        return true;
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > goldstein.w.n wrote:
> > > nikic wrote:
> > > > nikic wrote:
> > > > > Isn't this going to assert if the max value is wider than the bit width?
> > > > Should probably just use `KnownBits::shl()` here.
> > > > Should probably just use `KnownBits::shl()` here.
> > > 
> > > Will do, does using `KnownBits::shl` fix the issue of failing the assert if max value is too wide, or do I need to add a check for that? Adding a few tests for that case and not seeing any assertions.
> > > Should probably just use `KnownBits::shl()` here.
> > 
> > Using `KnownBits::shl(Known, KnownCnt).isNonZero()` causes regressions. How about just adding a checking `KnownCnt.getMaxValue().ult(Known.getBitWidth())`?
> > Should probably just use `KnownBits::shl()` here.
> 
> 
> > Should probably just use `KnownBits::shl()` here.
> 
> Using `KnownBits::shl(Known, KnownCnt).isNonZero()` causes regressions. How about just adding a checking `KnownCnt.getMaxValue().ult(Known.getBitWidth())`?

Yeah `KnownBits::shl` isn't ideal here b.c its computing something more precision than we need. We only need to know if the maximum shift will clear the result. We don't need the precision of where the bits end up. For example with the `1 << (cnt & 16)` case, `KnowBits::shl` cant set any bits for the output. But the current logic figures out that can't be zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147897