[PATCH] D101826: [RISCV][VP] Lower VP ISD nodes to RVV instructions

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D101826#2738216 <https://reviews.llvm.org/D101826#2738216>, @rogfer01 wrote:

> Hi @frasercrmck, tests are using `zeroext`, is this for simpler code generation before we are able to combine this case and avoid unnecessary sign extensions?

Yeah exactly. I'm using `zeroext` mostly because it means we can use the same IR and get the same output on both RV32 and RV64 (the zero-extension is not required on RV32 and is eliminated on RV64). Otherwise I think we'd need separated RV32 and RV64 tests. In the real world I don't think we'd get the VL from a function parameter very often so it's a contrived example.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:507
+        // RV64 must custom-legalize the i32 EVL parameter.
+        if (Subtarget.is64Bit())
+          setOperationAction(VPOpc, MVT::i32, Custom);
----------------
craig.topper wrote:
> Is this something we should fix in the type legalizer?
Do you mean that the legalizer would zero-extend the EVL if the operand required promotion, and the VP nodes would have to accommodate an EVL operand that may not be i32? I'd be interested in experimenting with that because we'd get another round of DAG combining on the generic nodes.

I don't have a good feeling for the impact. Maybe @simoll could weigh in.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:531
 
-  // Handle vnot the same as the vnot.mm pseudoinstruction.
-  def : Pat<(mti.Mask (vnot VR:$rs)),
+  // Handle rvv_vnot the same as the vnot.mm pseudoinstruction.
+  def : Pat<(mti.Mask (rvv_vnot VR:$rs)),
----------------
rogfer01 wrote:
> Now that you're here: I'm not sure there is a `vnot.mm` (I fail to find it in the spec) I think this comment should have said `vmnot.m`, right?
Good question. I wonder if it means the `vnot.v` pseudoinstruction which I see in the 0.10 spec.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101826/new/

https://reviews.llvm.org/D101826