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

[Quentin Colombet via llvm-commits]

Hi Derek,

> On May 27, 2016, at 9:59 AM, Derek Schuff <dschuff at google.com> wrote:
> 
> 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?

To be fair, I think the whole PEI and the related APIs (scavenger and such) would need a bigger revamp to properly adjust to the fact that PEI may occur on unallocated code. I still see the current approach as a hack and that is why I was trying to conceal it in one place.

That being said, I agree the current approach is indeed unnatural compared to other LLVM passes and given both Justin and Derek have raised concerns about that I am fine reverting my position on that patch.

The bottom line is (I am going to regret this :P), Derek, please go ahead with the usesPhysRegsForPEI approach instead of the function pointers.

Cheers,
-Quentin

> 
> On Mon, May 23, 2016 at 2:58 PM Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
> 
> > On May 23, 2016, at 2:15 PM, Justin Bogner <mail at justinbogner.com <mailto:mail at justinbogner.com>> wrote:
> >
> > Quentin Colombet <qcolombet at apple.com <mailto: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 <mailto: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> <mailto: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/20160531/826d4eba/attachment.html>