[CodeGen][NFC] Remove unused virtual function

[myhsu]

Summary

Context: https://reviews.llvm.org/D101588

TargetFrameLowering::emitCalleeSavedFrameMoves with 4 arguments is not used anywhere in CodeGen. Thus it shouldn't be exposed as a virtual function. NFC.

[dblaikie]

llvm/lib/Target/M68k/M68kFrameLowering.cpp

659

Is this a change in behavior? Or is the 3 arg version just the 4 arg version with 'true' as the 4th arg?

[myhsu]

llvm/lib/Target/M68k/M68kFrameLowering.cpp

659

This doesn't change any behavior. This function has always been 3 arguments until D101588, in which we added the 4-th argument only to conform with the virtual function prototype we're removing here. But the added 4-th argument has never been used at all in the function body so removing it is safe and doesn't change any behavior.

[dblaikie]

llvm/lib/Target/M68k/M68kFrameLowering.cpp

659

Hmm, I'm a bit confused then - so this M68KFrameLowering has a function "emitCalleeSavedFrameMoves" that takes 3 arguments and the base class has one that's a no-op and only takes two arguments & isn't overriden by the M68KFrameLowering backend. Is that right? That seems somewhat problematic - does that mean generic code that's calling the virtual function isn't getting the desired implementation? Or is the 2 argument version really intended to be a no-op for M68KFrameLowering? Despite there being a 3 arg version that does do some work?

Any chance there's a reasonable other name for the function, if it's not related to the virtual one?

The X86 one makes sense to me, for instance - in that the 2 arg virtual one is implemented in terms of the 4 arg (now non-virtual) one. But the M68k one isn't quite making sense to me that there's a 3 arg function with the same name that does work - despite the 2-arg one still being the default implementation that does nothing?

[myhsu]

llvm/lib/Target/M68k/M68kFrameLowering.cpp

659

That seems somewhat problematic - does that mean generic code that's calling the virtual function isn't getting the desired implementation? Or is the 2 argument version really intended to be a no-op for M68KFrameLowering? Despite there being a 3 arg version that does do some work?

I agree with this, I'll rename the current emitCalleeSavedFrameMoves into some other name

[myhsu]

Rename M68kFrameLowering::emitCalleeSavedFrameMoves with 3 args into another name to avoid confusion

[dblaikie]

Thanks!

(@tmsriram - perhaps you could rename the X86 version of this function too in a separate patch, to avoid overloading a virtual/reduce confusion here? (I think this function was added for BB sections & was initially virtual in the base FrameLowering API - but has now been sunk into the X86 backend/etc)

llvm/include/llvm/CodeGen/TargetFrameLowering.h

virtual void emitEpilogue(MachineFunction &MF,
                             MachineBasicBlock &MBB) const = 0;
 
   /// With basic block sections, emit callee saved frame moves for basic blocks
   /// that are in a different section.
   virtual void
   emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI) const {}
 
-  virtual void emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
-                                         MachineBasicBlock::iterator MBBI,
-                                         const DebugLoc &DL,
-                                         bool IsPrologue) const {}
-
   /// Replace a StackProbe stub (if any) with the actual probe code inline
   virtual void inlineStackProbe(MachineFunction &MF,
                                 MachineBasicBlock &PrologueMBB) const {}
 
   /// Adjust the prologue to have the function use segmented stacks. This works
   /// by adding a check even before the "normal" function prologue.
   virtual void adjustForSegmentedStacks(MachineFunction &MF,
                                         MachineBasicBlock &PrologueMBB) const {}

llvm/lib/Target/M68k/M68kFrameLowering.h

class M68kFrameLowering : public TargetFrameLowering {
   void BuildStackAlignAND(MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator MBBI, const DebugLoc &DL,
                           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,
+  void emitPrologueCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                                          MachineBasicBlock::iterator MBBI,
-                                 const DebugLoc &DL,
-                                 bool IsProlog) const override;
+                                         const DebugLoc &DL) const;
 
   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,
                                  const DebugLoc &DL,
                                  const MCCFIInstruction &CFIInst) const {
   MachineFunction &MF = *MBB.getParent();
   unsigned CFIIndex = MF.addFrameInst(CFIInst);
   BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
       .addCFIIndex(CFIIndex);
 }
 
-void M68kFrameLowering::emitCalleeSavedFrameMoves(
+void M68kFrameLowering::emitPrologueCalleeSavedFrameMoves(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
-    const DebugLoc &DL, bool IsProlog) const {
+    const DebugLoc &DL) 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, true);
+      emitPrologueCalleeSavedFrameMoves(MBB, MBBI, DL);
   }
 
   // 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/X86/X86FrameLowering.h

void inlineStackProbe(MachineFunction &MF,
                         MachineBasicBlock &PrologMBB) const override;
 
   void
   emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI) const override;
 
   void emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MBBI,
-                                 const DebugLoc &DL,
-                                 bool IsPrologue) const override;
+                                 const DebugLoc &DL, bool IsPrologue) const;
 
   /// emitProlog/emitEpilog - These methods insert prolog and epilog code into
   /// the function.
   void emitPrologue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
   void emitEpilogue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
 
   void adjustForSegmentedStacks(MachineFunction &MF,
                                 MachineBasicBlock &PrologueMBB) const override;