TargetInstrInfo: add virtual function GetInstSizeInBytes

[SjoerdMeijer]

Summary

This adds virtual function GetInstSizeInBytes to TargetInstrInfo that a lot of subclasses already implement.

[arsenm]

include/llvm/Target/TargetInstrInfo.h

255 ↗

(On Diff #65795)

This should start with a lowercase letter. It should also have a default implementation of llvm_unreachable since I don't think guessing 4 is a reasonable default

[jmolloy]

This needs to have a conservatively correct return value or an 'unimplemented' sentinel value. 4 is not a correct assumption to make.

Cheers,

James

[SjoerdMeijer]

Thanks for reviewing.
I have added llvm_unreachable as the default implementation instead of 4.
Can we keep the function name that starts with a capital letter because that's the function that 6 targets already implement?

[arsenm]

In D22885#498210, @SjoerdMeijer wrote:

Thanks for reviewing.
I have added llvm_unreachable as the default implementation instead of 4.
Can we keep the function name that starts with a capital letter because that's the function that 6 targets already implement?

You have to touch all of them anyway to add the overrides, so might as well fix the name while you're at it

[SjoerdMeijer]

Ok, thanks, will do. And I actually need to double check if the "unreachable" solves my problem, or if I need to come up with a better default int value.

[SjoerdMeijer]

I have 1) changed the default value to ~0U, 2) renamed the function name to use a lowercase letter, and 3) added "override" to the different subclasses

[jmolloy]

include/llvm/Target/TargetInstrInfo.h

253 ↗

(On Diff #65892)

You must document the ~0U sentinel and what it means. Also mention that the unit is "bytes".

[SjoerdMeijer]

Thanks, that has been fixed.

[SjoerdMeijer]

The renaming is in a separate commit: https://reviews.llvm.org/D22925.
When that is accepted/committed, I will continue here with the functional change.

[SjoerdMeijer]

This now only implements the functional change to add getInstSizeInBytes to TargetInstrInfo.

[arsenm]

AMDGPU needs an override added in SIInstrInfo.h. LGTM with that fixed

[SjoerdMeijer]

I have fixed that and committed. Thanks!

llvm/trunk/include/llvm/Target/TargetInstrInfo.h

public:
   /// subregisters has no single offset.
   ///
   /// Targets with nontrivial bigendian implementations may need to override
   /// this, particularly to support spilled vector registers.
   virtual bool getStackSlotRange(const TargetRegisterClass *RC, unsigned SubIdx,
                                  unsigned &Size, unsigned &Offset,
                                  const MachineFunction &MF) const;
 
+  /// Returns the size in bytes of the specified MachineInstr, or ~0U
+  /// when this function is not implemented by a target.
+  virtual unsigned getInstSizeInBytes(const MachineInstr &MI) const {
+    return ~0U;
+  }
+
   /// Return true if the instruction is as cheap as a move instruction.
   ///
   /// Targets for different archs need to override this, and different
   /// micro-architectures can also be finely tuned inside.
   virtual bool isAsCheapAsAMove(const MachineInstr &MI) const {
     return MI.isAsCheapAsAMove();
   }
 

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h

 public:
   explicit AArch64InstrInfo(const AArch64Subtarget &STI);
 
   /// getRegisterInfo - TargetInstrInfo is a superset of MRegister info.  As
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).
   const AArch64RegisterInfo &getRegisterInfo() const { return RI; }
 
-  unsigned getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   bool isAsCheapAsAMove(const MachineInstr &MI) const override;
 
   bool isCoalescableExtInstr(const MachineInstr &MI, unsigned &SrcReg,
                              unsigned &DstReg, unsigned &SubIdx) const override;
 
   bool
   areMemAccessesTriviallyDisjoint(MachineInstr &MIa, MachineInstr &MIb,

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h

public:
   bool isHighLatencyInstruction(const MachineInstr &MI) const;
 
   /// \brief Return the descriptor of the target-specific machine instruction
   /// that corresponds to the specified pseudo or native opcode.
   const MCInstrDesc &getMCOpcodeFromPseudo(unsigned Opcode) const {
     return get(pseudoToMCOpcode(Opcode));
   }
 
-  unsigned getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   ArrayRef<std::pair<int, const char *>>
   getSerializableTargetIndices() const override;
 
   ScheduleHazardRecognizer *
   CreateTargetPostRAHazardRecognizer(const InstrItineraryData *II,
                                  const ScheduleDAG *DAG) const override;
 

llvm/trunk/lib/Target/AVR/AVRInstrInfo.h

 class AVRInstrInfo : public AVRGenInstrInfo {
 public:
   explicit AVRInstrInfo();
 
   const AVRRegisterInfo &getRegisterInfo() const { return RI; }
   const MCInstrDesc &getBrCond(AVRCC::CondCodes CC) const;
   AVRCC::CondCodes getCondFromBranchOpc(unsigned Opc) const;
   AVRCC::CondCodes getOppositeCondition(AVRCC::CondCodes CC) const;
-  unsigned getInstSizeInBytes(const MachineInstr *MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr *MI) const override;
 
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
                    const DebugLoc &DL, unsigned DestReg, unsigned SrcReg,
                    bool KillSrc) const override;
   void storeRegToStackSlot(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MI, unsigned SrcReg,
                            bool isKill, int FrameIndex,
                            const TargetRegisterClass *RC,

llvm/trunk/lib/Target/MSP430/MSP430InstrInfo.h

void storeRegToStackSlot(MachineBasicBlock &MBB,
                            const TargetRegisterClass *RC,
                            const TargetRegisterInfo *TRI) const override;
   void loadRegFromStackSlot(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MI,
                             unsigned DestReg, int FrameIdx,
                             const TargetRegisterClass *RC,
                             const TargetRegisterInfo *TRI) const override;
 
-  unsigned getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   // Branch folding goodness
   bool
   ReverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const override;
   bool isUnpredicatedTerminator(const MachineInstr &MI) const override;
   bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
                      MachineBasicBlock *&FBB,
                      SmallVectorImpl<MachineOperand> &Cond,

llvm/trunk/lib/Target/Mips/MipsInstrInfo.h

public:
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).
   ///
   virtual const MipsRegisterInfo &getRegisterInfo() const = 0;
 
   virtual unsigned getOppositeBranchOpc(unsigned Opc) const = 0;
 
   /// Return the number of bytes of code the specified instruction may be.
-  unsigned getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   void storeRegToStackSlot(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI,
                            unsigned SrcReg, bool isKill, int FrameIndex,
                            const TargetRegisterClass *RC,
                            const TargetRegisterInfo *TRI) const override {
     storeRegToStack(MBB, MBBI, SrcReg, isKill, FrameIndex, RC, TRI, 0);
   }

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.h

public:
 
   bool optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
                             unsigned SrcReg2, int Mask, int Value,
                             const MachineRegisterInfo *MRI) const override;
 
   /// GetInstSize - Return the number of bytes of code the specified
   /// instruction may be.  This returns the maximum number of bytes.
   ///
-  unsigned getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   void getNoopForMachoTarget(MCInst &NopInst) const override;
 
   std::pair<unsigned, unsigned>
   decomposeMachineOperandsTargetFlags(unsigned TF) const override;
 
   ArrayRef<std::pair<unsigned, const char *>>
   getSerializableDirectMachineOperandTargetFlags() const override;

llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.h

public:
   bool expandPostRAPseudo(MachineInstr &MBBI) const override;
   bool ReverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const
     override;
 
   // Return the SystemZRegisterInfo, which this class owns.
   const SystemZRegisterInfo &getRegisterInfo() const { return RI; }
 
   // Return the size in bytes of MI.
-  uint64_t getInstSizeInBytes(const MachineInstr &MI) const;
+  unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   // Return true if MI is a conditional or unconditional branch.
   // When returning true, set Cond to the mask of condition-code
   // values on which the instruction will branch, and set Target
   // to the operand that contains the branch target.  This target
   // can be a register or a basic block.
   SystemZII::Branch getBranchInfo(const MachineInstr &MI) const;
 

llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.cpp

case TargetOpcode::LOAD_STACK_GUARD:
     expandLoadStackGuard(&MI);
     return true;
 
   default:
     return false;
   }
 }
 
-uint64_t SystemZInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
+unsigned SystemZInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   if (MI.getOpcode() == TargetOpcode::INLINEASM) {
     const MachineFunction *MF = MI.getParent()->getParent();
     const char *AsmStr = MI.getOperand(0).getSymbolName();
     return getInlineAsmLength(AsmStr, *MF->getTarget().getMCAsmInfo());
   }
   return MI.getDesc().getSize();
 }