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

[Yeting Kuo via Phabricator via llvm-commits]

fakepaper56 marked an inline comment as done.
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126794