[PATCH] D103328: [CodeGen][NFC] Remove unused virtual function

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:659
     if (PushedRegs)
-      emitCalleeSavedFrameMoves(MBB, MBBI, DL, true);
+      emitCalleeSavedFrameMoves(MBB, MBBI, DL);
   }
----------------
dblaikie wrote:
> myhsu wrote:
> > dblaikie wrote:
> > > Is this a change in behavior? Or is the 3 arg version just the 4 arg version with 'true' as the 4th arg?
> > 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.
> 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?
> 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103328/new/

https://reviews.llvm.org/D103328