[SelectionDAG] Prevent combination on inconsistent type in combineCarryDiamond
#84888
Conversation
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: XChy (XChy) ChangesFixes #84831 I'm new to DAG, any suggestion is appreciated. Full diff: https://github.com/llvm/llvm-project/pull/84888.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index cdcb7114640471..7e8b1d82b22504 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -3442,6 +3442,9 @@ static SDValue combineCarryDiamond(SelectionDAG &DAG, const TargetLowering &TLI,
return SDValue();
if (Opcode != ISD::UADDO && Opcode != ISD::USUBO)
return SDValue();
+ // Guarantee identical type of CarryOut
+ if (N->getValueType(0) != Carry0.getValue(1).getValueType())
+ return SDValue();
// Canonicalize the add/sub of A and B (the top node in the above ASCII art)
// as Carry0 and the add/sub of the carry in as Carry1 (the middle node).
diff --git a/llvm/test/CodeGen/X86/addcarry.ll b/llvm/test/CodeGen/X86/addcarry.ll
index 3fc4ed99fad0fa..f0efd6fe5de370 100644
--- a/llvm/test/CodeGen/X86/addcarry.ll
+++ b/llvm/test/CodeGen/X86/addcarry.ll
@@ -1490,3 +1490,26 @@ define { i64, i64 } @addcarry_commutative_2(i64 %x0, i64 %x1, i64 %y0, i64 %y1)
%r1 = insertvalue { i64, i64 } %r0, i64 %b1s, 1
ret { i64, i64 } %r1
}
+
+define i1 @pr84831(i64 %0) {
+; CHECK-LABEL: pr84831:
+; CHECK: # %bb.0:
+; CHECK-NEXT: testq %rdi, %rdi
+; CHECK-NEXT: setne %al
+; CHECK-NEXT: xorl %ecx, %ecx
+; CHECK-NEXT: addb $-1, %al
+; CHECK-NEXT: adcq $1, %rcx
+; CHECK-NEXT: setb %al
+; CHECK-NEXT: retq
+ %2 = icmp ult i64 0, %0
+ %3 = add i64 0, 1
+ %4 = icmp ult i64 %3, 0
+ %5 = zext i1 %2 to i64
+ %6 = add i64 %3, %5
+ %7 = icmp ult i64 %6, %3
+ %8 = zext i1 %4 to i63
+ %9 = zext i1 %7 to i63
+ %new0 = or i63 %8, %9
+ %last = trunc i63 %new0 to i1
+ ret i1 %last
+}
|
llvm/test/CodeGen/X86/addcarry.ll
Outdated
| ; CHECK-NEXT: setb %al | ||
| ; CHECK-NEXT: retq | ||
| %2 = icmp ult i64 0, %0 | ||
| %3 = add i64 0, 1 |
There was a problem hiding this comment.
Are these trivially foldable cases necessary to reproduce? If not they should be folded away
There was a problem hiding this comment.
Yes, I tried to reduce it manually, but every fold stops it crashing llc. I'll add the names related to semantics, so that it can be understood.
| @@ -3442,6 +3442,9 @@ static SDValue combineCarryDiamond(SelectionDAG &DAG, const TargetLowering &TLI, | |||
| return SDValue(); | |||
| if (Opcode != ISD::UADDO && Opcode != ISD::USUBO) | |||
| return SDValue(); | |||
| // Guarantee identical type of CarryOut | |||
| if (N->getValueType(0) != Carry0.getValue(1).getValueType()) | |||
There was a problem hiding this comment.
Most of this code implicitly assumes we're returning == MVT::i1, so maybe:
if (N->getValueType(0) != MVT::i1 ||
Carry0.getValue(1).getValueType() != MVT::i1 ||
Carry1.getValue(1).getValueType() != MVT::i1)
return SDValue();There was a problem hiding this comment.
It can be i8 in X86 too. If we only handle i1, we cannot combine addcarry_fake_carry in test/X86/CodeGen/addcarry.ll.
I missed the check for Carry1, and it seems that we should replace MVT::i1 for and operator.
There was a problem hiding this comment.
I don't see how this is implicitly assuming i1? I can see implicitly assuming the uaddo output and input bools are the same
XChy
force-pushed
the
dagcombine-fix-carry-diamond
branch
from
168f43e
to
050e5ca
Compare
|
@RKSimon, any comment on this patch? |
|
@RKSimon gently ping. |
…rryDiamond` (llvm#84888) Fixes llvm#84831 When matching carry pattern with `getAsCarry`, it may produce different type of carryout. This patch checks such case and does early exit. I'm new to DAG, any suggestion is appreciated.
|
/cherry-pick cb4453d |
Error: Command failed due to missing milestone. |
…rryDiamond` (llvm#84888) Fixes llvm#84831 When matching carry pattern with `getAsCarry`, it may produce different type of carryout. This patch checks such case and does early exit. I'm new to DAG, any suggestion is appreciated. (cherry picked from commit cb4453d)
…rryDiamond` (llvm#84888) Fixes llvm#84831 When matching carry pattern with `getAsCarry`, it may produce different type of carryout. This patch checks such case and does early exit. I'm new to DAG, any suggestion is appreciated. (cherry picked from commit cb4453d)
Fixes #84831
When matching carry pattern with
getAsCarry, it may produce different type of carryout. This patch checks such case and does early exit.I'm new to DAG, any suggestion is appreciated.