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.

Sign up for GitHub

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

Jump to bottom

[SelectionDAG] Prevent combination on inconsistent type in combineCarryDiamond #84888

Merged
merged 1 commit into from
Mar 22, 2024
Merged

[SelectionDAG] Prevent combination on inconsistent type in combineCarryDiamond #84888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3442,6 +3442,11 @@ static SDValue combineCarryDiamond(SelectionDAG &DAG, const TargetLowering &TLI,
return SDValue();
if (Opcode != ISD::UADDO && Opcode != ISD::USUBO)
return SDValue();
// Guarantee identical type of CarryOut
EVT CarryOutType = N->getValueType(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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();

Copy link
Member Author

@XChy XChy Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this is implicitly assuming i1? I can see implicitly assuming the uaddo output and input bools are the same

if (CarryOutType != Carry0.getValue(1).getValueType() ||
CarryOutType != Carry1.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).
Expand Down Expand Up @@ -3489,7 +3494,7 @@ static SDValue combineCarryDiamond(SelectionDAG &DAG, const TargetLowering &TLI,
// TODO: match other operations that can merge flags (ADD, etc)
DAG.ReplaceAllUsesOfValueWith(Carry1.getValue(0), Merged.getValue(0));
if (N->getOpcode() == ISD::AND)
return DAG.getConstant(0, DL, MVT::i1);
return DAG.getConstant(0, DL, CarryOutType);
return Merged.getValue(1);
}

Expand Down
23 changes: 23 additions & 0 deletions llvm/test/CodeGen/X86/addcarry.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 %arg) {
; 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
%a = icmp ult i64 0, %arg
%add1 = add i64 0, 1
%carryout1 = icmp ult i64 %add1, 0
%b = zext i1 %a to i64
%add2 = add i64 %add1, %b
%carryout2 = icmp ult i64 %add2, %add1
%zc1 = zext i1 %carryout1 to i63
%zc2 = zext i1 %carryout2 to i63
%or = or i63 %zc1, %zc2
%trunc = trunc i63 %or to i1
ret i1 %trunc
}
Loading