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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:659
     if (PushedRegs)
-      emitCalleeSavedFrameMoves(MBB, MBBI, DL, true);
+      emitCalleeSavedFrameMoves(MBB, MBBI, DL);
   }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103328