Skip to content

Commit 78b99c7

Browse files
bjopetstellar
authored andcommitted
[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. (cherry picked from commit f9b419b)
1 parent 1aa9172 commit 78b99c7

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
@@ -9636,8 +9636,15 @@ static SDValue combineShiftOfShiftedLogic(SDNode *Shift, SelectionDAG &DAG) {
96369636
if (ShiftAmtVal->getBitWidth() != C1Val.getBitWidth())
96379637
return false;
96389638

9639+
// The fold is not valid if the sum of the shift values doesn't fit in the
9640+
// given shift amount type.
9641+
bool Overflow = false;
9642+
APInt NewShiftAmt = C1Val.uadd_ov(*ShiftAmtVal, Overflow);
9643+
if (Overflow)
9644+
return false;
9645+
96399646
// The fold is not valid if the sum of the shift values exceeds bitwidth.
9640-
if ((*ShiftAmtVal + C1Val).uge(V.getScalarValueSizeInBits()))
9647+
if (NewShiftAmt.uge(V.getScalarValueSizeInBits()))
96419648
return false;
96429649

96439650
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)