[DAG] fix wrong type check in DAGCombiner::visitSRA #153762
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 |
|
@woruyu @StephenYoung2754 We now have 2 identical PRs for this with exactly the same issue - no test coverage :( |
Good catch—these two PRs are duplicates and both lack tests. |
| TLI.isOperationLegalOrCustom(ISD::SIGN_EXTEND, TruncVT) && | ||
| TLI.isOperationLegalOrCustom(ISD::TRUNCATE, VT) && | ||
| TLI.isOperationLegalOrCustom(ISD::SIGN_EXTEND, VT) && | ||
| TLI.isOperationLegalOrCustom(ISD::TRUNCATE, TruncVT) && |
There was a problem hiding this comment.
This amdgpu testcase hits this path:
define i64 @foo(i64 %x) {
%shl = shl i64 %x, 22
%sra = ashr i64 %shl, 32
ret i64 %sra
}
But there's no net change in codegen after this change, probably because we have custom combines to hack out i64 shifts. Is the inner shift dropping flags that can be preserved?
Thanks for the pointer about missing coverage.The existing tests already exercise the fold of you need to find two type(the first value's bool value is 1,x,1,and the second type's bool value is 1,1,x) and the first should be transfer to the second, Obviously, you can't creat a diff testcase. Any suggestion? Friendly ping, @StephenYoung2754 |
Summary
This PR resolves #153543