[RISCV] Remove what appears to be an unnecessary check for LMUL_8 in getSingleShuffleSrc. #127250
Conversation
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
…getSingleShuffleSrc. I think this dates to a time when we used to use a type twice as large as necessary for the input to the vnsrl. This was changed in The existing test for this regresses because it uses a lot of undef elements and we previously figured out we could reduce its size and then try the vnsrl again. We now match it before we try to reduce the width so we miss this opportunity. I've added a second test that doesn't have any undef elements in the first half. Prior to this patch we used a vcompress lowering instead of vnsrl.
topperc
changed the title
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesI think this dates to a time when we used to use a type twice as large as necessary for the input to the vnsrl. This was changed in The existing test for this regresses because it uses a lot of undef elements and we previously figured out we could reduce its size and then try the vnsrl again. We now match it before we try to reduce the width so we miss this opportunity. I've added a second test that doesn't have any undef elements in the first half. Prior to this patch we used a vcompress lowering instead of vnsrl. Full diff: https://github.com/llvm/llvm-project/pull/127250.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index da04880348af6..122d717b0ef1b 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -4498,11 +4498,9 @@ static SDValue lowerScalarInsert(SDValue Scalar, SDValue VL, MVT VT,
}
// Can this shuffle be performed on exactly one (possibly larger) input?
-static SDValue getSingleShuffleSrc(MVT VT, MVT ContainerVT, SDValue V1,
- SDValue V2) {
+static SDValue getSingleShuffleSrc(MVT VT, SDValue V1, SDValue V2) {
- if (V2.isUndef() &&
- RISCVTargetLowering::getLMUL(ContainerVT) != RISCVII::VLMUL::LMUL_8)
+ if (V2.isUndef())
return V1;
// Both input must be extracts.
@@ -5577,7 +5575,7 @@ static SDValue lowerVECTOR_SHUFFLE(SDValue Op, SelectionDAG &DAG,
unsigned Index = 0;
if (ShuffleVectorInst::isDeInterleaveMaskOfFactor(Mask, Factor, Index) &&
1 < count_if(Mask, [](int Idx) { return Idx != -1; })) {
- if (SDValue Src = getSingleShuffleSrc(VT, ContainerVT, V1, V2))
+ if (SDValue Src = getSingleShuffleSrc(VT, V1, V2))
return getDeinterleaveShiftAndTrunc(DL, VT, Src, Factor, Index, DAG);
}
}
diff --git a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.ll b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.ll
index 3c28e978842b9..4e5ef9c002f1a 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.ll
@@ -517,15 +517,15 @@ entry:
ret void
}
-; Can't match the m8 result type as the source would have to be m16 which
-; isn't a legal type.
+; FIXME: We could use a smaller vl for the vnsrl since some elts are undefined.
define void @vnsrl_0_i32_single_src_m8(ptr %in, ptr %out) {
; V-LABEL: vnsrl_0_i32_single_src_m8:
; V: # %bb.0: # %entry
; V-NEXT: li a2, 64
; V-NEXT: vsetvli zero, a2, e32, m8, ta, ma
; V-NEXT: vle32.v v8, (a0)
-; V-NEXT: vsetivli zero, 16, e32, m2, ta, ma
+; V-NEXT: li a0, 32
+; V-NEXT: vsetvli zero, a0, e32, m4, ta, ma
; V-NEXT: vnsrl.wi v16, v8, 0
; V-NEXT: vsetvli zero, a2, e32, m8, ta, ma
; V-NEXT: vse32.v v16, (a1)
@@ -551,3 +551,36 @@ entry:
store <64 x i32> %shuffle.i5, ptr %out, align 4
ret void
}
+
+define void @vnsrl_0_i32_single_src_m8_2(ptr %in, ptr %out) {
+; V-LABEL: vnsrl_0_i32_single_src_m8_2:
+; V: # %bb.0: # %entry
+; V-NEXT: li a2, 64
+; V-NEXT: vsetvli zero, a2, e32, m8, ta, ma
+; V-NEXT: vle32.v v8, (a0)
+; V-NEXT: li a0, 32
+; V-NEXT: vsetvli zero, a0, e32, m4, ta, ma
+; V-NEXT: vnsrl.wi v16, v8, 0
+; V-NEXT: vsetvli zero, a2, e32, m8, ta, ma
+; V-NEXT: vse32.v v16, (a1)
+; V-NEXT: ret
+;
+; ZVE32F-LABEL: vnsrl_0_i32_single_src_m8_2:
+; ZVE32F: # %bb.0: # %entry
+; ZVE32F-NEXT: li a2, 64
+; ZVE32F-NEXT: vsetvli zero, a2, e32, m8, ta, ma
+; ZVE32F-NEXT: vle32.v v8, (a0)
+; ZVE32F-NEXT: lui a0, 349525
+; ZVE32F-NEXT: addi a0, a0, 1365
+; ZVE32F-NEXT: vsetivli zero, 2, e32, m1, ta, ma
+; ZVE32F-NEXT: vmv.v.x v16, a0
+; ZVE32F-NEXT: vsetvli zero, a2, e32, m8, ta, ma
+; ZVE32F-NEXT: vcompress.vm v24, v8, v16
+; ZVE32F-NEXT: vse32.v v24, (a1)
+; ZVE32F-NEXT: ret
+entry:
+ %0 = load <64 x i32>, ptr %in, align 4
+ %shuffle.i5 = shufflevector <64 x i32> %0, <64 x i32> poison, <64 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14, i32 16, i32 18, i32 20, i32 22, i32 24, i32 26, i32 28, i32 30, i32 32, i32 34, i32 36, i32 38, i32 40, i32 42, i32 44, i32 46, i32 48, i32 50, i32 52, i32 54, i32 56, i32 58, i32 60, i32 62, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+ store <64 x i32> %shuffle.i5, ptr %out, align 4
+ ret void
+}
|
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 0931a2a5a26f4926fc74087017a8a6ba3ab874df 9d5aa406fdfad7ba5ef6017bcd156d2e51ff86b7 llvm/lib/Target/RISCV/RISCVISelLowering.cpp llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.llThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
Looks like this comment didn't get completed. LGTM w/that addressed or the description otherwise reworked. |
preames
approved these changes
Feb 15, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/12303 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/12651 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/7080 Here is the relevant piece of the build log for the reference |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…getSingleShuffleSrc. (llvm#127250) I think this dates to a time when we used to use a type twice as large as necessary for the input to the vnsrl. This was changed in llvm#118509 when factor 4 and 8 were added. The existing test for this regresses because it uses a lot of undef elements and we previously figured out we could reduce its size and then try the vnsrl again. We now match it before we try to reduce the width so we miss this opportunity. I've added a second test that doesn't have any undef elements in the first half. Prior to this patch we used a vcompress lowering instead of vnsrl.
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.
I think this dates to a time when we used to use a type twice as large as necessary for the input to the vnsrl. This was changed in #118509 when factor 4 and 8 were added.
The existing test for this regresses because it uses a lot of undef elements and we previously figured out we could reduce its size and then try the vnsrl again. We now match it before we try to reduce the width so we miss this opportunity.
I've added a second test that doesn't have any undef elements in the first half. Prior to this patch we used a vcompress lowering instead of vnsrl.