[PATCH] D93849: [RISCV] Define vmclr.m/vmset.m intrinsics.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Out of curiosity, what would the pros/cons be between instead expanding these early in the ISel custom inserter vs. late in the expand-pseudos pass as you've done? I would imagine that the `vmxor` and `vmxnor` instructions will have better integration with the rest of the compiler (scheduling info, for instance) so we'd prefer to have those around. Otherwise we might have to add special-cases for these pseudos to treat them like their underlying instructions.



================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:407
                     [IntrNoMem]>, RISCVVIntrinsic;
+  // Output: (mask type output)
+  // Input: (vl)
----------------
nit: the class is called `RISCVNullaryIntrinsic` producing `llvm_anyvector_ty` but says it outputs "mask type". Is that too specific? The other intrinsic classes which output "mask type" are called e.g. `RISCVMaskXXX`. Should this be named e.g. `RISCVMaskNullary(Intrinsic)`? I don't think we expect other nullaries, do we?


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:236
+                                          MachineBasicBlock::iterator MBBI,
+                                          unsigned OpCode) {
+  MachineInstr &MI = *MBBI;
----------------
`Opcode` would be a more familiar casing for this variable name.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:244
+  BuildMI(MBB, MBBI, DL, Desc)
+      .addReg(DstReg, RegState::Define)
+      .addReg(DstReg, RegState::Undef)
----------------
Is there a reason not to use `BuildMI(..., DstReg)` instead of this extra `.addReg`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93849