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] Add missing RISCVMaskedPseudo for TIED pseudos #86787

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Mar 27, 2024

This was preventing us from folding away the vmerge into its mask.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 27, 2024

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

Author: Luke Lau (lukel97)

Changes

This was preventing us from folding away the vmerge into its mask.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td (+2-1)
  • (modified) llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll (+2-5)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td b/llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
index e42ac68a8b67ff..0ed7f0a43ed57f 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
@@ -2212,7 +2212,8 @@ multiclass VPseudoTiedBinary<VReg RetClass,
     def "_" # MInfo.MX # "_TIED": VPseudoTiedBinaryNoMask<RetClass, Op2Class,
                                                           Constraint, TargetConstraintType>;
     def "_" # MInfo.MX # "_MASK_TIED" : VPseudoTiedBinaryMask<RetClass, Op2Class,
-                                                         Constraint, TargetConstraintType>;
+                                                         Constraint, TargetConstraintType>,
+                                        RISCVMaskedPseudo<MaskIdx=2>;
   }
 }
 
diff --git a/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll b/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll
index 571e2df13c2636..21ae1580503afe 100644
--- a/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll
@@ -1192,11 +1192,8 @@ define <vscale x 2 x i32> @vmerge_larger_vl_false_becomes_tail(<vscale x 2 x i32
 define <vscale x 2 x i64> @vpmerge_vwsub.w_tied(<vscale x 2 x i64> %passthru, <vscale x 2 x i64> %x, <vscale x 2 x i32> %y, <vscale x 2 x i1> %mask, i32 zeroext %vl) {
 ; CHECK-LABEL: vpmerge_vwsub.w_tied:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    vsetvli zero, a0, e32, m1, tu, ma
-; CHECK-NEXT:    vmv2r.v v10, v8
-; CHECK-NEXT:    vwsub.wv v10, v10, v12
-; CHECK-NEXT:    vsetvli zero, zero, e64, m2, tu, ma
-; CHECK-NEXT:    vmerge.vvm v8, v8, v10, v0
+; CHECK-NEXT:    vsetvli zero, a0, e32, m1, tu, mu
+; CHECK-NEXT:    vwsub.wv v8, v8, v12, v0.t
 ; CHECK-NEXT:    ret
   %vl.zext = zext i32 %vl to i64
   %a = call <vscale x 2 x i64> @llvm.riscv.vwsub.w.nxv2i64.nxv2i32(<vscale x 2 x i64> %passthru, <vscale x 2 x i64> %passthru, <vscale x 2 x i32> %y, i64 %vl.zext)

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

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

LGTM.
I don't know if we need it for VPseudoTiedBinaryRoundingMode.

@@ -1192,11 +1192,8 @@ define <vscale x 2 x i32> @vmerge_larger_vl_false_becomes_tail(<vscale x 2 x i32
define <vscale x 2 x i64> @vpmerge_vwsub.w_tied(<vscale x 2 x i64> %passthru, <vscale x 2 x i64> %x, <vscale x 2 x i32> %y, <vscale x 2 x i1> %mask, i32 zeroext %vl) {
Copy link
Member

Choose a reason for hiding this comment

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

Might not related to this patch but is the %x necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, it's not. Will remove it after this PR!

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.

LGTM w/comment.

Note that this is a slightly larger change than it first appears. We're not adding the annotation for another instance of a TIED pseudo, this is the first instance of the TIED pseudo.

Looking at the existing code in performCombineVMergeAndVOps, I don't see a problem, but please make sure you audit this code closely - if you haven't already.

@lukel97 lukel97 force-pushed the tied-pseudo-masked-pseudo-class branch from 603431f to 2a0bdb9 Compare March 29, 2024 11:56
@lukel97
Copy link
Contributor Author

lukel97 commented Mar 29, 2024

LGTM w/comment.

Note that this is a slightly larger change than it first appears. We're not adding the annotation for another instance of a TIED pseudo, this is the first instance of the TIED pseudo.

Looking at the existing code in performCombineVMergeAndVOps, I don't see a problem, but please make sure you audit this code closely - if you haven't already.

Yeah, we should already have all the bits in place to handle tied pseudos since they share the same operand layout as VPseudoUnaryNoMask. As far as I can tell the two pseudo classes are almost the same

class VPseudoTiedBinaryNoMask<VReg RetClass,
                              DAGOperand Op2Class,
                              string Constraint,
                              int TargetConstraintType = 1> :
      Pseudo<(outs RetClass:$rd),
             (ins RetClass:$rs2, Op2Class:$rs1, AVL:$vl, ixlenimm:$sew,
                  ixlenimm:$policy), []>,
      RISCVVPseudo {
  let mayLoad = 0;
  let mayStore = 0;
  let hasSideEffects = 0;
  let Constraints = !interleave([Constraint, "$rd = $rs2"], ",");
  let TargetOverlapConstraintType = TargetConstraintType;
  let HasVLOp = 1;
  let HasSEWOp = 1;
  let HasVecPolicyOp = 1;
  let isConvertibleToThreeAddress = 1;
  let IsTiedPseudo = 1;
}

class VPseudoUnaryNoMask<DAGOperand RetClass,
                         DAGOperand OpClass,
                         string Constraint = "",
                         int TargetConstraintType = 1> :
      Pseudo<(outs RetClass:$rd),
             (ins RetClass:$merge, OpClass:$rs2,
                  AVL:$vl, ixlenimm:$sew, ixlenimm:$policy), []>,
      RISCVVPseudo {
  let mayLoad = 0;
  let mayStore = 0;
  let hasSideEffects = 0;
  let Constraints = !interleave([Constraint, "$rd = $merge"], ",");
  let TargetOverlapConstraintType = TargetConstraintType;
  let HasVLOp = 1;
  let HasSEWOp = 1;
  let HasVecPolicyOp = 1;
}

@lukel97 lukel97 merged commit 3f69d90 into llvm:main Mar 29, 2024
4 checks passed
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

5 participants