[DAGCombiner] Fix miscompile bug in combineShiftOfShiftedLogic#89616
Merged
[DAGCombiner] Fix miscompile bug in combineShiftOfShiftedLogic#89616
Conversation
Member
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: Björn Pettersson (bjope) ChangesEnsure that the sum of the shift amounts does not overflow the Solves a miscompile bug found when testing the C23 BitInt feature. Targets like X86 that only use an i8 for shift amounts after Full diff: https://github.com/llvm/llvm-project/pull/89616.diff 2 Files Affected:
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 bb0fd9c68afbaf..ca3898d358e221 100644
--- a/llvm/test/CodeGen/X86/shift-combine.ll
+++ b/llvm/test/CodeGen/X86/shift-combine.ll
@@ -787,3 +787,38 @@ 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: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl {{[0-9]+}}(%esp), %ecx
+; 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 %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
+ %shl = shl i192 %zext1, 130
+ %or = or i192 %shl, %zext2
+ %res = shl i192 %or, 160
+ store i192 %res, ptr %p, align 8
+ ret void
+}
|
nikic
reviewed
Apr 23, 2024
| ret <4 x i32> %r | ||
| } | ||
|
|
||
| ; FIXME: Reproducer for a DAGCombiner::combineShiftOfShiftedLogic |
Collaborator
Author
There was a problem hiding this comment.
Thanks! (I actually realized that I had forgotten to remove this when I woke up this morning, but was not quick enough to fix it before you found it.)
jayfoad
reviewed
Apr 23, 2024
| ; 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 |
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.
Contributor
SiliconA-Z
pushed a commit
to SiliconA-Z/llvm-project
that referenced
this pull request
Apr 23, 2024
…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)
Member
SiliconA-Z
pushed a commit
to SiliconA-Z/llvm-project
that referenced
this pull request
Apr 23, 2024
…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)
tstellar
pushed a commit
to SiliconA-Z/llvm-project
that referenced
this pull request
Apr 25, 2024
…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)
Closed
Tedlion
pushed a commit
to Tedlion/llvm-project
that referenced
this pull request
Jun 15, 2025
…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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.