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

[Philip Reames via Phabricator via llvm-commits]

reames 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;
----------------
fakepaper56 wrote:
> 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?
If it were hidden in a reasonable named helper function, maybe.  I'm concerned about how many other instructions might have two explicit defs, and in particular, how easy it would be to add an unrelated one without knowing this check existed.  


================
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;
----------------
fakepaper56 wrote:
> 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?
Two other options for you to consider.

Could we set the second def to X0?  Conceptually, X0 is a constant and thus write discarding register, so this would be a "correct" GPR def.

Could we set the second def to NoRegister?  We have precedent for this in a couple other places (e.g. mask reg).  

Basically, I'm looking for something we can sensibly set it to which allows it to be a nop after we expand the separate copy, and yet leaves us with one family of psuedos.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126794