[PATCH] D126794: [RISCV] Lower VLEFF/VLSEGFF SDNodes to MachineInstrs with VL outputs.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In general, I like this patch more than the alternative.  However, I also have to acknowledge that this is mostly just shifting complexity from one place to another.  I'm curious to know if others think this is the right tradeoff.

Once we've settled on the approach, I think a few bits of this can be separated out into NFCs to simplify the review, but let's leave that for the moment.  What's here is a pretty good view of the overall impact.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:176
+/// \returns true if there is a VL output for the instruction.
+static inline bool hasVLOutput(uint64_t TSFlags) {
+  return TSFlags & HasVLOutputMask;
----------------
Not sure how much value we have here from having the generic bit field for this over a static method which switches on the opcodes.  This really only applies to the FF variants.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1438
+      MachineInstrBuilder MIB =
+          BuildMI(MBB, I, DL, TII->get(NewOpc), MI.getOperand(0).getReg());
+      unsigned Cur = 2;
----------------
Did you play with what happens if you just set the second destination register to VL?

If that works, then we'd avoid the need for the second set of pseudos.  Not sure it does, but maybe?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1439
+          BuildMI(MBB, I, DL, TII->get(NewOpc), MI.getOperand(0).getReg());
+      unsigned Cur = 2;
+
----------------
It looks like this block of code is just copying all the operands right?  If so, simpler to write it that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126794