[RISCV] move `isFaultFirstLoad` into `RISCVInstrInfo`

[sunshaoce]

Summary

Fix build errors in D126794

ld.lld: error: undefined symbol: llvm::MachineInstr::getNumExplicitDefs() const
>>> referenced by RISCVBaseInfo.cpp
>>>               RISCVBaseInfo.cpp.o:(llvm::isFaultFirstLoad(llvm::MachineInstr const&)) in archive lib/libLLVMRISCVDesc.a

ld.lld: error: undefined symbol: llvm::MachineInstr::findRegisterDefOperandIdx(llvm::Register, bool, bool, llvm::TargetRegisterInfo const*) const
>>> referenced by RISCVBaseInfo.cpp
>>>               RISCVBaseInfo.cpp.o:(llvm::isFaultFirstLoad(llvm::MachineInstr const&)) in archive lib/libLLVMRISCVDesc.a
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

[fakepaper56]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

436

Maybe we could add inline static in a header file, like here?

[fakepaper56]

I am sorry to create the trouble, but I didn't encounter this problem in my local. Could you provide me your cmake command?
My cmake command:

cmake -G "Ninja" -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_BUILD_EXAMPLES=False -DLLVM_PARALLEL_LINK_JOBS=2 -DCMAKE_BUILD_TYPE=Debug ../llvm

[fakepaper56]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

436

I mean you could rewrite to in a header file.

inline static bool isFaultFirstLoad(const MachineInstr &MI) {
  return MI.getNumExplicitDefs() == 2 && MI.modifiesRegister(RISCV::VL) &&
       !MI.isInlineAsm();
}

[sunshaoce]

In D127477#3572700, @fakepaper56 wrote:

I am sorry to create the trouble, but I didn't encounter this problem in my local. Could you provide me your cmake command?
My cmake command:

cmake -G "Ninja" -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_BUILD_EXAMPLES=False -DLLVM_PARALLEL_LINK_JOBS=2 -DCMAKE_BUILD_TYPE=Debug ../llvm

cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_USE_SPLIT_DWARF=ON -DLLVM_TARGETS_TO_BUILD="X86;RISCV" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=lld ../llvm
ninja clang-tidy

[sunshaoce]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

436

It works. Should we move isFaultFirstLoad into a LLVM::RISCV* namespace?

[fakepaper56]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

436

Should we move isFaultFirstLoad into a LLVM::RISCV* namespace?

I think the idea is great.

[fakepaper56]

This comment was removed by fakepaper56.

[fakepaper56]

Thank you for providing your cmake command.

[sunshaoce]

Address comments.

[fakepaper56]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

435

RISCVVInstInfo is similar as RISCVInstInfo. Maybe you could write it as static function in RISCVInstInfo. Sorry I ignoring it.

[fakepaper56]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

435

Fix typo. Suggest declare as a static function of class RISCVInstrInfo.

[fakepaper56]

LGTM. It's fine to me that not declare isFaultFirstLoad as a static function of class RISCVInstrInfo, although I prefer this method.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

18–19

CodeGen header should not be included in this file. This is an MC layer file.

435

This file should not refererence MachineInstr.

[sunshaoce]

Reopen

[reames]

Please revert both this and the original change. There are unresolved concerns, and usual practice is to revert, and then reintroduce a revised patch - possibly after review if needed.

[sunshaoce]

Address @reames's comment.

[craig.topper]

LGTM

[fakepaper56]

I am sorry that giving wrong comments made @sunshaoce rewrite and commit the code repeatedly. I promise I will review code more carefully and be more familiar with llvm.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

 // useful for the compiler back-end and the MC libraries.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
-#include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Support/RISCVISAInfo.h"
 
 namespace llvm {
 
 // RISCVII - This namespace holds all of the target specific flags that
 // instruction info tracks. All definitions must match RISCVInstrFormats.td.
 namespace RISCVII {

 inline static bool isTailAgnostic(unsigned VType) { return VType & 0x40; }
 
 inline static bool isMaskAgnostic(unsigned VType) { return VType & 0x80; }
 
 void printVType(unsigned VType, raw_ostream &OS);
 
 } // namespace RISCVVType
 
-bool isFaultFirstLoad(const MachineInstr &MI);
 } // namespace llvm
 
 #endif

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp

else
     OS << ", tu";
 
   if (isMaskAgnostic(VType))
     OS << ", ma";
   else
     OS << ", mu";
 }
 
-bool isFaultFirstLoad(const MachineInstr &MI) {
-  return MI.getNumExplicitDefs() == 2 && MI.modifiesRegister(RISCV::VL) &&
-         !MI.isInlineAsm();
-}
-
 } // namespace llvm

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

if (VRegDef != RISCV::X0 &&
       UsedVL = true;
   }
 
   for (auto *MI : ToDelete)
     MI->eraseFromParent();
 }
 
 void RISCVInsertVSETVLI::insertReadVL(MachineBasicBlock &MBB) {
+  const MachineFunction *MF = MBB.getParent();
+  const RISCVInstrInfo *TII = MF->getSubtarget<RISCVSubtarget>().getInstrInfo();
+
   for (auto I = MBB.begin(), E = MBB.end(); I != E;) {
     MachineInstr &MI = *I++;
-    if (isFaultFirstLoad(MI)) {
+    if (TII->isFaultFirstLoad(MI)) {
       Register VLOutput = MI.getOperand(1).getReg();
       if (!MRI->use_nodbg_empty(VLOutput))
         BuildMI(MBB, I, MI.getDebugLoc(), TII->get(RISCV::PseudoReadVL),
                 VLOutput);
       // We don't use the vl output of the VLEFF/VLSEGFF anymore.
       MI.getOperand(1).setReg(RISCV::X0);
     }
   }

llvm/lib/Target/RISCV/RISCVInstrInfo.h

public:
   // Returns true if the given MI is an RVV instruction opcode for which we may
   // expect to see a FrameIndex operand. When CheckFIs is true, the instruction
   // must contain at least one FrameIndex operand.
   bool isRVVSpill(const MachineInstr &MI, bool CheckFIs) const;
 
   Optional<std::pair<unsigned, unsigned>>
   isRVVSpillForZvlsseg(unsigned Opcode) const;
 
+  bool isFaultFirstLoad(const MachineInstr &MI) const;
+
 protected:
   const RISCVSubtarget &STI;
 };
 
 namespace RISCV {
 
 // Implemented in RISCVGenInstrInfo.inc
 int16_t getNamedOperandIdx(uint16_t Opcode, uint16_t NamedIndex);

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

RISCVInstrInfo::isRVVSpillForZvlsseg(unsigned Opcode) const {
   case RISCV::PseudoVSPILL7_M1:
   case RISCV::PseudoVRELOAD7_M1:
     return std::make_pair(7u, 1u);
   case RISCV::PseudoVSPILL8_M1:
   case RISCV::PseudoVRELOAD8_M1:
     return std::make_pair(8u, 1u);
   }
 }
+
+bool RISCVInstrInfo::isFaultFirstLoad(const MachineInstr &MI) const {
+  return MI.getNumExplicitDefs() == 2 && MI.modifiesRegister(RISCV::VL) &&
+         !MI.isInlineAsm();
+}

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

static bool lowerRISCVVMachineInstrToMCInst(const MachineInstr *MI,
 
   const MachineBasicBlock *MBB = MI->getParent();
   assert(MBB && "MI expected to be in a basic block");
   const MachineFunction *MF = MBB->getParent();
   assert(MF && "MBB expected to be in a machine function");
 
   const TargetRegisterInfo *TRI =
       MF->getSubtarget<RISCVSubtarget>().getRegisterInfo();
+  const RISCVInstrInfo *TII = MF->getSubtarget<RISCVSubtarget>().getInstrInfo();
+
   assert(TRI && "TargetRegisterInfo expected");
 
   uint64_t TSFlags = MI->getDesc().TSFlags;
   unsigned NumOps = MI->getNumExplicitOperands();
 
   // Skip policy, VL and SEW operands which are the last operands if present.
   if (RISCVII::hasVecPolicyOp(TSFlags))
     --NumOps;
   if (RISCVII::hasVLOp(TSFlags))
     --NumOps;
   if (RISCVII::hasSEWOp(TSFlags))
     --NumOps;
 
-  bool hasVLOutput = isFaultFirstLoad(*MI);
+  bool hasVLOutput = TII->isFaultFirstLoad(*MI);
   for (unsigned OpNo = 0; OpNo != NumOps; ++OpNo) {
     const MachineOperand &MO = MI->getOperand(OpNo);
     // Skip vl ouput. It should be the second output.
     if (hasVLOutput && OpNo == 1)
       continue;
 
     // Skip merge op. It should be the first operand after the result.
     if (RISCVII::hasMergeOp(TSFlags) && OpNo == 1U + hasVLOutput) {