[PATCH] D30311: [ValueTracking] Don't do an unchecked shift in ComputeNumSignBits

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 24 06:42:05 PST 2017


spatel added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:2209
     if (match(U->getOperand(1), m_APInt(ShAmt))) {
-      Tmp += ShAmt->getZExtValue();
+      Tmp += ShAmt->getLimitedValue(TyBits);
       if (Tmp > TyBits) Tmp = TyBits;
----------------
sanjoy wrote:
> majnemer wrote:
> > sanjoy wrote:
> > > majnemer wrote:
> > > > Shouldn't we treat this in the same way we treat Instruction::Shl?
> > > You mean return `1` for undefined shifts?  I don't see a good reason why we should do that (both here and in `shl`) -- `ashr` ing more than the bitwidth is `undef` anyway, so why not be maximally aggressive?
> > It is undef but I don't think ComputeNumSignBits it allowed to assign known bits to undef values.
> I see what you mean.  I'm a bit embarrassed now that it took me this long. :)
Since it's non-obvious how undef is handled, how about adding a comment to explain why we don't saturate to TyBits? This could be a function-level comment because this can occur with more than just Ashr (and Shl?).


================
Comment at: test/Transforms/IndVarSimplify/pr32045.ll:7
+define i32 @foo(i32 %a, i32 %b, i32 %c) {
+; CHECK-LABEL: @foo(
+entry:
----------------
I think IndVars does make a transformation on this test, so it would be better to check the correct output rather than just checking that no assert occurs. The script at utils/update_test_checks.py can be used with loops now. :)


https://reviews.llvm.org/D30311





More information about the llvm-commits mailing list