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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This is looking pretty reasonable to me.  Another rev or two, and we're probably good to go.



================
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:
> fakepaper56 wrote:
> > reames wrote:
> > > 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.  
> > Yes, there is a bunch of MI has 2 explicit defines but not VLEFF/VLSEGFF. Executing `sed '/RISCVInsts\[\] =/,/^}/!d' <llvm-build-dir>/lib/Target/RISCV/RISCVGenInstrInfo.inc | awk 'index($4, 2)'` could see all the opcode having 2 explicit defines .
> > I am sorry not do the experiment first.
> I found we could use `hasSEWOp(TsFlags) && MI->mayLoad() && MI->getNumExplicitDefs() == 2` to replace the `hasVLOutput()`. Or maybe we just enumerate all the VLEFF/VLSEGFF instructions into a switch case like `isFaultFirstLoad()` in D126227.
The isFaultFirstLoad function from the other patch was what I'd originally suggested.  I would mildly prefer that, but your proposed last condition in a well named function would be fine too.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:695
 
+class VPseudoUSLoadFFNoMask<VReg RetClass, int EEW, bit DummyMask = 1> :
+      Pseudo<(outs RetClass:$rd, GPR:$vl),
----------------
Unless I'm missing something, the need for separate FF pseudos disappears once you remove the VLOperand flag doesn't it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126794