release/18.x: [DAGCombiner] In mergeTruncStore, make sure we aren't storing shifted in bits. (#90939) #91038
Conversation
|
@AtariDreams This bug has existed since at least LLVM 10. What makes it a candidate for backporting? |
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-aarch64 Author: AtariDreams (AtariDreams) ChangesWhen looking through a right shift, we need to make sure that all of the bits we are using from the shift come from the shift input and not the sign or zero bits that are shifted in. Fixes #90936. (cherry picked from commit 3563af6) Full diff: https://github.com/llvm/llvm-project/pull/91038.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 5038f8a1fc156..4951e45edb9ed 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -8952,6 +8952,10 @@ SDValue DAGCombiner::mergeTruncStores(StoreSDNode *N) {
if (ShiftAmtC % NarrowNumBits != 0)
return SDValue();
+ // Make sure we aren't reading bits that are shifted in.
+ if (ShiftAmtC > WideVal.getScalarValueSizeInBits() - NarrowNumBits)
+ return SDValue();
+
Offset = ShiftAmtC / NarrowNumBits;
WideVal = WideVal.getOperand(0);
}
diff --git a/llvm/test/CodeGen/AArch64/pr90936.ll b/llvm/test/CodeGen/AArch64/pr90936.ll
new file mode 100644
index 0000000000000..38cda8d388945
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/pr90936.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc < %s -mtriple=aarch64 | FileCheck %s
+
+define void @f(i16 %arg, ptr %arg1) {
+; CHECK-LABEL: f:
+; CHECK: // %bb.0:
+; CHECK-NEXT: ubfx w8, w0, #8, #6
+; CHECK-NEXT: strb w0, [x1]
+; CHECK-NEXT: strb w8, [x1, #1]
+; CHECK-NEXT: ret
+bb:
+ %i = trunc i16 %arg to i8
+ %i2 = trunc i16 %arg to i14
+ %i3 = lshr i14 %i2, 8
+ store i8 %i, ptr %arg1, align 1
+ %i4 = getelementptr i8, ptr %arg1, i64 1
+ %i5 = trunc i14 %i3 to i8
+ store i8 %i5, ptr %i4, align 1
+ ret void
+}
|
At best, if the code triggers, we abort the fold, so there is no risk of anything crazy going on if this is added. If this code were to instead try alternate folding, there is a risk that a bug may slip through, however, given that this is simply aborting the merge, there is no harm in leaving the DAG untouched. So, the worst that could happen:
Pros:
Again, less risk of miscompile because this just bails out of the merge just like the condition above this code does. |
|
We do not need to know how to fold every single possible permutation that comes our way, especially if they are so rare that writing compile code optimizing it isn't even worth it. We do, however, need to strive to avoid miscompiles wherever we can, no matter how esoteric the code is. Now, this isn't always possible, but in this case, the alternative codepath given just bails the transform, which is preferable to folding something that should not be. |
|
@topperc Do you have any strong objections to backporting this? This looks small to me and I think it's OK to fix long-standing bugs. |
No objection. |
|
@AtariDreams I've noticed you've filed a lot of backport requests. How are you choosing which fixes to backport? Is there a specific use case you care about? |
There a particular LLVM miscompile bug in WebKit I'm trying to figure out. It's been there since 2019. Backports is literally just avoiding miscompilations |
|
@tstellar AtariDreams is requesting backports for random commits that somehow mention miscompilations or crashes, without having any understanding of what the changes are about or how they relate to other changes. They have submitted a large amount of invalid or nonsensical backports for that reason, despite many requests to stop doing so. When there is any doubt, you should not merge backport PRs created by this user. That said, this specific one does look harmless to me. |
@AtariDreams Has the bug disappeared in llvm trunk and you think a recent commit has fixed/hidden it? Has this bug been reported either to WebKit or LLVM that we can track please? Have you been able to confirm if its a llvm bug or UB in WebKit? |
… in bits. (llvm#90939) When looking through a right shift, we need to make sure that all of the bits we are using from the shift come from the shift input and not the sign or zero bits that are shifted in. Fixes llvm#90936. (cherry picked from commit 3563af6)
When looking through a right shift, we need to make sure that all of the bits we are using from the shift come from the shift input and not the sign or zero bits that are shifted in.
Fixes #90936.
(cherry picked from commit 3563af6)