-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[DAG] fix wrong type check in DAGCombiner::visitSRA #153762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-llvm-selectiondag Author: woruyu (woruyu) ChangesSummary Full diff: https://github.com/llvm/llvm-project/pull/153762.diff 1 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index d343b644e41cb..245070b8b30be 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -10872,8 +10872,8 @@ SDValue DAGCombiner::visitSRA(SDNode *N) {
// on that type, and the truncate to that type is both legal and free,
// perform the transform.
if ((ShiftAmt > 0) &&
- TLI.isOperationLegalOrCustom(ISD::SIGN_EXTEND, TruncVT) &&
- TLI.isOperationLegalOrCustom(ISD::TRUNCATE, VT) &&
+ TLI.isOperationLegalOrCustom(ISD::SIGN_EXTEND, VT) &&
+ TLI.isOperationLegalOrCustom(ISD::TRUNCATE, TruncVT) &&
TLI.isTruncateFree(VT, TruncVT)) {
SDValue Amt = DAG.getShiftAmountConstant(ShiftAmt, VT, DL);
SDValue Shift = DAG.getNode(ISD::SRL, DL, VT,
|
Just check type exchanged! Is it necessary to add a testcase? |
If at all possible - yes |
I tried, but finding a case where |
The crash came from an out of tree target that I can't share, so I'm afraid I can't supply a test for it. |
I can provide the selectionDAG before and after the optimization takes place when it shouldn't:
|
Thank you for the DAG! @akiva-pripas, however, I tried to create a lit test that hits this change, but it’s very hard to reproduce reliably. Would it be acceptable to land the fix without a test?(just for trade-off). ; RUN: llc -mtriple=x86_64-pc-linux-gnu -mattr=+avx512f -O2 < %s | FileCheck %s
define void @extract_bit31_mask_v8i64(<8 x i64>* %dst, <8 x i64>* %src) {
entry:
%x = load <8 x i64>, <8 x i64>* %src, align 64
%v31 = shufflevector <8 x i64> <i64 31, i64 31, i64 31, i64 31, i64 31, i64 31, i64 31, i64 31>,
<8 x i64> undef,
<8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
%v63 = shufflevector <8 x i64> <i64 63, i64 63, i64 63, i64 63, i64 63, i64 63, i64 63, i64 63>,
<8 x i64> undef,
<8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
%shr = lshr <8 x i64> %x, %v31
%shl = shl <8 x i64> %shr,
<i64 62, i64 62, i64 62, i64 62,
i64 62, i64 62, i64 62, i64 62>
%sra = ashr <8 x i64> %shl, %v63
store <8 x i64> %sra, <8 x i64>* %dst, align 64
ret void
}
|
I'd still add that testcase that shows the pattern, though you should fix the opaque pointers and use of undef in it |
Summary
This PR resolves #153543