Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[InstCombine] Fix assertion failure in issue80597 #80614

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Feb 4, 2024

The assertion in #80597 failed when we were trying to compute known bits of a value in an unreachable BB.

case Instruction::AShr: {
unsigned SignBits = ComputeNumSignBits(I->getOperand(0), Depth + 1, CxtI);
// If we only want bits that already match the signbit then we don't need
// to shift.
unsigned NumHiDemandedBits = BitWidth - DemandedMask.countr_zero();
if (SignBits >= NumHiDemandedBits)
return I->getOperand(0);
// If this is an arithmetic shift right and only the low-bit is set, we can
// always convert this into a logical shr, even if the shift amount is
// variable. The low bit of the shift cannot be an input sign bit unless
// the shift amount is >= the size of the datatype, which is undefined.
if (DemandedMask.isOne()) {
// Perform the logical shift right.
Instruction *NewVal = BinaryOperator::CreateLShr(
I->getOperand(0), I->getOperand(1), I->getName());
return InsertNewInstWith(NewVal, I->getIterator());
}
const APInt *SA;
if (match(I->getOperand(1), m_APInt(SA))) {
uint32_t ShiftAmt = SA->getLimitedValue(BitWidth-1);
// Signed shift right.
APInt DemandedMaskIn(DemandedMask.shl(ShiftAmt));
// If any of the high bits are demanded, we should set the sign bit as
// demanded.
if (DemandedMask.countl_zero() <= ShiftAmt)
DemandedMaskIn.setSignBit();
if (SimplifyDemandedBits(I, 0, DemandedMaskIn, Known, Depth + 1)) {
// exact flag may not longer hold.
I->dropPoisonGeneratingFlags();
return I;
}
assert(!Known.hasConflict() && "Bits known to be one AND zero?");
// Compute the new bits that are at the top now plus sign bits.
APInt HighBits(APInt::getHighBitsSet(
BitWidth, std::min(SignBits + ShiftAmt - 1, BitWidth)));
Known.Zero.lshrInPlace(ShiftAmt);
Known.One.lshrInPlace(ShiftAmt);
// If the input sign bit is known to be zero, or if none of the top bits
// are demanded, turn this into an unsigned shift right.
assert(BitWidth > ShiftAmt && "Shift amount not saturated?");
if (Known.Zero[BitWidth-ShiftAmt-1] ||
!DemandedMask.intersects(HighBits)) {
BinaryOperator *LShr = BinaryOperator::CreateLShr(I->getOperand(0),
I->getOperand(1));
LShr->setIsExact(cast<BinaryOperator>(I)->isExact());
LShr->takeName(I);
return InsertNewInstWith(LShr, I->getIterator());
} else if (Known.One[BitWidth-ShiftAmt-1]) { // New bits are known one.
Known.One |= HighBits;
}
} else {
computeKnownBits(I, Known, Depth, CxtI);
}
break;
}

In this case, SignBits is 30 (deduced from instr info), but Known is 10000101010111010011110101000?0?00000000000000000000000000000000 (deduced from dom cond). Setting high bits of lshr Known, 1 will lead to conflict.

This patch masks out high bits of Known.Zero to address this problem.

Fixes #80597.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

The assertion in #80597 failed when we were trying to compute known bits of a value in an unreachable BB.

case Instruction::AShr: {
unsigned SignBits = ComputeNumSignBits(I->getOperand(0), Depth + 1, CxtI);
// If we only want bits that already match the signbit then we don't need
// to shift.
unsigned NumHiDemandedBits = BitWidth - DemandedMask.countr_zero();
if (SignBits >= NumHiDemandedBits)
return I->getOperand(0);
// If this is an arithmetic shift right and only the low-bit is set, we can
// always convert this into a logical shr, even if the shift amount is
// variable. The low bit of the shift cannot be an input sign bit unless
// the shift amount is >= the size of the datatype, which is undefined.
if (DemandedMask.isOne()) {
// Perform the logical shift right.
Instruction *NewVal = BinaryOperator::CreateLShr(
I->getOperand(0), I->getOperand(1), I->getName());
return InsertNewInstWith(NewVal, I->getIterator());
}
const APInt *SA;
if (match(I->getOperand(1), m_APInt(SA))) {
uint32_t ShiftAmt = SA->getLimitedValue(BitWidth-1);
// Signed shift right.
APInt DemandedMaskIn(DemandedMask.shl(ShiftAmt));
// If any of the high bits are demanded, we should set the sign bit as
// demanded.
if (DemandedMask.countl_zero() <= ShiftAmt)
DemandedMaskIn.setSignBit();
if (SimplifyDemandedBits(I, 0, DemandedMaskIn, Known, Depth + 1)) {
// exact flag may not longer hold.
I->dropPoisonGeneratingFlags();
return I;
}
assert(!Known.hasConflict() && "Bits known to be one AND zero?");
// Compute the new bits that are at the top now plus sign bits.
APInt HighBits(APInt::getHighBitsSet(
BitWidth, std::min(SignBits + ShiftAmt - 1, BitWidth)));
Known.Zero.lshrInPlace(ShiftAmt);
Known.One.lshrInPlace(ShiftAmt);
// If the input sign bit is known to be zero, or if none of the top bits
// are demanded, turn this into an unsigned shift right.
assert(BitWidth > ShiftAmt && "Shift amount not saturated?");
if (Known.Zero[BitWidth-ShiftAmt-1] ||
!DemandedMask.intersects(HighBits)) {
BinaryOperator *LShr = BinaryOperator::CreateLShr(I->getOperand(0),
I->getOperand(1));
LShr->setIsExact(cast<BinaryOperator>(I)->isExact());
LShr->takeName(I);
return InsertNewInstWith(LShr, I->getIterator());
} else if (Known.One[BitWidth-ShiftAmt-1]) { // New bits are known one.
Known.One |= HighBits;
}
} else {
computeKnownBits(I, Known, Depth, CxtI);
}
break;
}

In this case, SignBits is 30 (deduced from instr info), but Known is 10000101010111010011110101000?0?00000000000000000000000000000000 (deduced from dom cond). Setting high bits of lshr Known, 1 will lead to conflict.

This patch syncs SignBits with Known.countMinSignBits() to address this problem.

Fixes #80597.


Full diff: https://github.com/llvm/llvm-project/pull/80614.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp (+5)
  • (added) llvm/test/Transforms/InstCombine/pr80597.ll (+33)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
index a8a5f9831e15e..1f34469980585 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -784,6 +784,7 @@ Value *InstCombinerImpl::SimplifyDemandedUseBits(Value *V, APInt DemandedMask,
       }
 
       assert(!Known.hasConflict() && "Bits known to be one AND zero?");
+      unsigned MinSignBits = Known.countMinSignBits();
       // Compute the new bits that are at the top now plus sign bits.
       APInt HighBits(APInt::getHighBitsSet(
           BitWidth, std::min(SignBits + ShiftAmt - 1, BitWidth)));
@@ -801,6 +802,10 @@ Value *InstCombinerImpl::SimplifyDemandedUseBits(Value *V, APInt DemandedMask,
         LShr->takeName(I);
         return InsertNewInstWith(LShr, I->getIterator());
       } else if (Known.One[BitWidth-ShiftAmt-1]) { // New bits are known one.
+        // Sync SignBits with computeKnownBits to make sure there is no
+        // conflict.
+        HighBits = APInt::getHighBitsSet(
+            BitWidth, std::min(MinSignBits + ShiftAmt - 1, BitWidth));
         Known.One |= HighBits;
       }
     } else {
diff --git a/llvm/test/Transforms/InstCombine/pr80597.ll b/llvm/test/Transforms/InstCombine/pr80597.ll
new file mode 100644
index 0000000000000..148da056486f9
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr80597.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+define i64 @pr80597(i1 %cond) {
+; CHECK-LABEL: define i64 @pr80597(
+; CHECK-SAME: i1 [[COND:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[ADD:%.*]] = select i1 [[COND]], i64 0, i64 -12884901888
+; CHECK-NEXT:    [[SEXT1:%.*]] = add nsw i64 [[ADD]], 8836839514384105472
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ult i64 [[SEXT1]], -34359738368
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
+; CHECK:       if.else:
+; CHECK-NEXT:    [[SEXT2:%.*]] = ashr exact i64 [[ADD]], 1
+; CHECK-NEXT:    [[ASHR:%.*]] = or disjoint i64 [[SEXT2]], 4418419761487020032
+; CHECK-NEXT:    ret i64 [[ASHR]]
+; CHECK:       if.then:
+; CHECK-NEXT:    ret i64 0
+;
+entry:
+  %add = select i1 %cond, i64 0, i64 4294967293
+  %add8 = shl i64 %add, 32
+  %sext1 = add i64 %add8, 8836839514384105472
+  %cmp = icmp ult i64 %sext1, -34359738368
+  br i1 %cmp, label %if.then, label %if.else
+
+if.else:
+  %sext2 = or i64 %add8, 8836839522974040064
+  %ashr = ashr i64 %sext2, 1
+  ret i64 %ashr
+
+if.then:
+  ret i64 0
+}

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dtcxzyw dtcxzyw merged commit cb8d83a into llvm:main Feb 5, 2024
4 of 5 checks passed
@dtcxzyw dtcxzyw deleted the fix-ashr-conflict branch February 5, 2024 17:29
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 5, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 6, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
The assertion in llvm#80597 failed when we were trying to compute known bits
of a value in an unreachable BB.

https://github.com/llvm/llvm-project/blob/859b09da08c2a47026ba0a7d2f21b7dca705864d/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L749-L810

In this case, `SignBits` is 30 (deduced from instr info), but `Known` is
`10000101010111010011110101000?0?00000000000000000000000000000000`
(deduced from dom cond). Setting high bits of `lshr Known, 1` will lead
to conflict.

This patch masks out high bits of `Known.Zero` to address this problem.

Fixes llvm#80597.

(cherry picked from commit cb8d83a)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

llvm crash: Assertion `!LHSKnown.hasConflict() && "Bits known to be one AND zero?"' failed.
3 participants