I need to investigate some failures with this patch whereby sections of code which reference different temporary symbols still get outlined, with only one reference being used in the outlined function.

In D62190#1712206, @apazos wrote:

Lewis, this patch is not applying clean on top of https://reviews.llvm.org/D62686. Can you please rebase?

Disable the save/restore optimization when a function contains tail calls. Address various miscellaneous concerns with the patch. Update tests to include less redundant code when checking cases where save/restore libcalls are not used.

In D62686#1708792, @apazos wrote:

Yes Eli thanks for pointing out there are more scenarios that can fail.
It looks like the best solution is to permit both flags on, but then bail out from doing this transformation if there is any type of tail call already in the function.
This way we avoid messing with reverting tail calls back to regular calls.

Use MBB.end() and a blank DebugLoc for epilogue insertion when a basic block is empty.

In D66210#1699653, @lewis-revill wrote:

Rebased prior to commit; Will run this through the testsuite once more first.

Rebased prior to commit; Will run this through the testsuite once more first.

Rebased to fix conflicts with recent split SP adjustments

Make this patch independent of save/restore compatibility

Hi Ana, I'm yet to try this out on our benchmarks, but it looks like a good implementation to me. Considering Alex's comments above, should this patch be reduced to just the isCompressible check and I can merge the rest into the machine outliner patch after adding this one as a parent?

Sure, that's sensible. I'll keep a separate patch for ensuring compatibility with save/restore for if/when it's required.

Rebased due to updated dependency.

Rewrote logic to calculate stack sizes, frame indexes and frame pointer offsets. This was necessary to take into account the fact that the save/restore lib calls are essentially an opaque section of the stack that is inserted at the beginning of the frame, after positive offset objects such as fixed stack arguments, but before additional callee saved registers (IE FP registers) and negative offset (dynamic) objects. So calculations of the actual offsets of these objects must be adjusted accordingly for the stack to function correctly.

Add missing bailout condition after potentially removing candidate locations.

In D66210#1684230, @apazos wrote:

I have run a couple of standard workloads like SPEC.
There is no correctness issue when enabling the MO feature (except for spec2000/twolf which fails with/without MO).
The MO code size gains without compression are up to 7%. With compression enabled, most of the code size gain is gone, and I see more code size increase.
It is possible it has to do with the SequenceSize we are estimating.
The reason we enable compression late is to have all paths - coming from codegen (LLVM IR), parsing, assembling .s or inline asm - go through the same mechanism for compression.
This is similar/compatible with GCC behavior, which relies on the external assembler to implement compression.
We can better estimate SequenceSize by checking if an instruction is compressable. We can modify the tablegen backend for compression emitter to generate a function that does the isCompressable check, but instead of using MCInst and MCOperands for the checks, we need to use MachineInstr and MachineOperand types. I will try this solution. Another alternative is to compress LLVM IR code before running the machine outliner.

It seems like the regressions I'm seeing are due to the fact that calculating offsets for fixed objects at the top of the frame didn't account for extra stack size adjustment from the libcalls. I'm trying to find a neat way around this without breaking other calculations.

In D62686#1675347, @lenary wrote:

We discussed this in the RISC-V meeting on 19 Sept 2019. @apazos says there are some SPEC failures in both 2006 and 2017, which would be good to triage.

Rebased and updated to incorporate changes to parent revision

Replace internal -mllvm option with target feature enabled through the clang frontend using -msave-restore.

In D62686#1672346, @lenary wrote:

In D62686#1671123, @lewis-revill wrote:

There is an option to Clang for '-msave-restore' which should be utilized to enable this.

I don't think we'd have a problem if clang support for this came in a separate commit.

Updated to address comments.

There is an option to Clang for '-msave-restore' which should be utilized to enable this.

Fix git error whereby logic from this patch was mistakenly included in D62190

Fix git error whereby logic from D62686 was mistakenly included in this patch

Refactor the MIR test case again:

• use -run-pass=machine-outliner to only test this pass (required removing the no-machineoutliner checks, they are redundant anyway)
• add riscv64 checks for completeness?
• add -verify-machineinstrs

Don't allow return statements in outlined sequences until tail calls to outlined functions are properly implemented. Improve logic for checking if X5 is available for use at the insertion point of an outlined sequence, and update the test to be pure MIR.

I've been testing this on the Embench set of benchmarks (https://github.com/embench/embench-iot), and it appears that some programs cause execution failures with this enabled, which is a little concerning... I'm going to look into this and then also update the tests.

Updated the test to an MIR-based test, and added checks for the cases where outlining cannot be done.

Thanks Kito. This looks good to me.

I'd just like to bring up that at Embecosm we have been in discussions with the GCC community for a while about an alternative approach to this style of blacklisting, where instead we add more fine grained procedures within the target-supports.exp file to allow us to annotate tests with things like /* { dg-require-effective-target builtin_setjmplngjmp } */. For now we have also added gcc_frontend and gcc_internals 'effective targets' to help with cases like tests which check for GCC-specific warning output, or GCC-specific optimization behaviour.

Rebased and addressed a couple of comments.

Rebased, and modified the logic to find the insertion point for the epilogue code.

Rebased.

Committed (ill formed commit message so phabricator did not pick it up)

Rebased prior to commit.

Rebased prior to commit.

In D62686#1571192, @asb wrote:

Hi Lewis, did you evaluate implementing the machine outliner as a more general approach for getting code size savings in this sort of way?

Abandoned since this is no longer relevant to RISC-V; other individual fixes for issues relating to this problem have been merged since. I will keep D61907 around for discussion even though it's a similar 'big hammer' approach to the problem.

In D62686#1571192, @asb wrote:

Hi Lewis, did you evaluate implementing the machine outliner as a more general approach for getting code size savings in this sort of way?

You should have tests for at least a subset of target-abi {ilp32,lp64}{f,d} and the compiler shouldn't assert in those cases.

Rebased and updated to use '0(' prefix for memory constraints.

Add support for a split save/restore approach when callee saved registers from the floating point ABIs are used.

Rebased

In D62686#1571360, @lewis-revill wrote:

Will do, I can rebase this patch on top of your hard float patch.

In D62686#1571192, @asb wrote:

Hi Lewis, did you evaluate implementing the machine outliner as a more general approach for getting code size savings in this sort of way?

I've reworked this patch to safely work around the implementation of save/restore via libcalls. These place constraints on the save/restore points that can be chosen by the shrink wrapping algorithm.

Rebase, and only use the getUseSaveRestoreLibCalls getter method rather than the useSaveRestoreLibCalls method which does not save its result.

Quick end-of-file newline fix

Added additional tests

Rebased to incorporate the recent CFI additions.

Just a quick comment, I'm assuming this depends on D58335 based on the test file you've modified?

Should the error change at all? We'll still get 'error: operand must be a bare symbol name' for call foo, bar. I guess this patch doesn't put us in a greate situation now that parseCallSymbol can return NoMatch even though technically we parsed a call symbol...

Abandoned in favour of D61907