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

Fri Jun 3 03:32:07 PDT 2022

fakepaper56 added inline comments.

================
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;
----------------
reames wrote:
> 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.  
How about use `getNumExplicitDefs() == 2` to check whether a MI is VLEFF/VLSEGFF?

================
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;
----------------
craig.topper wrote:
> reames wrote:
> > 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?
> The register class wouldn't match, so I doubt it would work.
> 
> Might be able to use an OptionalDef though?
Just replacing `CurDAG->getRegister(RISCV::VL, Subtarget->getXLenVT())` with ReadVL causes ICE about legalization. I don't really understand OptionalDef yet, but I have some questions about the method. 

First,  the method seems to break the SSA form of VL and we need to know which modifies the VL right?
Second, Is possible that the use of VL be moved behind the VLEFF/VLSEGFF MI?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126794