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

[Quentin Colombet via llvm-commits]

> On May 23, 2016, at 2:15 PM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> 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?

No idea.

> 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.

Well, bunch is 2 here :). Anyhow, there is nothing to inline in one case, but yeah the performance argument is probably weak.

> 
>> 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 is actually more than that. The reason why I wanted to have these changes all gathered at one point is because I do not want to spread the use of UsesPhysReg all around. IMHO, it confuses the reader than anything else to have ifs around. Indeed, the pass is about laying the prologue and epilogue, I.e., we are after register allocation, but we still have to do special cases for virtual registers?!


> 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.

This one is actually a fairly new mechanism and I actually missed it in the review. We should rethink how we could change that as well… An additional indirect call would do the job :P.
@Derek could you do that?
We shouldn’t have any check of UsePhysReg out of the constructor.

The thing I like with the current approach (modulo this one place) is that we are sure all places that need to operate differently are set once and for all for the whole process.

> 
> 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.

You should expect that. Only “weird" targets won’t do that. 

> People don't generally expect a function to sometimes be empty
> based on a runtime check somewhere else in the file.

That could actually be a static check if we bothered pushing different very of the pass for the “weird” target. What I am saying is that the “weird” targets know what they are doing and the others get what they expect and reading through the code does not involve the confusing boolean :).

Cheers,
-Quentin