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

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

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!

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 of the specified MachineInstr. 
+  virtual unsigned GetInstSizeInBytes(const MachineInstr &MI) const {
+    return 4;
+  }
+
   /// 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();
   }