Skip to content

Commit fb9c9eb

Browse files
authored
[X86][GISel] Fix carry-in for selectUAddSub. (llvm#199261)
When G_UADDE/G_USUBE was chained off a previous G_UADDE/G_UADDO/ G_USUBE/G_USUBO, selectUAddSub re-materialized EFLAGS.CF from the previous SETB byte using CMP r, 1. That computes (r - 1) and sets CF iff r < 1 unsigned, i.e. CF = (r == 0) -- the inverse of the desired carry. The following ADC/SBB then consumed the wrong CF and produced an off-by-one upper word; e.g. `add i128 0xFF..FF, 1` under -global-isel returned hi=0 lo=0 instead of hi=1 lo=0. Emit NEG r instead: NEG sets CF iff its operand is non-zero, matching the SETB byte. NEG is a two-address (tied) instruction, so emit it into a fresh virtual register rather than redefining the carry-in vreg. C reproducer (compile on x86_64-linux-gnu and run): ``` // clang -O2 -fglobal-isel repro.c -o repro && ./repro #include <stdio.h> __attribute__((noinline)) __int128 add128(__int128 a, __int128 b) { return a + b; } int main(void) { __int128 a = (__int128)0xFFFFFFFFFFFFFFFFULL; __int128 r = add128(a, 1); unsigned long long lo = r, hi = r >> 64; printf("hi=0x%llx lo=0x%llx\n", hi, lo); return (hi == 1 && lo == 0) ? 0 : 1; } ``` Without -fglobal-isel: prints "hi=0x1 lo=0x0", exits 0. With -fglobal-isel: prints "hi=0x0 lo=0x0", exits 1. The buggy code-gen emits the inverted-carry sequence (clang trunk, pre-fix): ``` add128: movq %rdx, %rax addq %rdi, %rax setb %dl cmpb $1, %dl # CF = (dl == 0), inverse of intended adcq %rsi, %rcx movq %rcx, %rdx retq ``` This bug was found by a large run of Opus 4.7 looking for bugs in LLVM.
1 parent c0c56f4 commit fb9c9eb

4 files changed

Lines changed: 18 additions & 17 deletions

File tree

llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,15 +1272,16 @@ bool X86InstructionSelector::selectUAddSub(MachineInstr &I,
12721272
Def->getOpcode() == TargetOpcode::G_UADDO ||
12731273
Def->getOpcode() == TargetOpcode::G_USUBE ||
12741274
Def->getOpcode() == TargetOpcode::G_USUBO) {
1275-
// carry set by prev ADD/SUB.
1276-
1277-
BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(X86::CMP8ri))
1278-
.addReg(CarryInReg)
1279-
.addImm(1);
1280-
1275+
// The carry-in is a SETB byte (0 or 1) from a chained add/sub.
1276+
// Materialize EFLAGS.CF from that byte for the following ADC/SBB
1277+
// by emitting NEG, which sets CF iff its operand is non-zero.
12811278
if (!RBI.constrainGenericRegister(CarryInReg, *CarryRC, MRI))
12821279
return false;
12831280

1281+
Register NegDef = MRI.createVirtualRegister(CarryRC);
1282+
BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(X86::NEG8r), NegDef)
1283+
.addReg(CarryInReg);
1284+
12841285
Opcode = IsSub ? OpSBB : OpADC;
12851286
} else if (auto val = getIConstantVRegVal(CarryInReg, MRI)) {
12861287
// carry is constant, support only 0.

llvm/test/CodeGen/X86/GlobalISel/add-scalar.ll

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ define i128 @test_add_i128(i128 %arg1, i128 %arg2) nounwind {
88
; X64-NEXT: movq %rdx, %rax
99
; X64-NEXT: addq %rdi, %rax
1010
; X64-NEXT: setb %dl
11-
; X64-NEXT: cmpb $1, %dl
11+
; X64-NEXT: negb %dl
1212
; X64-NEXT: adcq %rsi, %rcx
1313
; X64-NEXT: movq %rcx, %rdx
1414
; X64-NEXT: retq
@@ -25,13 +25,13 @@ define i128 @test_add_i128(i128 %arg1, i128 %arg2) nounwind {
2525
; X86-NEXT: movl {{[0-9]+}}(%esp), %edi
2626
; X86-NEXT: addl {{[0-9]+}}(%esp), %ecx
2727
; X86-NEXT: setb %bl
28-
; X86-NEXT: cmpb $1, %bl
28+
; X86-NEXT: negb %bl
2929
; X86-NEXT: adcl {{[0-9]+}}(%esp), %edx
3030
; X86-NEXT: setb %bl
31-
; X86-NEXT: cmpb $1, %bl
31+
; X86-NEXT: negb %bl
3232
; X86-NEXT: adcl {{[0-9]+}}(%esp), %esi
3333
; X86-NEXT: setb %bl
34-
; X86-NEXT: cmpb $1, %bl
34+
; X86-NEXT: negb %bl
3535
; X86-NEXT: adcl {{[0-9]+}}(%esp), %edi
3636
; X86-NEXT: movl %ecx, (%eax)
3737
; X86-NEXT: movl %edx, 4(%eax)
@@ -57,7 +57,7 @@ define i64 @test_add_i64(i64 %arg1, i64 %arg2) {
5757
; X86-NEXT: movl {{[0-9]+}}(%esp), %edx
5858
; X86-NEXT: addl {{[0-9]+}}(%esp), %eax
5959
; X86-NEXT: setb %cl
60-
; X86-NEXT: cmpb $1, %cl
60+
; X86-NEXT: negb %cl
6161
; X86-NEXT: adcl {{[0-9]+}}(%esp), %edx
6262
; X86-NEXT: retl
6363
%ret = add i64 %arg1, %arg2

llvm/test/CodeGen/X86/GlobalISel/select-add-x32.mir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ body: |
3232
; X32-NEXT: [[DEF3:%[0-9]+]]:gr32 = IMPLICIT_DEF
3333
; X32-NEXT: [[ADD32rr:%[0-9]+]]:gr32 = ADD32rr [[DEF]], [[DEF2]], implicit-def $eflags
3434
; X32-NEXT: [[SETCCr:%[0-9]+]]:gr8 = SETCCr 2, implicit $eflags
35-
; X32-NEXT: CMP8ri [[SETCCr]], 1, implicit-def $eflags
35+
; X32-NEXT: [[NEG8r:%[0-9]+]]:gr8 = NEG8r [[SETCCr]], implicit-def $eflags
3636
; X32-NEXT: [[ADC32rr:%[0-9]+]]:gr32 = ADC32rr [[DEF1]], [[DEF3]], implicit-def $eflags, implicit $eflags
3737
; X32-NEXT: [[SETCCr1:%[0-9]+]]:gr8 = SETCCr 2, implicit $eflags
3838
; X32-NEXT: $eax = COPY [[ADD32rr]]

llvm/test/CodeGen/X86/GlobalISel/sub-scalar.ll

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ define i128 @test_sub_i128(i128 %arg1, i128 %arg2) nounwind {
88
; X64-NEXT: movq %rdi, %rax
99
; X64-NEXT: subq %rdx, %rax
1010
; X64-NEXT: setb %dl
11-
; X64-NEXT: cmpb $1, %dl
11+
; X64-NEXT: negb %dl
1212
; X64-NEXT: sbbq %rcx, %rsi
1313
; X64-NEXT: movq %rsi, %rdx
1414
; X64-NEXT: retq
@@ -25,13 +25,13 @@ define i128 @test_sub_i128(i128 %arg1, i128 %arg2) nounwind {
2525
; X86-NEXT: movl {{[0-9]+}}(%esp), %edi
2626
; X86-NEXT: subl {{[0-9]+}}(%esp), %ecx
2727
; X86-NEXT: setb %bl
28-
; X86-NEXT: cmpb $1, %bl
28+
; X86-NEXT: negb %bl
2929
; X86-NEXT: sbbl {{[0-9]+}}(%esp), %edx
3030
; X86-NEXT: setb %bl
31-
; X86-NEXT: cmpb $1, %bl
31+
; X86-NEXT: negb %bl
3232
; X86-NEXT: sbbl {{[0-9]+}}(%esp), %esi
3333
; X86-NEXT: setb %bl
34-
; X86-NEXT: cmpb $1, %bl
34+
; X86-NEXT: negb %bl
3535
; X86-NEXT: sbbl {{[0-9]+}}(%esp), %edi
3636
; X86-NEXT: movl %ecx, (%eax)
3737
; X86-NEXT: movl %edx, 4(%eax)
@@ -58,7 +58,7 @@ define i64 @test_sub_i64(i64 %arg1, i64 %arg2) {
5858
; X86-NEXT: movl {{[0-9]+}}(%esp), %edx
5959
; X86-NEXT: subl {{[0-9]+}}(%esp), %eax
6060
; X86-NEXT: setb %cl
61-
; X86-NEXT: cmpb $1, %cl
61+
; X86-NEXT: negb %cl
6262
; X86-NEXT: sbbl {{[0-9]+}}(%esp), %edx
6363
; X86-NEXT: retl
6464
%ret = sub i64 %arg1, %arg2

0 commit comments

Comments
 (0)