[llvm] [DAGCombiner] Fix miscompile bug in combineShiftOfShiftedLogic (PR #89616)
Björn Pettersson via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 22 08:52:28 PDT 2024
https://github.com/bjope created https://github.com/llvm/llvm-project/pull/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.
>From dc363596067d0595e56ccc03a7bf2fdcb8f75f7d Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 22 Apr 2024 17:34:48 +0200
Subject: [PATCH 1/2] [DAGCombiner] Pre-commit test case for miscompile bug in
combineShiftOfShiftedLogic
DAGCombiner is trying to fold shl over binops, and in the process
combining it with another shl. However it needs to be more careful
to ensure that the sum of the shift counts fits in the type used
for the shift amount.
For example, X86 is using i8 as shift amount type. So we need to
make sure that the sum of the shift amounts isn't greater than 255.
Fix will be applied in a later commit. This only pre-commits the
test case to show that we currently get the wrong result.
Bug was found when testing the C23 BitInt feature.
---
llvm/test/CodeGen/X86/shift-combine.ll | 65 ++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/llvm/test/CodeGen/X86/shift-combine.ll b/llvm/test/CodeGen/X86/shift-combine.ll
index bb0fd9c68afbaf..67cb876cc2e702 100644
--- a/llvm/test/CodeGen/X86/shift-combine.ll
+++ b/llvm/test/CodeGen/X86/shift-combine.ll
@@ -787,3 +787,68 @@ define <4 x i32> @or_tree_with_mismatching_shifts_vec_i32(<4 x i32> %a, <4 x i32
%r = or <4 x i32> %or.ab, %or.cd
ret <4 x i32> %r
}
+
+; FIXME: Reproducer for a DAGCombiner::combineShiftOfShiftedLogic
+; bug. DAGCombiner need to check that the sum of the shift amounts fits in i8,
+; which is the legal type used to described X86 shift amounts. Verify that we
+; do not try to create a shift with 140+120 as shift amount, and verify that
+; the stored value do not depend on %a1.
+define void @combineShiftOfShiftedLogic(i128 %a1, i32 %a2, ptr %p) {
+; X86-LABEL: combineShiftOfShiftedLogic:
+; X86: # %bb.0:
+; X86-NEXT: pushl %ebx
+; X86-NEXT: .cfi_def_cfa_offset 8
+; X86-NEXT: pushl %edi
+; X86-NEXT: .cfi_def_cfa_offset 12
+; X86-NEXT: pushl %esi
+; X86-NEXT: .cfi_def_cfa_offset 16
+; X86-NEXT: .cfi_offset %esi, -16
+; X86-NEXT: .cfi_offset %edi, -12
+; X86-NEXT: .cfi_offset %ebx, -8
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl {{[0-9]+}}(%esp), %ecx
+; X86-NEXT: movl {{[0-9]+}}(%esp), %edx
+; X86-NEXT: movl {{[0-9]+}}(%esp), %ebx
+; X86-NEXT: movl {{[0-9]+}}(%esp), %edi
+; X86-NEXT: movl %edi, %esi
+; X86-NEXT: shldl $2, %ebx, %edi
+; X86-NEXT: shldl $2, %edx, %ebx
+; X86-NEXT: shrl $30, %esi
+; X86-NEXT: orl {{[0-9]+}}(%esp), %esi
+; X86-NEXT: shldl $2, %ecx, %edx
+; X86-NEXT: shll $2, %ecx
+; X86-NEXT: movl %edi, 16(%eax)
+; X86-NEXT: movl %ebx, 12(%eax)
+; X86-NEXT: movl %edx, 8(%eax)
+; X86-NEXT: movl %ecx, 4(%eax)
+; X86-NEXT: movl %esi, 20(%eax)
+; X86-NEXT: movl $0, (%eax)
+; X86-NEXT: popl %esi
+; X86-NEXT: .cfi_def_cfa_offset 12
+; X86-NEXT: popl %edi
+; X86-NEXT: .cfi_def_cfa_offset 8
+; X86-NEXT: popl %ebx
+; X86-NEXT: .cfi_def_cfa_offset 4
+; X86-NEXT: retl
+;
+; X64-LABEL: combineShiftOfShiftedLogic:
+; X64: # %bb.0:
+; X64-NEXT: # kill: def $edx killed $edx def $rdx
+; X64-NEXT: shlq $32, %rdx
+; X64-NEXT: movq %rsi, %rax
+; X64-NEXT: shrq $30, %rax
+; X64-NEXT: orq %rdx, %rax
+; X64-NEXT: shldq $34, %rdi, %rsi
+; X64-NEXT: shlq $34, %rdi
+; X64-NEXT: movq %rsi, 8(%rcx)
+; X64-NEXT: movq %rdi, (%rcx)
+; X64-NEXT: movq %rax, 16(%rcx)
+; X64-NEXT: retq
+ %zext1 = zext i128 %a1 to i192
+ %zext2 = zext i32 %a2 to i192
+ %shl = shl i192 %zext1, 130
+ %or = or i192 %shl, %zext2
+ %res = shl i192 %or, 160
+ store i192 %res, ptr %p, align 8
+ ret void
+}
>From d0596355469959054287b7d3d641863d3f33cd55 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 22 Apr 2024 17:47:57 +0200
Subject: [PATCH 2/2] [DAGCombiner] Fix miscompile bug in
combineShiftOfShiftedLogic
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.
---
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 9 +++-
llvm/test/CodeGen/X86/shift-combine.ll | 48 ++++---------------
2 files changed, 17 insertions(+), 40 deletions(-)
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index e6e0a1fc7d8277..fd265b12d73ca4 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -9503,8 +9503,15 @@ static SDValue combineShiftOfShiftedLogic(SDNode *Shift, SelectionDAG &DAG) {
if (ShiftAmtVal->getBitWidth() != C1Val.getBitWidth())
return false;
+ // The fold is not valid if the sum of the shift values doesn't fit in the
+ // given shift amount type.
+ bool Overflow = false;
+ APInt NewShiftAmt = C1Val.uadd_ov(*ShiftAmtVal, Overflow);
+ if (Overflow)
+ return false;
+
// The fold is not valid if the sum of the shift values exceeds bitwidth.
- if ((*ShiftAmtVal + C1Val).uge(V.getScalarValueSizeInBits()))
+ if (NewShiftAmt.uge(V.getScalarValueSizeInBits()))
return false;
return true;
diff --git a/llvm/test/CodeGen/X86/shift-combine.ll b/llvm/test/CodeGen/X86/shift-combine.ll
index 67cb876cc2e702..ca3898d358e221 100644
--- a/llvm/test/CodeGen/X86/shift-combine.ll
+++ b/llvm/test/CodeGen/X86/shift-combine.ll
@@ -796,53 +796,23 @@ define <4 x i32> @or_tree_with_mismatching_shifts_vec_i32(<4 x i32> %a, <4 x i32
define void @combineShiftOfShiftedLogic(i128 %a1, i32 %a2, ptr %p) {
; X86-LABEL: combineShiftOfShiftedLogic:
; X86: # %bb.0:
-; X86-NEXT: pushl %ebx
-; X86-NEXT: .cfi_def_cfa_offset 8
-; X86-NEXT: pushl %edi
-; X86-NEXT: .cfi_def_cfa_offset 12
-; X86-NEXT: pushl %esi
-; X86-NEXT: .cfi_def_cfa_offset 16
-; X86-NEXT: .cfi_offset %esi, -16
-; X86-NEXT: .cfi_offset %edi, -12
-; X86-NEXT: .cfi_offset %ebx, -8
; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
; X86-NEXT: movl {{[0-9]+}}(%esp), %ecx
-; X86-NEXT: movl {{[0-9]+}}(%esp), %edx
-; X86-NEXT: movl {{[0-9]+}}(%esp), %ebx
-; X86-NEXT: movl {{[0-9]+}}(%esp), %edi
-; X86-NEXT: movl %edi, %esi
-; X86-NEXT: shldl $2, %ebx, %edi
-; X86-NEXT: shldl $2, %edx, %ebx
-; X86-NEXT: shrl $30, %esi
-; X86-NEXT: orl {{[0-9]+}}(%esp), %esi
-; X86-NEXT: shldl $2, %ecx, %edx
-; X86-NEXT: shll $2, %ecx
-; X86-NEXT: movl %edi, 16(%eax)
-; X86-NEXT: movl %ebx, 12(%eax)
-; X86-NEXT: movl %edx, 8(%eax)
-; X86-NEXT: movl %ecx, 4(%eax)
-; X86-NEXT: movl %esi, 20(%eax)
-; X86-NEXT: movl $0, (%eax)
-; X86-NEXT: popl %esi
-; X86-NEXT: .cfi_def_cfa_offset 12
-; X86-NEXT: popl %edi
-; X86-NEXT: .cfi_def_cfa_offset 8
-; X86-NEXT: popl %ebx
-; X86-NEXT: .cfi_def_cfa_offset 4
+; X86-NEXT: movl %eax, 20(%ecx)
+; X86-NEXT: movl $0, 16(%ecx)
+; X86-NEXT: movl $0, 12(%ecx)
+; X86-NEXT: movl $0, 8(%ecx)
+; X86-NEXT: movl $0, 4(%ecx)
+; X86-NEXT: movl $0, (%ecx)
; X86-NEXT: retl
;
; X64-LABEL: combineShiftOfShiftedLogic:
; X64: # %bb.0:
; X64-NEXT: # kill: def $edx killed $edx def $rdx
; X64-NEXT: shlq $32, %rdx
-; X64-NEXT: movq %rsi, %rax
-; X64-NEXT: shrq $30, %rax
-; X64-NEXT: orq %rdx, %rax
-; X64-NEXT: shldq $34, %rdi, %rsi
-; X64-NEXT: shlq $34, %rdi
-; X64-NEXT: movq %rsi, 8(%rcx)
-; X64-NEXT: movq %rdi, (%rcx)
-; X64-NEXT: movq %rax, 16(%rcx)
+; X64-NEXT: movq %rdx, 16(%rcx)
+; X64-NEXT: movq $0, 8(%rcx)
+; X64-NEXT: movq $0, (%rcx)
; X64-NEXT: retq
%zext1 = zext i128 %a1 to i192
%zext2 = zext i32 %a2 to i192
More information about the llvm-commits
mailing list