[PATCH] D16481: Split PrologEpilogInserter into 2 parts

[Derek Schuff via llvm-commits]

dschuff added inline comments.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:77
@@ +76,3 @@
+  // replacement sometimes depends on it.
+  RegScavenger *RS = nullptr;
+
----------------
qcolombet wrote:
> It feels wrong to me that the high level class needs to have this field.
I kind of agree. There are a couple of reasons:
1. `TargetFrameLowering::processFunctionBeforeFrameFinalized()` is called by the base `PEI::finalizeFrame()` and it takes an RS pointer because this is the point where targets (PPC) create a stack slot for the RS to spill into. We could maybe work around that by adding a virtual method on the base class that calls `processFunctionBeforeFrameFinalized()` with a nullptr and with the real RS in the derived class.
2. The base `PEI::calculateFrameObjectOffsets()` takes the scavenging spill slots into account when it lays out the stack frame, and uses the RS to find out where they are. (Likewise it also handles the CSR spill slots, which in principle the base class doesn't need to do). This seems trickier to work around. We could move the code that actually uses RS out to some other function but the logic of where it goes in the frame would still be local.
3. The base `PEI::replaceFrameIndices()` keeps the RS up to date as it goes and passes RS to `TargetRegisterInfo::eliminateFrameIndex()` which is of course where it might actually be wanted by a target.

The root sources of this are that the stack frame layout (which is implemented in this pass, and is always needed) has to be aware of both CSRs and scavenging; and the target hooks (which are also always used) might need RS in any case.
We could try to move the RS/CSR frame layout code and the RS updating code out into some kind of other abstraction, but the RS class is itself essentially the abstraction that you'd want.


http://reviews.llvm.org/D16481