This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Store call frame size in MachineBasicBlock
ClosedPublic

Authored by foad on Jul 24 2023, 5:38 AM.

Details

Summary

Record the call frame size on entry to each basic block. This is usually
zero except when a basic block has been split in the middle of a call
sequence.

This simplifies PEI::replaceFrameIndices which previously had to visit
basic blocks in a specific order and had special handling for
unreachable blocks. More importantly it paves the way for an equally
simple implementation of a backwards version of replaceFrameIndices,
which is required to fully convert PrologEpilogInserter to backwards
register scavenging, which is preferred because it does not rely on
accurate kill flags.

Diff Detail

Event Timeline

foad created this revision.Jul 24 2023, 5:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 5:38 AM
foad requested review of this revision.Jul 24 2023, 5:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 5:38 AM
foad added a comment.Jul 24 2023, 5:43 AM

This is a rework of D154281:

  • Fixes the ARM bug reported by @olista01. ARM frame index elimination was relying on SPAdj being passed in as 0 if call frame pseudos were eliminated early (the canSimplifyCallFramePseudos case).
  • Stores "call frame size" instead of "SP adjustment". This feels a bit more canonical and maps directly to the call frame size stored in call frame setup/destroy pseudos.
  • Adds machine verification and a bunch of fixes that that required.
arsenm added inline comments.Jul 24 2023, 5:50 AM
llvm/lib/CodeGen/MachineVerifier.cpp
3405

Separate err from the report seems weird but it seems that's what all this code is already doing

3407

single quotes

llvm/lib/CodeGen/PrologEpilogInserter.cpp
391–394

Should the pseudo elimination itself take care of this?

llvm/lib/CodeGen/TargetInstrInfo.cpp
1466

I'd assume this would be a compute this to set this situation, not a try to compute and fallback to precomputed?

foad added inline comments.Jul 24 2023, 5:58 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
391–394

I don't see how that would work. You could (at least in theory) have block with a non-0 call frame size on entry, that doesn't contain any call frame pseudos. In any case it seems a bit weird for the per-instruction call eliminateCallFramePseudoInstr to change per-block state.

llvm/lib/CodeGen/TargetInstrInfo.cpp
1466

It's not really a matter of computing vs using a precomputed value.

To find the call frame size at an arbitrary point, conceptually you should start with the stored value at the start of the BB and then walk forwards updating it on every call frame pseudo. But because call frame pseudos are not allowed to be nested it is quicker to walk backwards and stop at the first pseudo you hit.

foad updated this revision to Diff 543495.Jul 24 2023, 6:05 AM

Single quotes.

arsenm accepted this revision.Jul 26 2023, 2:23 PM
This revision is now accepted and ready to land.Jul 26 2023, 2:23 PM
This revision was landed with ongoing or failed builds.Jul 27 2023, 2:32 AM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Jul 27 2023, 9:58 AM

The new test is failing on Windows, would it be possible to take a look and revert if needed?

Script:
--
: 'RUN: at line 2';   c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe -march=arm -run-pass none -o - C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir | c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 2"
$ "c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe" "-march=arm" "-run-pass" "none" "-o" "-" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"
# command stderr:
error: Function 'callframesize' uses ARM instructions, but the target does not support ARM mode execution.

error: command failed with exit status: 1
$ "c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"

--

The new test is failing on Windows, would it be possible to take a look and revert if needed?

Script:
--
: 'RUN: at line 2';   c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe -march=arm -run-pass none -o - C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir | c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 2"
$ "c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe" "-march=arm" "-run-pass" "none" "-o" "-" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"
# command stderr:
error: Function 'callframesize' uses ARM instructions, but the target does not support ARM mode execution.

error: command failed with exit status: 1
$ "c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"

--

I assume this just needs s/march/mtriple

foad added a comment.Jul 27 2023, 10:12 AM

The new test is failing on Windows, would it be possible to take a look and revert if needed?

Script:
--
: 'RUN: at line 2';   c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe -march=arm -run-pass none -o - C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir | c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 2"
$ "c:\b\s\w\ir\x\w\llvm_build\bin\llc.exe" "-march=arm" "-run-pass" "none" "-o" "-" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"
# command stderr:
error: Function 'callframesize' uses ARM instructions, but the target does not support ARM mode execution.

error: command failed with exit status: 1
$ "c:\b\s\w\ir\x\w\llvm_build\bin\filecheck.exe" "C:\b\s\w\ir\x\w\llvm-llvm-project\llvm\test\CodeGen\MIR\ARM\call-frame-size.mir"

--

I assume this just needs s/march/mtriple

@phosek does that fix it for you?

thakis added a subscriber: thakis.Jul 27 2023, 10:15 AM

Also failing on http://45.33.8.238/win/81600/step_11.txt

If you think that helps, can you commit that? I think we're all just looking at bots and can't easily if a fix helps.

foad added a comment.Jul 27 2023, 10:25 AM

If you think that helps, can you commit that? I think we're all just looking at bots and can't easily if a fix helps.

e5f04830c59fba05e39b7ca2c958c76876e7cd91, thanks @arsenm