Skip to content
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

[RISCV] Truncate constants to eltwidth before checking simm5 when con… #67062

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Sep 21, 2023

…verting VMV_V_X to VMV_X_S.

Instruction selection knows the bits past EltWidth are ignored, we should do the same here.

…verting VMV_V_X to VMV_X_S.

Instruction selection knows the bits past EltWidth are ignored, we should
do the same here.
; CHECK-LABEL: shuffle_v8i32_2:
; CHECK: # %bb.0:
; CHECK-NEXT: vsetivli zero, 1, e8, mf8, ta, ma
; CHECK-NEXT: vmv.v.i v0, -13
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this patch we get li a0, 243 and vmv.s.x v0, a0

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 21, 2023

@llvm/pr-subscribers-backend-risc-v

Changes

…verting VMV_V_X to VMV_X_S.

Instruction selection knows the bits past EltWidth are ignored, we should do the same here.


Full diff: https://github.com/llvm/llvm-project/pull/67062.diff

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+2-1)
  • (modified) llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-shuffles.ll (+13)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 8b745b2afaf956b..1039d52a3f6cf7b 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -14435,7 +14435,8 @@ SDValue RISCVTargetLowering::PerformDAGCombine(SDNode *N,
     // patterns on rv32..
     ConstantSDNode *Const = dyn_cast<ConstantSDNode>(Scalar);
     if (isOneConstant(VL) && EltWidth <= Subtarget.getXLen() &&
-        (!Const || Const->isZero() || !isInt<5>(Const->getSExtValue())))
+        (!Const || Const->isZero() ||
+         !Const->getAPIntValue().sextOrTrunc(EltWidth).isSignedIntN(5)))
       return DAG.getNode(RISCVISD::VMV_S_X_VL, DL, VT, Passthru, Scalar, VL);
 
     break;
diff --git a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-shuffles.ll b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-shuffles.ll
index 78cafbb84bb926e..927fd3e203355c0 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-shuffles.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-shuffles.ll
@@ -781,3 +781,16 @@ define <8 x i8> @unmergable(<8 x i8> %v, <8 x i8> %w) {
   %res = shufflevector <8 x i8> %v, <8 x i8> %w, <8 x i32> <i32 2, i32 9, i32 4, i32 11, i32 6, i32 13, i32 8, i32 15>
   ret <8 x i8> %res
 }
+
+; Make sure we use a vmv.v.i to load the mask constant.
+define <8 x i32> @shuffle_v8i32_2(<8 x i32> %x, <8 x i32> %y) {
+; CHECK-LABEL: shuffle_v8i32_2:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vsetivli zero, 1, e8, mf8, ta, ma
+; CHECK-NEXT:    vmv.v.i v0, -13
+; CHECK-NEXT:    vsetivli zero, 8, e32, m2, ta, ma
+; CHECK-NEXT:    vmerge.vvm v8, v10, v8, v0
+; CHECK-NEXT:    ret
+  %s = shufflevector <8 x i32> %x, <8 x i32> %y, <8 x i32> <i32 0, i32 1, i32 10, i32 11, i32 4, i32 5, i32 6, i32 7>
+  ret <8 x i32> %s
+}

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you looking at this bit of code, can I ask for a review on https://reviews.llvm.org/D159230?

@@ -14435,7 +14435,8 @@ SDValue RISCVTargetLowering::PerformDAGCombine(SDNode *N,
// patterns on rv32..
ConstantSDNode *Const = dyn_cast<ConstantSDNode>(Scalar);
if (isOneConstant(VL) && EltWidth <= Subtarget.getXLen() &&
(!Const || Const->isZero() || !isInt<5>(Const->getSExtValue())))
(!Const || Const->isZero() ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that SimplifyDemandedLowBitsHelper just above can't catch this case? If only the low bits are demanded - which I think is what you're suggesting here - shouldn't that have folded the constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the DemandedBits will turn a zero extended i64 constant into a sign extended i64 constant if the upper bits aren't used. Is that what you're suggesting?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the question I'd asked, but to reframe it, why not go ahead and convert the constant to the sign extended form eagerly? Even if we don't convert to the vmv_s_x, isn't that a reasonable canonicalization of the vmv_v_x?

p.s. I'm completely fine with this landing as is, just suggesting a potentially cleaner way of doing it.

Copy link
Contributor

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@topperc topperc merged commit ec5b0ef into llvm:main Sep 22, 2023
3 checks passed
@topperc topperc deleted the pr/vmv_x_s branch September 22, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants