[llvm] r269750 - Factor PrologEpilogInserter around spilling, frame finalization, and scavenging

[Justin Bogner via llvm-commits]

Quentin Colombet <qcolombet at apple.com> writes:
> Hi Justin,
>
> On May 23, 2016, at 1:25 PM, Justin Bogner via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> Derek Schuff via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> writes:
>>> -  PEI() : MachineFunctionPass(ID) {
>>> + explicit PEI(const TargetMachine *TM = nullptr) :
> MachineFunctionPass(ID) {
>>>     initializePEIPass(*PassRegistry::getPassRegistry());
>>> +
>>> +    if (TM && (!TM->usesPhysRegsForPEI())) {
>>> +      SpillCalleeSavedRegisters = [](MachineFunction &, RegScavenger *,
>>> + unsigned &, unsigned &, const MBBVector &,
>>> +                                     const MBBVector &) {};
>>> +      ScavengeFrameVirtualRegs = [](MachineFunction &, RegScavenger *) {};
>>> +    } else {
>>> +      SpillCalleeSavedRegisters = doSpillCalleeSavedRegs;
>>> +      ScavengeFrameVirtualRegs = doScavengeFrameVirtualRegs;
>>> +      UsesCalleeSaves = true;
>>> +    }
>> 
>> This is a pretty strange and confusing way to do this. Wouldn't it be
>> way simpler to just set a bool (I'd call it UsesPhysReg) and check it
>> before the single call to each of these functions? You'd presumably also
>> be able to use the same bool in the one place we check UsesCalleeSaves
>> as well.
>
> A comment on that, because this situation is my doing :).

Personally I think the more direct style would be better, but the
difference isn't all that big so I'll just explain my rationale and move
on. You or Derek can change it if you're convinced by my argument.

> The difference is performance and maintainability.
> Performance: We check what to call only once (at build time) against
> every time we process a function.

Is the performance impact of this measurable? I would expect the branch
to be predicted very well, since it never changes, so I doubt this
really matters. There's also the fact that this changes a bunch of
direct calls to indirect calls, so we've balanced out saving a check
with making the call impossible to inline.

> Maintainability: All the code that is modified because of that check
> is in one place.

Well, sort of. Except it basically splits this into three variables
instead of one - it's easy to grep for "UsesPhysReg" and find everything
that the bool modifies and update accordingly. It's harder to grep for
two functions and a bool to see where this makes a difference. There's
also the small matter that we set a bool based on this check *anyway*
and have to check that elsewhere, so we haven't even accomplished
getting all of the checks in the same place.

OTOH, there's a third element to the tradeoff here, which is
discoverability/clarity. If I read through runOnMachineFunction I see a
call to `SpillCalleeSavedRegisters`, so it looks like this always
happens. People don't generally expect a function to sometimes be empty
based on a runtime check somewhere else in the file.