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

[Derek Schuff via llvm-commits]

Personally I find the approach of setting function pointers that do either
something or nothing less straightforward and understandable than just
checking a boolean variable or target hook. (That's why my initial attempts
at this patch did exactly that). Obviously if there is just a boolean
variable or target hook, then it's more readable if the uses of it are
fewer and at a higher level (instead of scattered everywhere in the code).
Making the check happen at pass instantiation time as is done here kind of
forces things in that direction. Now that I think about it though, I
actually think that the more uses there need to be, the better it is just
to have some boolean (since you would just end up having a lot of function
pointers); as Justin pointed out, I still failed to get everything into one
place because of the check in `getRequiredProperties()`. Also the
difference between traditional and virtual-reg architectures still leaks
through via the use of MinCSFrameIndex and MaxCSFrameIndex in
calculateFrameObjectOffsets(). So I guess that's just a long-winded way of
repeating that I agree with Justin. I also don't know of anywhere else in
LLVM where we use this approach; generally we just put a check against the
target hook in a conditional.

The original concern and goal of all of this was that this code never had
to consider virtual registers before, and we wanted to more clearly
separate code that now has to deal with a virtual targets from code which
doesn't. In this case that's the CSR spilling code and the register
scavenging code, which are now factored into functions. Maybe it would
accomplish the same purpose and be simpler just to put something like
`if(TM->usesPhysRegsForPEI()) return;` with an explanatory comment at the
top of those functions?

On Mon, May 23, 2016 at 2:58 PM Quentin Colombet <qcolombet at apple.com>
wrote:

>
> > 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160527/4813faa7/attachment.html>