This is an archive of the discontinued LLVM Phabricator instance.

Factor PrologEpilogInserter around spilling, frame finalization, and scavenging
ClosedPublic

Authored by dschuff on Mar 22 2016, 11:25 AM.

Details

Summary

PrologEpilogInserter has these 3 phases, which are related, but not
all of them are needed by all targets. This patch reorganizes PEI's
varous functions around those phases for more clear separation, and adds
more useful assertions based on which parts must support virtual
registers, callee-saved register, and scavenging.

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 51311.Mar 22 2016, 11:25 AM
dschuff retitled this revision from to Factor PrologEpilogInserter around spilling, frame finalization, and scavenging.
dschuff updated this object.
dschuff added reviewers: qcolombet, hfinkel.
dschuff added a subscriber: llvm-commits.
qcolombet edited edge metadata.Mar 23 2016, 10:14 AM

Hi Derek,

Thanks for keep pushing on that.

The HasVirtualRegister will not change for a given target, thus, I would rather have it checked only at the instantiation of the class.
In other words, instead of having two if blocks in runOnMachineFunction, I would suggest calling a helper object that would have been instantiated differently according to the mode.

E.g., instead of doing
for each function
if (HasVirtual) {

// handlingA

} else {

// handlingAbis

}

I would do:
Once for the Module:
if (HasVirtual) {
helper = helperA
} else {
helper = helperAbis // where helperAbis is a sub class of helperA and helperA does not nothing by default.
}

for each function

helper->callA

What do you think?

Cheers,
-Quentin

lib/CodeGen/PrologEpilogInserter.cpp
148 ↗(On Diff #51311)

HasVirtualRegisters should be an argument of the pass and we should have an assert HasVirtualRegisters or ! Fn.getRegInfo().getNumVirtRegs().
The rationale is that some backends might end up having virtual registers here whereas they should not.

The problem with checking at pass instantiation time is that (I think) you can't query the target. It's only the MachineFunction object that has the knowledge of what target you are compiling for, and you don't get that until runOnMachineFunction (or runOnFunction in the base MachineFunctionPass).

lib/CodeGen/PrologEpilogInserter.cpp
148 ↗(On Diff #51311)

See the bottom comment about arguments to the pass; here I'll just note that the asserts on lines 161 and 198 do enforce the invariants we care about (assuming there are no virtual targets that require virtual registers but also have CSRs).

dschuff updated this revision to Diff 52010.Mar 29 2016, 4:59 PM
dschuff edited edge metadata.
  • Configure PEI at instantiation time and factor CSR out of pass class

Here's a stab in that direction (just for CSR spilling, we'd do the same kind of thing for scavenging before actually landing it).
There are several changes:

  • Pass instantiation gets a TargetMachine so it can decide whether the target wants virtual registers. We'd need to add some kind of target hook for that.
  • SpillCalleeSavedRegisters becomes a function pointer that does nothing, or the usual thing. This version of the patch makes it a non-member function and passes the data it needs explicitly (before it was just setting member variables). I'm on the fence about whether that's better; I like that it's easier to see the dependencies, but it's 5 arguments to pass around.

Ping!
(can't wait to delete my copy of PrologEpilogInserter from the wasm backend :)

qcolombet added inline comments.Apr 11 2016, 2:15 PM
lib/CodeGen/PrologEpilogInserter.cpp
52 ↗(On Diff #52010)

I’d say for consistency with LLVM codebase we should stick to typedef.

67 ↗(On Diff #52010)

Indeed :).

211 ↗(On Diff #52010)

I still don’t like this part of the patch.
We could do another helper function here, couldn’t we?

653 ↗(On Diff #52010)

Unrelated change.

dschuff added inline comments.Apr 11 2016, 4:54 PM
lib/CodeGen/PrologEpilogInserter.cpp
211 ↗(On Diff #52010)

Yes, sorry if I wasn't clear enough in my comment from 3/29; the intent was to get your opinion on the general direction using CSR spilling and then apply the same idea to scavenging. Since you didn't have any objection to the rest of it, I'll go ahead and do that now :)

dschuff updated this revision to Diff 53465.Apr 12 2016, 2:41 PM
dschuff marked an inline comment as done.
  • Configure PEI at instantiation time and factor CSR out of pass class
  • Also factor out scavenging, fix pass instantiation
dschuff updated this revision to Diff 53467.Apr 12 2016, 2:49 PM
  • remove HasVirtualRegisters and its assert

OK, Here's PEI with register scavenging also factored out.

  1. It's still missing the target hook. I'm looking into that.
  2. Requiring the query of the TargetMachine at pass instantiation time makes the logic in TargetPassConfig a bit ugly because passes are substituted or disabled by ID but now we have instantiate the pass in TargetPassConfig directly. Maybe there's a better way but this way at least preserves the ability of targets to disable, override, or substitute PrologEpilogInserterID.
lib/CodeGen/PrologEpilogInserter.cpp
653 ↗(On Diff #52010)

It's actually not an unrelated change. MinCSFrameIndex and MaxCSFrameIndex are unsigned. Previously MinCSFrameIndex was initialized to INT_MAX, and when I moved that i made it std::numeric_limits<unsigned>::max() to match the type. But then this casting is wrong because MinCSFI becomes -1 instead of a big number when there are no CSRs. So I decided to fix this type mismatch rather than keep the initializer type mismatch just for the purpose of also being able to keep this type mismatch.

dschuff added inline comments.Apr 12 2016, 3:03 PM
lib/CodeGen/PrologEpilogInserter.cpp
69 ↗(On Diff #53467)

For the target hook you could almost use TargetRegisterInfo::getCalleeSavedRegs() (if CSRs is empty, then you can skip spilling) but that requires a MachineFunction because it depends on the calling convention. We could add something like TargetRegisterInfo::usesCalleeSavedRegs() or even come full circle to something like in D15394 :)

qcolombet added inline comments.Apr 21 2016, 11:40 AM
lib/CodeGen/PrologEpilogInserter.cpp
69 ↗(On Diff #53467)

I don’t like the name in D15394, in particular the mention of register allocation (it is antinomic to me to have both virtual register and after regalloc in the same name :)).
The problem with usesCalleeSavedRegs is that it does not convey the information that we want or do not want to scavenge the virtual registers used for frame.

87 ↗(On Diff #53467)

I do not like that UsesCalleeSaves implies all vregs are allocated. That is the same naming problem I mentioned previously, We need to come up with a name for the hook that conveys all that information at once. usesPhysReg for PEI?

dschuff added inline comments.May 11 2016, 5:20 PM
lib/CodeGen/PrologEpilogInserter.cpp
87 ↗(On Diff #53467)

Do you mean usesPhysRegForPEI? or we could be more generic and do usesVirtRegsOnly?.

One issue with the code as-is, is that there's no way to get a targetRegisterInfo without a function (because it hangs off the subtarget, which can be different for each function). We could add a new method to TargetMachine itself but that seems like a big hammer. I'm not sure what else you could do though if you really want to do the pass configuration at instantiation time like this.

qcolombet added inline comments.May 13 2016, 9:56 AM
lib/CodeGen/PrologEpilogInserter.cpp
87 ↗(On Diff #53467)

Do you mean usesPhysRegForPEI?

Yes, that’s what I meant, sorry.

I don’t think this should be in TargetRegisterInfo. I believe this property should hold for the target and not differ between sub targets, otherwise we are mixing some strange code :).
A hook in TargetMachine maybe?

dschuff updated this revision to Diff 57212.May 13 2016, 10:26 AM
  • Use TargetMachine hook
dschuff marked an inline comment as done.May 13 2016, 10:27 AM

OK; I think this might actually be ready to go then...

qcolombet accepted this revision.May 16 2016, 10:53 AM
qcolombet edited edge metadata.

Hi Derek,

Thanks for pushing this all the way through!

LGTM.

Cheers,
-Quentin

This revision is now accepted and ready to land.May 16 2016, 10:53 AM
This revision was automatically updated to reflect the committed changes.

Thanks for all the reviews!