Rebased!

The documentation changes to the riscv-toolchain-conventions repository have been merged now, (https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/10). In addition, there's a pending patch to implement this in GCC (https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592467.html). I think this means there's nothing blocking this except a rebase.

To summarize where I think this stands. We don't want to implement something in LLVM which is undocumented or not likely to be implemented for other tools, so I also opened a pull request to document this change (https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/10) in the hope that it could be later implemented for GCC. From the discussion on that pull request there's an understandable reluctance to introduce a feature which would stop libgcc/compiler-rt from being drop in replacements for each other.

Fix comment

Added CFI directives to the __riscv_restore_tailcall_<N> functions

Rebased

In D109371#3099989, @jrtc27 wrote:

In D109371#3099949, @edward-jones wrote:

Some of the previously changes made were undone in order to match the spec as-written (notably I undid the renaming of relocations).

This could do with another review now because I don't expect this patch will change too much now unless the spec also changes.

Aside from the comment I just made about this attitude on the corresponding Clang patch, standard RISC-V relocations can only be specified by the psABI, so the names absolutely can still be changed if the spec authors propose bad names.

In D109372#3099969, @jrtc27 wrote:

In D109372#3099947, @edward-jones wrote:

I reverted some of the previous changes I made so that this patch matches the spec as currently written - this means it's two attributes again, and the diagnostic messages have been updated a bit too. The two Clang attributes match to the same LLVM attribute internally though.

This is at a stage where more review would be nice. Obviously this is gated on patches to other toolchain components, but I hope that these changes won't change too much now unless the spec also changes.

The whole point of putting it up for review is so you get feedback about the entire direction of the spec, which was written by people who are not experts when it comes to toolchains. You’re supposed to take our feedback, relay it to them and get the draft spec revised. Otherwise, if the spec written by people who don’t know what makes sense for toolchains is regarded as holy and immutable then I’m just going to NACK this as poorly designed and something LLVM shouldn’t bow to implementing, and you’ve wasted my time asking for a full review.

Some of the previously changes made were undone in order to match the spec as-written (notably I undid the renaming of relocations).

I reverted some of the previous changes I made so that this patch matches the spec as currently written - this means it's two attributes again, and the diagnostic messages have been updated a bit too. The two Clang attributes match to the same LLVM attribute internally though.

I'm not entirely sure if there's anything that needs to be agreed on still for this. It feels like this has soft agreement, but I don't know whether I would get push back if I get this committed

Changes here:

I've tried to take into account most of the suggested changes into this update:

Thanks for all of the prompt feedback, I'll incorporate the suggested changes and update the patch

In D109372#2987411, @jrtc27 wrote:

In D109372#2987405, @MaskRay wrote:

The name "overlay" is ambiguous. Even after I ruled out Gentoo Overlay and overlayfs, I am thinking whether this has anything to do with OVERLAY description in a linker script: https://sourceware.org/binutils/docs/ld/Overlay-Description.html#Overlay-Description

which are used to mark functions or global data as only accessible through the overlay engine

Can you give more descriptions for folks who don't follow the RISC-V side proposal but need to review your changes? :)

Basically hardware-assisted code+rodata banking (I guess either by actually banking ROMs or just paging stuff in and out) that's mostly transparent to software. Functions at the boundary of components (don't know what the granularity is) use a weird indirect calling convention where you instead call into some magic runtime with a unique ID for the callee, it ensures everything's loaded and then tail calls it for you.

I've rebased and added an option so that the tailcall version of save/restore is only used if -mllvm -riscv-save-restore-tailcall is provided.

It was pointed out in the RISC-V LLVM sync-up that these entry points could be in separate sections to allow -gc-sections to remove any unnecessary entry points. This would mitigate the code size penalty for small programs as only the required entry points would be pulled in, however it would require the linker script to behave and not reorder the sections, and would require some changes to ensure that the fallthrough function does not accidentally get collected.

On the RISC-V sync-up call I said I would try and provide some code size numbers for these changes. First I mentioned that I saw a code size regression when compiled Embench, here's per-benchmark numbers:

Documentation for -msave-restore has been committed to riscv-toolchain-conventions, I have an additional pull request to document this proposed tail-call version here

Updated the implementation to closely match the __riscv_restore_N entry points.

I have updated this so that the generation of the address passed to the restore functions is done in a separate method RISCVFrameLowering::buildAddr. This handles both the code models and mirrors RISCVISelLowering::getAddr - it would be nice if these functions could share code, or at least be in a common place.

In D91717#2594267, @luismarques wrote:

Now that RV32 and RV64 have separate implementations, is there still a point in keeping the LOAD/STORE/STRIDE macros?

I've rebased, switched to a grouping of 2 for rv64 and 4 for rv32, and fixed the formatting/comments.

In D91717#2411369, @jrtc27 wrote:

In D91717#2411334, @edward-jones wrote:

In D91717#2407884, @luismarques wrote:

It seems a bit excessive to me to coalesce the entry points into bundles of 4. Do you have any particular benchmarking data or reasoning that supports choosing that threshold?
Also, shouldn't this implementation include CFI directives?

I used bundles of 4 just to follow the behaviour I saw in libgcc, and the grouping of 2 for rv64 seemed a bit too fine-grained. I'm not sure what the original justification for the coalescing into groups of 2/4 was in libgcc.

I'll update to account for other suggested changes and see if I can find any benchmarks which show the tradeoff for the grouping threshold

Because the stack alignment is 16 bytes; see my earlier comment.

In D91717#2407884, @luismarques wrote:

It seems a bit excessive to me to coalesce the entry points into bundles of 4. Do you have any particular benchmarking data or reasoning that supports choosing that threshold?
Also, shouldn't this implementation include CFI directives?

This looks good to me now.

Looks tidy and a really decent set of tests. I've added a few comments, though they are mainly me agreeing with Simon's points.

Looks good to me. Clang tests all pass on my local build.

Added tests that a user can specify a specific runtime library through --rtlib. Also rebased on master, and added a comment explaining the use of --rtlib=platform in the tests.

If I set -DCLANG_DEFAULT_RTLIB=compiler-rt I see the following failure in clang/test/Driver/cross-linux.c:

It seems that the option --rtlib=platform exists to force the driver to ignore the -DCLANG_DEFAULT_RTLIB for testing purposes, so I've added this option to the tests that were broken.

Okay. I'll see if I can find a way to test this when CLANG_DEFAULT_RTLIB is set, and then I'll resubmit

Okay I've reverted this in rG90ecfa2f5f7f . I'll make improvements and resubmit this for review.

I've changed this to always return ToolChain::UNW_None from RISCVToolChain::GetUnwindLibType now. As a consequence we get the original behaviour of only -lgcc being added to the link command.

In D58896#1737113, @sberg wrote:

But how about literals like '\x80' where the promoted value depends on whether plain char is signed or unsigned?

In D68407#1732685, @lenary wrote:

This patch is looking much better, thanks for updating it.

Please may you clarify what RISC-V gcc does for -lgcc, -lgcc_s, -lgcc_eh? Is it different to what gcc does on other targets? Being closer to matching the linker arguments that gcc provides to ld seems like a good idea, IMO.

This looks good to me, the only thing that I'm not sure about is the phrasing of the warnings:

In D68407#1709235, @lenary wrote:

Please can you add a test for riscv32 and riscv64 without libgcc?

I've rebased, and also refactored this to use AddRunTimeLibs and GetDefaultRuntimeLibType. The tests have been updated to reflect the changes. Notable changes compared to the last version of the patch:

Rebased, added a comment to explain that this is using the user provided triple instead of the canonical one, and split out the tests requiring a shell into separate files (clang/test/Driver/riscv{32,64}-toolchain-extra.c)

In D68393#1707675, @luismarques wrote:

@edward-jones: what LLVM cmake options are you using to test this?

Rebased, updated tests from D68391 to check for existence of compiler-rt crtbegin/crtend and runtime library.

Rebased and added tests