In D62686#1820816, @apazos wrote:

Lewis, your latest patch looks good, we just had another run with no new failures. But we know it will have issues with -g. So I think we should not merge it yet. Do you have a version of the patch that creates the labels for the compiler-generated save/restore lib calls, so that this optimization does not depend on D71593? We could merge that version then, and when D71593 is accepted, you just have to rework/remove the label generation part of the patch.

Should I wait for the comments to be resolved on D71593 before I commit this patch? Ideally if this patch makes it into a release then that bug fix should be there too.

Quick fix of shadow variable assignment.

In D71593#1816377, @djtodoro wrote:

Please add a test case.

In D71593#1810372, @dblaikie wrote:

What about going the other way, and making sure instructions in frame setup that need labels get them?

In D62686#1808041, @apazos wrote:

Lewis, is the patch final? It would be good to merge it before the 10.0 release branch creation on Jan 15th

Fix .cfi_offset signedness error.

In D62686#1815158, @pzheng wrote:

I see the following .cfi_offset directives generated using @shiva0217's test case. Any idea why the offset for ra is 536870908?

call    t0, __riscv_save_0
.cfi_def_cfa_offset 16
.cfi_offset ra, 536870908

This patch is yes, so long as it applies cleanly. So if it's agreed that it should be landed then I can do that as soon as I get a chance.

Added .cfi_offset directives for registers saved by libcalls.

Fixed existing .cfi_offset offsets. Since these are frame-pointer based they also need to account for the libcall stack adjustment.

Pinging @paquette perhaps you have some insight into whether there's anything that can be done to avoid this issue (also discussed above).

Thank you for the update! Please see comments.

In D66210#1787974, @apazos wrote:

Can you open a bug for the machine outliner and maybe contact Jessica, may be she can help fix this quickly.

Rebased.

So the failure in the GCC testsuite occurs due to the following test:

Rebased and addressed StackSize vs RealStackSize error.

Rebased.

Is this waiting for someone with commit access to go ahead?

Thanks Scott, it should be relatively easy to add tests for this in 'clang/test/Preprocessor/riscv-target-features.c' if you could.

I'm doing a final rebase and check on this before merging. I have a failure in the GCC testsuite which I'm triaging.

Rebased.

Rebased

So I have a quick comment about this patch, perhaps it might help to get things moving again.

In D62190#1755159, @asb wrote:

I note that the TargetFrameLowering hooks canUseAsPrologue and canUseAsEpilogue are both called by the shrink wrapper. They default to true, but targets may need to override this for correctness. Looking at e.g. AArch64, I see it overrides canUseAsPrologue and returns false in the case that the function needs stack realignment and there are no scratch registers available. Are you certain that no such case is needed for RISC-V?

See comments above. Otherwise, this looks good to me although others should evaluate too.

Rebased

Rebased

Rebased

Rebased. Added special case for size of jr t0 with compression enabled. Fixed failure in GCC testsuite.

This functionality has been merged into the save/restore patch.

Rebased and merged D68644 into this patch - this patch already assumes shrink wrapping support anyway.

Rebased.

So I've been looking into the benchmarks that I've been running to figure out why code doesn't change with this enabled.

Rebase on top of shrink wrapping patch.

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.