[M68k] fix -Wdefaulted-function-deleted and -Woverloaded-virtual

[nickdesaulniers]

Summary

Fixes the following warnings observerd when building the experimental
m68k backend (-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="M68k"):

../lib/Target/M68k/M68kMachineFunction.h:71:3: warning: explicitly
defaulted default constructor is implicitly deleted
[-Wdefaulted-function-deleted]

M68kMachineFunctionInfo() = default;
^

../lib/Target/M68k/M68kMachineFunction.h:24:20: note: default
constructor of 'M68kMachineFunctionInfo' is implicitly deleted because
field 'MF' of reference type 'llvm::MachineFunction &' would not be
initialized

MachineFunction &MF;
                 ^

In file included from ../lib/Target/M68k/M68kISelLowering.cpp:18:
In file included from ../lib/Target/M68k/M68kSubtarget.h:17:
../lib/Target/M68k/M68kFrameLowering.h:60:8: warning:
'llvm::M68kFrameLowering::emitCalleeSavedFrameMoves' hides overloaded
virtual functions [-Woverloaded-virtual]

void emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
     ^

../include/llvm/CodeGen/TargetFrameLowering.h:215:3: note: hidden
overloaded virtual function
'llvm::TargetFrameLowering::emitCalleeSavedFrameMoves' declared here:
different number of parameters (2 vs 3)

emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
^

../include/llvm/CodeGen/TargetFrameLowering.h:218:16: note: hidden
overloaded virtual function
'llvm::TargetFrameLowering::emitCalleeSavedFrameMoves' declared here:
different number of parameters (4 vs 3)

virtual void emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
             ^

pr/50071

[myhsu]

LGTM, Thank you!

[dblaikie]

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

[dblaikie]

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

[myhsu]

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

Sorry I missed your previous comment. I will try to add a test accordingly.

[myhsu]

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

This might not be the right place to ask, but I accidentally found that no one outside lib/Target is actually using emitCalleeSavedFrameMoves with 4 arguments (please correct me if I'm wrong. I only found one call site of emitCalleeSavedFrameMoves with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
A spin-off question might be: Why was emitCalleeSavedFrameMoves with 4 arguments got exposed as virtual function at the first place if no one is using it?

[dblaikie]

In D101588#2767764, @myhsu wrote:

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

This might not be the right place to ask, but I accidentally found that no one outside lib/Target is actually using emitCalleeSavedFrameMoves with 4 arguments (please correct me if I'm wrong. I only found one call site of emitCalleeSavedFrameMoves with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
A spin-off question might be: Why was emitCalleeSavedFrameMoves with 4 arguments got exposed as virtual function at the first place if no one is using it?

Could you check the version control history of the function? Perhaps it was added for some use at some point, but that use is no longer needed and it could now be removed?

[myhsu]

In D101588#2769214, @dblaikie wrote:

In D101588#2767764, @myhsu wrote:

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

This might not be the right place to ask, but I accidentally found that no one outside lib/Target is actually using emitCalleeSavedFrameMoves with 4 arguments (please correct me if I'm wrong. I only found one call site of emitCalleeSavedFrameMoves with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
A spin-off question might be: Why was emitCalleeSavedFrameMoves with 4 arguments got exposed as virtual function at the first place if no one is using it?

Could you check the version control history of the function? Perhaps it was added for some use at some point, but that use is no longer needed and it could now be removed?

It was first introduced in https://reviews.llvm.org/D79978.
emitCalleeSavedFrameMoves with 4 arguments was not used outside lib/Target in that particular patch and I doubt anyone (lib/CodeGen) has used it ever since then.
I can send a patch regarding this matter if that's appropriate.

[dblaikie]

In D101588#2771414, @myhsu wrote:

In D101588#2769214, @dblaikie wrote:

In D101588#2767764, @myhsu wrote:

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

This might not be the right place to ask, but I accidentally found that no one outside lib/Target is actually using emitCalleeSavedFrameMoves with 4 arguments (please correct me if I'm wrong. I only found one call site of emitCalleeSavedFrameMoves with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
A spin-off question might be: Why was emitCalleeSavedFrameMoves with 4 arguments got exposed as virtual function at the first place if no one is using it?

Could you check the version control history of the function? Perhaps it was added for some use at some point, but that use is no longer needed and it could now be removed?

It was first introduced in https://reviews.llvm.org/D79978.

Ah, thanks for the pointer!

emitCalleeSavedFrameMoves with 4 arguments was not used outside lib/Target in that particular patch and I doubt anyone (lib/CodeGen) has used it ever since then.
I can send a patch regarding this matter if that's appropriate.

Yep, sounds good to me. Please do!

[myhsu]

In D101588#2771467, @dblaikie wrote:

In D101588#2771414, @myhsu wrote:

In D101588#2769214, @dblaikie wrote:

In D101588#2767764, @myhsu wrote:

In D101588#2764656, @dblaikie wrote:

In D101588#2730131, @dblaikie wrote:

@myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?

@myhsu Ping on this question ^

This might not be the right place to ask, but I accidentally found that no one outside lib/Target is actually using emitCalleeSavedFrameMoves with 4 arguments (please correct me if I'm wrong. I only found one call site of emitCalleeSavedFrameMoves with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
A spin-off question might be: Why was emitCalleeSavedFrameMoves with 4 arguments got exposed as virtual function at the first place if no one is using it?

Could you check the version control history of the function? Perhaps it was added for some use at some point, but that use is no longer needed and it could now be removed?

It was first introduced in https://reviews.llvm.org/D79978.

Ah, thanks for the pointer!

emitCalleeSavedFrameMoves with 4 arguments was not used outside lib/Target in that particular patch and I doubt anyone (lib/CodeGen) has used it ever since then.
I can send a patch regarding this matter if that's appropriate.

Yep, sounds good to me. Please do!

Sorry for the delay, I'd just submitted https://reviews.llvm.org/D103328

llvm/lib/Target/M68k/M68kFrameLowering.h

void BuildStackAlignAND(MachineBasicBlock &MBB,
                           unsigned Reg, uint64_t MaxAlign) const;
 
   /// Wraps up getting a CFI index and building a MachineInstr for it.
   void BuildCFI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                 const DebugLoc &DL, const MCCFIInstruction &CFIInst) const;
 
   void emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MBBI,
-                                 const DebugLoc &DL) const;
+                                 const DebugLoc &DL,
+                                 bool IsProlog) const override;
 
   unsigned getPSPSlotOffsetFromSP(const MachineFunction &MF) const;
 
 public:
   explicit M68kFrameLowering(const M68kSubtarget &sti, Align Alignment);
 
   static const M68kFrameLowering *create(const M68kSubtarget &ST);
 

llvm/lib/Target/M68k/M68kFrameLowering.cpp

void M68kFrameLowering::BuildCFI(MachineBasicBlock &MBB,
   MachineFunction &MF = *MBB.getParent();
   unsigned CFIIndex = MF.addFrameInst(CFIInst);
   BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
       .addCFIIndex(CFIIndex);
 }
 
 void M68kFrameLowering::emitCalleeSavedFrameMoves(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
-    const DebugLoc &DL) const {
+    const DebugLoc &DL, bool IsProlog) const {
   MachineFunction &MF = *MBB.getParent();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   MachineModuleInfo &MMI = MF.getMMI();
   const MCRegisterInfo *MRI = MMI.getContext().getRegisterInfo();
 
   // Add callee saved registers to move list.
   const auto &CSI = MFI.getCalleeSavedInfo();
   if (CSI.empty())
if (!HasFP && NumBytes) {
       assert(StackSize);
       BuildCFI(
           MBB, MBBI, DL,
           MCCFIInstruction::cfiDefCfaOffset(nullptr, -StackSize + stackGrowth));
     }
 
     // Emit DWARF info specifying the offsets of the callee-saved registers.
     if (PushedRegs)
-      emitCalleeSavedFrameMoves(MBB, MBBI, DL);
+      emitCalleeSavedFrameMoves(MBB, MBBI, DL, true);
   }
 
   // TODO Interrupt handlers
   // M68k Interrupt handling function cannot assume anything about the
   // direction flag (DF in CCR register). Clear this flag by creating "cld"
   // instruction in each prologue of interrupt handler function. The "cld"
   // instruction should only in these cases:
   // 1. The interrupt handling function uses any of the "rep" instructions.

llvm/lib/Target/M68k/M68kMachineFunction.h

class M68kMachineFunctionInfo : public MachineFunctionInfo {
   /// musttail call.
   SmallVector<ForwardedRegister, 1> ForwardedMustTailRegParms;
 
   /// The number of bytes on stack consumed by the arguments being passed on
   /// the stack.
   unsigned ArgumentStackSize = 0;
 
 public:
-  M68kMachineFunctionInfo() = default;
   explicit M68kMachineFunctionInfo(MachineFunction &MF) : MF(MF) {}
 
   bool getRestoreBasePointer() const { return RestoreBasePointerOffset != 0; }
   void setRestoreBasePointer(const MachineFunction *MF);
   int getRestoreBasePointerOffset() const { return RestoreBasePointerOffset; }
 
   unsigned getCalleeSavedFrameSize() const { return CalleeSavedFrameSize; }
   void setCalleeSavedFrameSize(unsigned bytes) { CalleeSavedFrameSize = bytes; }