[MachineFrameInfo] Refactoring with computeMaxcallFrameSize() (NFC) #78001
Conversation
|
@llvm/pr-subscribers-backend-systemz @llvm/pr-subscribers-llvm-selectiondag Author: Jonas Paulsson (JonPsson1) Changes
This refactoring looks like an improvement to me, but I have one open question and that is regarding global isel as it would also need to set AdjustsStack..? Full diff: https://github.com/llvm/llvm-project/pull/78001.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/MachineFrameInfo.h b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
index 7d11d63d4066f4..0fe73fec7ee67f 100644
--- a/llvm/include/llvm/CodeGen/MachineFrameInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
@@ -638,13 +638,17 @@ class MachineFrameInfo {
bool hasTailCall() const { return HasTailCall; }
void setHasTailCall(bool V = true) { HasTailCall = V; }
- /// Computes the maximum size of a callframe and the AdjustsStack property.
+ /// Computes the maximum size of a callframe.
/// This only works for targets defining
/// TargetInstrInfo::getCallFrameSetupOpcode(), getCallFrameDestroyOpcode(),
/// and getFrameSize().
/// This is usually computed by the prologue epilogue inserter but some
/// targets may call this to compute it earlier.
- void computeMaxCallFrameSize(const MachineFunction &MF);
+ /// If FrameSDOps is passed, the frame instructions in the MF will be
+ /// inserted into it.
+ void computeMaxCallFrameSize(
+ MachineFunction &MF,
+ std::vector<MachineBasicBlock::iterator> *FrameSDOps = nullptr);
/// Return the maximum size of a call frame that must be
/// allocated for an outgoing function call. This is only available if
diff --git a/llvm/lib/CodeGen/MachineFrameInfo.cpp b/llvm/lib/CodeGen/MachineFrameInfo.cpp
index 280d3a6a41edc9..a151e61a2f60de 100644
--- a/llvm/lib/CodeGen/MachineFrameInfo.cpp
+++ b/llvm/lib/CodeGen/MachineFrameInfo.cpp
@@ -184,7 +184,8 @@ uint64_t MachineFrameInfo::estimateStackSize(const MachineFunction &MF) const {
return alignTo(Offset, StackAlign);
}
-void MachineFrameInfo::computeMaxCallFrameSize(const MachineFunction &MF) {
+void MachineFrameInfo::computeMaxCallFrameSize(
+ MachineFunction &MF, std::vector<MachineBasicBlock::iterator> *FrameSDOps) {
const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
unsigned FrameSetupOpcode = TII.getCallFrameSetupOpcode();
unsigned FrameDestroyOpcode = TII.getCallFrameDestroyOpcode();
@@ -192,18 +193,15 @@ void MachineFrameInfo::computeMaxCallFrameSize(const MachineFunction &MF) {
"Can only compute MaxCallFrameSize if Setup/Destroy opcode are known");
MaxCallFrameSize = 0;
- for (const MachineBasicBlock &MBB : MF) {
- for (const MachineInstr &MI : MBB) {
+ for (MachineBasicBlock &MBB : MF) {
+ for (MachineInstr &MI : MBB) {
unsigned Opcode = MI.getOpcode();
if (Opcode == FrameSetupOpcode || Opcode == FrameDestroyOpcode) {
unsigned Size = TII.getFrameSize(MI);
MaxCallFrameSize = std::max(MaxCallFrameSize, Size);
AdjustsStack = true;
- } else if (MI.isInlineAsm()) {
- // Some inline asm's need a stack frame, as indicated by operand 1.
- unsigned ExtraInfo = MI.getOperand(InlineAsm::MIOp_ExtraInfo).getImm();
- if (ExtraInfo & InlineAsm::Extra_IsAlignStack)
- AdjustsStack = true;
+ if (FrameSDOps != nullptr)
+ FrameSDOps->push_back(&MI);
}
}
}
diff --git a/llvm/lib/CodeGen/PrologEpilogInserter.cpp b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
index 8af17e63e25c75..b0f2753c86230f 100644
--- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -228,9 +228,8 @@ bool PEI::runOnMachineFunction(MachineFunction &MF) {
FrameIndexVirtualScavenging = TRI->requiresFrameIndexScavenging(MF);
ORE = &getAnalysis<MachineOptimizationRemarkEmitterPass>().getORE();
- // Calculate the MaxCallFrameSize and AdjustsStack variables for the
- // function's frame information. Also eliminates call frame pseudo
- // instructions.
+ // Calculate the MaxCallFrameSize value for the function's frame
+ // information. Also eliminates call frame pseudo instructions.
calculateCallFrameInfo(MF);
// Determine placement of CSR spill/restore code and prolog/epilog code:
@@ -350,17 +349,13 @@ bool PEI::runOnMachineFunction(MachineFunction &MF) {
return true;
}
-/// Calculate the MaxCallFrameSize and AdjustsStack
-/// variables for the function's frame information and eliminate call frame
-/// pseudo instructions.
+/// Calculate the MaxCallFrameSize variables for the function's frame
+/// information and eliminate call frame pseudo instructions.
void PEI::calculateCallFrameInfo(MachineFunction &MF) {
const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
MachineFrameInfo &MFI = MF.getFrameInfo();
- unsigned MaxCallFrameSize = 0;
- bool AdjustsStack = MFI.adjustsStack();
-
// Get the function call frame set-up and tear-down instruction opcode
unsigned FrameSetupOpcode = TII.getCallFrameSetupOpcode();
unsigned FrameDestroyOpcode = TII.getCallFrameDestroyOpcode();
@@ -370,26 +365,14 @@ void PEI::calculateCallFrameInfo(MachineFunction &MF) {
if (FrameSetupOpcode == ~0u && FrameDestroyOpcode == ~0u)
return;
+ // (Re-)Compute the MaxCallFrameSize with some sanity checks.
+ bool WasComputed = MFI.isMaxCallFrameSizeComputed();
+ unsigned MaxCFSIn = MFI.getMaxCallFrameSize();
+ bool AdjStackIn = MFI.adjustsStack();
std::vector<MachineBasicBlock::iterator> FrameSDOps;
- for (MachineBasicBlock &BB : MF)
- for (MachineBasicBlock::iterator I = BB.begin(); I != BB.end(); ++I)
- if (TII.isFrameInstr(*I)) {
- unsigned Size = TII.getFrameSize(*I);
- if (Size > MaxCallFrameSize) MaxCallFrameSize = Size;
- AdjustsStack = true;
- FrameSDOps.push_back(I);
- } else if (I->isInlineAsm()) {
- // Some inline asm's need a stack frame, as indicated by operand 1.
- unsigned ExtraInfo = I->getOperand(InlineAsm::MIOp_ExtraInfo).getImm();
- if (ExtraInfo & InlineAsm::Extra_IsAlignStack)
- AdjustsStack = true;
- }
-
- assert(!MFI.isMaxCallFrameSizeComputed() ||
- (MFI.getMaxCallFrameSize() >= MaxCallFrameSize &&
- !(AdjustsStack && !MFI.adjustsStack())));
- MFI.setAdjustsStack(AdjustsStack);
- MFI.setMaxCallFrameSize(MaxCallFrameSize);
+ MFI.computeMaxCallFrameSize(MF, &FrameSDOps);
+ assert(!WasComputed || (MaxCFSIn >= MFI.getMaxCallFrameSize() &&
+ !(!AdjStackIn && MFI.adjustsStack())));
if (TFI->canSimplifyCallFramePseudos(MF)) {
// If call frames are not being included as part of the stack frame, and
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 9acfc76d7d5eac..d0609434b8b7ee 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -670,12 +670,12 @@ bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
for (const auto &MI : MBB) {
const MCInstrDesc &MCID = TII->get(MI.getOpcode());
- if ((MCID.isCall() && !MCID.isReturn()) ||
- MI.isStackAligningInlineAsm()) {
+ if ((MCID.isCall() && !MCID.isReturn()) || MI.isStackAligningInlineAsm())
MFI.setHasCalls(true);
- }
if (MI.isInlineAsm()) {
MF->setHasInlineAsm(true);
+ if (MI.isStackAligningInlineAsm())
+ MFI.setAdjustsStack(true);
}
}
}
|
You could probably set it in lowerInlineAsm, where the ExtraFlags are computed. How is this change going to affect code that doesn't pass through isel? For instance if someone writes a MIR test and doesn't specify |
We should teach the verifier to make sure the adjustsStack is consistent with the SP defs in the instruction stream |
JonPsson1
force-pushed
the
MaxCFSRefac
branch
from
8536685 to
3d92dc7
Compare
Thanks - patch updated.
Yeah - for MIR tests using inline-asms with alignstack, they would either have to be created with -stop-before= (to get the frameInfo correct), or adjustsStack would have to be specified manually. |
Should I add a TODO in MachineVerifier, or you think this is needed as part of this? Given this comment, I guess the verifier should check this after PEI. By then the frame instructions will have been removed, so it can only check for any def of SP. The rule would be "if the SP is defined anywhere in the function (or it has an inline-asm with alignstack), adjustsStack should be 'true'? |
Should be a separate patch
Yes, something like that |
| for (const MachineBasicBlock &MBB : MF) { | ||
| for (const MachineInstr &MI : MBB) { | ||
| for (MachineBasicBlock &MBB : MF) { | ||
| for (MachineInstr &MI : MBB) { | ||
| unsigned Opcode = MI.getOpcode(); | ||
| if (Opcode == FrameSetupOpcode || Opcode == FrameDestroyOpcode) { | ||
| unsigned Size = TII.getFrameSize(MI); | ||
| MaxCallFrameSize = std::max(MaxCallFrameSize, Size); | ||
| AdjustsStack = true; |
There was a problem hiding this comment.
Seems weird that we're still computing AdjustsStack for calls, but not asm here. I'd expect both or neither
There was a problem hiding this comment.
I think it would be best in that case to do the calls also during isel, given that not all targets actually need the frame pseudos. On SystemZ we would like to eliminate them early (see #77812) as they serve no use around calls (call frame allocated once for all calls in prolog), and we just realized that we are even suppose keep the StackFrameSize accuarate when splitting blocks in FinalizeISel, which we currently do not do.
I am a little uncertain on exactly what AdjustsStack means. Does it mean that the function redefines SP, or could it also mean that it makes any call (which do not necessarily change SP on all targets, I think)?
It seems that given the current MachineFrameInfo scan for frame instructions, AdjustsStack could be set always in SelectionDAG::getCALLSEQ_START() (and similar for GlobalISel)? (and the current definition of AdjustsStack is then "any call", so maybe the comment could be clarified).
There was a problem hiding this comment.
The call frame pseudos are also used to implement dynamic allocas. I kind of assumed it meant that kind of case, so I'm a little surprised it's set for calls (given we already have a separate hasCalls property)
There was a problem hiding this comment.
This has been a confusion for me also - I actually tried setting the AdjustsStack for any call, but that gave test failures on some target.
Yes, as callseq pseudos seem to be used for other things and not just calls, any refactoring needs to preserve this mapping from them to the AdjustsStack flag, I think.
| @@ -602,6 +603,9 @@ bool InlineAsmLowering::lowerInlineAsm( | |||
| } | |||
| } | |||
|
|
|||
| if (ExtraInfo.get() & InlineAsm::Extra_IsAlignStack) | |||
| MF.getFrameInfo().setAdjustsStack(true); | |||
There was a problem hiding this comment.
Also related #46222? Should probably split this to a separate change
JonPsson1
force-pushed
the
MaxCFSRefac
branch
from
3d92dc7 to
b5d53ef
Compare
|
@arsenm I guess you are right that it makes most sense to compute the AdjustsStack flag for both calls and inlineasms in the same place. I think it should probably be done during isel, since it is not affected by anything else computed later on. However, I started with that thinking that all that would be needed would be to do it once in getCALLSEQ_START(). That turned out to be far from true: There is GlobalISel, FastISel, X86 emits it during EmitCustomInserter, ... Global ISel even emits it by first creating it and then only after creating other instructions adding the final immediate values to it. I am not sure if it would actually be better to do it in isel, as there are so many different places this would have to be remembered. It is simplifying during isel to not have to set AdjustsStack, but of course it could be done. I kept that part out of the patch for now, so now this only avoids the code duplication of computeMaxCallFrameSize(). |
|
Actually, I got the idea of how to handle this for all instruction selectors and targets by simply doing this in finalize isel. This is very simple and seems ok per the comment in top of FinalizeISel.cpp. Patch updated. |
|
It needs to be done with an extra loop as targets may insert new call frame setup instructions during pseudo expansion. If this extra loop is not acceptable, maybe then give up here on setting AdjustsStack during/after isel. |
llvm/lib/CodeGen/FinalizeISel.cpp
Outdated
| @@ -69,6 +72,15 @@ bool FinalizeISel::runOnMachineFunction(MachineFunction &MF) { | |||
| } | |||
| } | |||
|
|
|||
| for (auto &MBB : MF) | |||
There was a problem hiding this comment.
I don't really think it's worth adding an extra loop for it. It doesn't seem like it would be too bad to require custom inserters to set this. It's more OK if we could catch this with the verifier
There was a problem hiding this comment.
OK - patch updated. I now actually found out there were cases of emitting calls even after finalize isel (PPCTLSDynamicCall.cpp), which made it obvious that this should be verified as late as possible. Seems there is no real need to employ the verifier - a single assert in PEI does the job, I think.
JonPsson1
force-pushed
the
MaxCFSRefac
branch
from
02a913e to
bd1779e
Compare
|
PING |
|
ping! I was hoping to commit this before continuing with "[SystemZ] Eliminate call sequence instructions early", so it would be nice to make some progress here... |
|
ping! |
| assert((FrameSDOps.empty() || MF.getFrameInfo().adjustsStack()) && | ||
| "AdjustsStack not set in presence of a frame pseudo instruction."); |
There was a problem hiding this comment.
This probably should go in the verifier. It's really unfortunate when MIR asserts and you have to debug just to realize it was supposed to be invalid MIR
There was a problem hiding this comment.
Ok, will do a follow-up patch for that.
|
Thanks for review. Some more .mir tests were fixed after review approval by adding 'adjustsStack: true' as needed: |
|
I bisect a crash that I see while building the Linux kernel to this change. A trivial reproducer from void input_report_abs(int);
void touchscreen_report_pos(_Bool multitouch) {
input_report_abs(multitouch ? 5 : 0);
input_report_abs(multitouch ? 6 : 1);
} |
Sorry for the trouble - thanks for the bugreport! Fix proposed at #85945 |
Check for all frame instructions in finalize isel, not just for the frame setup opcode. This was proven necessary, see #78001 for discussion.
…llvm#78001) - Use computeMaxCallFrameSize() in PEI::calculateCallFrameInfo() instead of duplicating the code. - Set AdjustsStack in FinalizeISel instead of in computeMaxCallFrameSize().
Check for all frame instructions in finalize isel, not just for the frame setup opcode. This was proven necessary, see llvm#78001 for discussion.
This refactoring looks like an improvement to me, but I have one open question and that is regarding global isel as it would also need to set AdjustsStack..?