Skip to content

Commit f9b419b

Browse files
authored
[DAGCombiner] Fix miscompile bug in combineShiftOfShiftedLogic (#89616)
Ensure that the sum of the shift amounts does not overflow the shift amount type when combining shifts in combineShiftOfShiftedLogic. Solves a miscompile bug found when testing the C23 BitInt feature. Targets like X86 that only use an i8 for shift amounts after legalization seems to be extra susceptible for bugs like this as it isn't legal to shift more than 255 steps.
1 parent 5fd9bbd commit f9b419b

File tree

2 files changed

+22
-45
lines changed

2 files changed

+22
-45
lines changed

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

+8-1
Original file line numberDiff line numberDiff line change
@@ -9503,8 +9503,15 @@ static SDValue combineShiftOfShiftedLogic(SDNode *Shift, SelectionDAG &DAG) {
95039503
if (ShiftAmtVal->getBitWidth() != C1Val.getBitWidth())
95049504
return false;
95059505

9506+
// The fold is not valid if the sum of the shift values doesn't fit in the
9507+
// given shift amount type.
9508+
bool Overflow = false;
9509+
APInt NewShiftAmt = C1Val.uadd_ov(*ShiftAmtVal, Overflow);
9510+
if (Overflow)
9511+
return false;
9512+
95069513
// The fold is not valid if the sum of the shift values exceeds bitwidth.
9507-
if ((*ShiftAmtVal + C1Val).uge(V.getScalarValueSizeInBits()))
9514+
if (NewShiftAmt.uge(V.getScalarValueSizeInBits()))
95089515
return false;
95099516

95109517
return true;

llvm/test/CodeGen/X86/shift-combine.ll

+14-44
Original file line numberDiff line numberDiff line change
@@ -788,61 +788,31 @@ define <4 x i32> @or_tree_with_mismatching_shifts_vec_i32(<4 x i32> %a, <4 x i32
788788
ret <4 x i32> %r
789789
}
790790

791-
; FIXME: Reproducer for a DAGCombiner::combineShiftOfShiftedLogic
792-
; bug. DAGCombiner need to check that the sum of the shift amounts fits in i8,
793-
; which is the legal type used to described X86 shift amounts. Verify that we
794-
; do not try to create a shift with 130+160 as shift amount, and verify that
795-
; the stored value do not depend on %a1.
791+
; Reproducer for a DAGCombiner::combineShiftOfShiftedLogic bug. DAGCombiner
792+
; need to check that the sum of the shift amounts fits in i8, which is the
793+
; legal type used to described X86 shift amounts. Verify that we do not try to
794+
; create a shift with 130+160 as shift amount, and verify that the stored
795+
; value do not depend on %a1.
796796
define void @combineShiftOfShiftedLogic(i128 %a1, i32 %a2, ptr %p) {
797797
; X86-LABEL: combineShiftOfShiftedLogic:
798798
; X86: # %bb.0:
799-
; X86-NEXT: pushl %ebx
800-
; X86-NEXT: .cfi_def_cfa_offset 8
801-
; X86-NEXT: pushl %edi
802-
; X86-NEXT: .cfi_def_cfa_offset 12
803-
; X86-NEXT: pushl %esi
804-
; X86-NEXT: .cfi_def_cfa_offset 16
805-
; X86-NEXT: .cfi_offset %esi, -16
806-
; X86-NEXT: .cfi_offset %edi, -12
807-
; X86-NEXT: .cfi_offset %ebx, -8
808799
; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
809800
; X86-NEXT: movl {{[0-9]+}}(%esp), %ecx
810-
; X86-NEXT: movl {{[0-9]+}}(%esp), %edx
811-
; X86-NEXT: movl {{[0-9]+}}(%esp), %ebx
812-
; X86-NEXT: movl {{[0-9]+}}(%esp), %edi
813-
; X86-NEXT: movl %edi, %esi
814-
; X86-NEXT: shldl $2, %ebx, %edi
815-
; X86-NEXT: shldl $2, %edx, %ebx
816-
; X86-NEXT: shrl $30, %esi
817-
; X86-NEXT: orl {{[0-9]+}}(%esp), %esi
818-
; X86-NEXT: shldl $2, %ecx, %edx
819-
; X86-NEXT: shll $2, %ecx
820-
; X86-NEXT: movl %edi, 16(%eax)
821-
; X86-NEXT: movl %ebx, 12(%eax)
822-
; X86-NEXT: movl %edx, 8(%eax)
823-
; X86-NEXT: movl %ecx, 4(%eax)
824-
; X86-NEXT: movl %esi, 20(%eax)
825-
; X86-NEXT: movl $0, (%eax)
826-
; X86-NEXT: popl %esi
827-
; X86-NEXT: .cfi_def_cfa_offset 12
828-
; X86-NEXT: popl %edi
829-
; X86-NEXT: .cfi_def_cfa_offset 8
830-
; X86-NEXT: popl %ebx
831-
; X86-NEXT: .cfi_def_cfa_offset 4
801+
; X86-NEXT: movl %eax, 20(%ecx)
802+
; X86-NEXT: movl $0, 16(%ecx)
803+
; X86-NEXT: movl $0, 12(%ecx)
804+
; X86-NEXT: movl $0, 8(%ecx)
805+
; X86-NEXT: movl $0, 4(%ecx)
806+
; X86-NEXT: movl $0, (%ecx)
832807
; X86-NEXT: retl
833808
;
834809
; X64-LABEL: combineShiftOfShiftedLogic:
835810
; X64: # %bb.0:
836811
; X64-NEXT: # kill: def $edx killed $edx def $rdx
837812
; X64-NEXT: shlq $32, %rdx
838-
; X64-NEXT: movq %rsi, %rax
839-
; X64-NEXT: shrq $30, %rax
840-
; X64-NEXT: orq %rdx, %rax
841-
; X64-NEXT: shldq $34, %rdi, %rsi
842-
; X64-NEXT: shlq $34, %rdi
843-
; X64-NEXT: movq %rsi, 8(%rcx)
844-
; X64-NEXT: movq %rdi, (%rcx)
845-
; X64-NEXT: movq %rax, 16(%rcx)
813+
; X64-NEXT: movq %rdx, 16(%rcx)
814+
; X64-NEXT: movq $0, 8(%rcx)
815+
; X64-NEXT: movq $0, (%rcx)
846816
; X64-NEXT: retq
847817
%zext1 = zext i128 %a1 to i192
848818
%zext2 = zext i32 %a2 to i192

0 commit comments

Comments
 (0)