Describe the tailcall version of the __riscv_restore_N functions

[edward-jones]

This describes some additional functions for -msave-restore which allow save-restore to be used even in cases where the original function ended in a tail-call.

These are not currently supported by GCC or LLVM, but I have patches to add support to LLVM and compiler-rt here:

https://reviews.llvm.org/D91720
https://reviews.llvm.org/D91719

[luismarques]

It would be good to also note in the patched documentation that the tailcall version is currently only supported by LLVM / Compiler-RT (maybe with a date, e.g. "As of XXXX...."?).

[edward-jones]

It would be good to also note in the patched documentation that the tailcall version is currently only supported by LLVM / Compiler-RT (maybe with a date, e.g. "As of XXXX...."?).

Okay, I've added an commit to note this

[luismarques]

ended -> end?

[TommyMurphyTM1234]

"end" is not grammatically correct.
"ended" (or "ends") is.

[luismarques]

Sorry, I meant "ends". The point is that it seemed more natural to me to talk about a function that presently exists and ends in a tail call. But it's just a minor nit and I'll leave the grammar and style to the native speakers :)

[luismarques]

I'm not sure if putting the tail here as a regular-looking parameter makes it seem like this is just a normal function call where the tail is going to be received in a0, instead of the actual t1. Maybe I'm overthinking this. Is there a precedent for this or a way to make this not be an issue?

[edward-jones]

I think I agree, would it be better for me to just write these as plain symbol names so a glance doesn't mislead people?

[luismarques]

I'm not sure what the best solution is. You could also add a // comment there or rename tail to tail_in_t1 or something like that. Or just leave it as is. libgcc does seem to use C-like function prototypes (sometimes loosely), so I guess there's precedent for that, despite the corner case ABI issues. I leave this to your best judgment. If anyone else has strong feelings about this please speak up.

[edward-jones]

I think I've done everything asked for the documentation, and the underlying LLVM patches. Is there anything that still needs to be agreed on to get this to land.

In the LLVM patches, the use of the entry points is disabled by default and has to be enabled with an explicit option (-mllvm -riscv-save-restore-tailcall). Should that be documented here? and is gating this behind an option a problem?

[luismarques]

In the LLVM patches, the use of the entry points is disabled by default and has to be enabled with an explicit option (-mllvm -riscv-save-restore-tailcall). Should that be documented here?

That sounds useful and appropriate for a document describing toolchain conventions.

and is gating this behind an option a problem?

I don't think so?

LGTM as is but please do add the info about the opt-in option. Thanks!

[luismarques]

LGTM.

[edward-jones]

Is there any reason this can't be committed now? Does this need to be committed at the same time as the LLVM patch?

[luismarques]

Is there any reason this can't be committed now? Does this need to be committed at the same time as the LLVM patch?

I don't think I have write permissions here. @asb (or someone else with the appropriate permissions): please commit this, unless you think there is any outstanding issue that should be addressed.

[jim-wilson]

There is no GNU toolchain implementation. At a minimum we would need to add the necessary library functions to libgcc so code compiled by LLVM can be linked with gcc. It would be nice if gcc performed the same optimization, but that isn't a requirement for now.

[luismarques]

There is no GNU toolchain implementation. At a minimum we would need to add the necessary library functions to libgcc so code compiled by LLVM can be linked with gcc. It would be nice if gcc performed the same optimization, but that isn't a requirement for now.

If there isn't anything wrong with the technical proposal itself then I don't see a problem with accepting this PR basically as-is. This is just documentation. I don't see the need to wait for the GNU toolchain implementation to add documentation, particularly since the PR indicates that currently only LLVM implements this (or, rather, will implement once this PR is merged, as the respective LLVM patch was gated on receiving feedback here). If you feel that there are still technical issues that merit discussion then IMO that's a different story.

[jim-wilson]

The point of this document is to ensure compatibility between GCC and LLVM. If we add LLVM only features, then we no longer have compatibility between the two compilers. The RISC-V Software group has rules on extending the ABI, and this requires that both GCC and LLVM support a feature before it can be part of the ABI.

[jim-wilson]

Something I wrote a while ago but never had time or help to finish. I don't know if it is compatible with the current definition of the feature. I will need time to check that. But if this is OK then this would be enough to add the feature to the docs. This is basically the same as the existing restore functions, except the ret at the end is replaced with a jr t1.
sibling-call-save-restore.txt

[luismarques]

The point of this document is to ensure compatibility between GCC and LLVM. If we add LLVM only features, then we no longer have compatibility between the two compilers. The RISC-V Software group has rules on extending the ABI, and this requires that both GCC and LLVM support a feature before it can be part of the ABI.

I think that in some cases it might make sense to take a less strict view of the scope of this document. But I agree that it's good to keep pushing for a high degree of compatibility, and I thank your effort here.

Something I wrote a while ago but never had time or help to finish. I don't know if it is compatible with the current definition of the feature. I will need time to check that. But if this is OK then this would be enough to add the feature to the docs. This is basically the same as the existing restore functions, except the ret at the end is replaced with a jr t1. sibling-call-save-restore.txt

Yup, that seems correct. Thanks for sharing.

[edward-jones]

Getting back to this a bit late. So in summary do we need to also add this to the libgcc implementation before this can become part of the documentation?

[jim-wilson]

I stopped reading a bunch of mailing lists for over a month. There is unfortunately a serious lack of people willing to do the work here, so if I'm not doing it then probably nothing will get done. You might try to get the psABI committee interested in this. At least there are people doing work over there. But they have a short deadline to try to finish the v1.0 psABI so they likely don't have time for this now.

I did do a trial GCC implementation before I took a break, but then realized that there is a serious flaw with my implementation. I put the new tailcall functions in the same libgcc file as the old function, but that results in a code size increase for every program using -msave-restore. The new tailcall functions need to be put in a different file, so they only get linked in if they are being called. I think I took a break before getting around to rewriting the patch to do that.

[lewis-revill]

I have gone ahead and submitted a patch with the libcalls to the GCC patches mailing list (https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592467.html) - it's essentially the same implementation as provided by @jim-wilson but placed in a separate file from the rest of the save/restore libcalls.

[kito-cheng]

LGTM, I would prefer to merge the gcc patch (https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592467.html) until GCC 13 is open for GNU toolchain side, but I think that should not be a blocker of this PR.

[kito-cheng]

I believe this PR open for review long enough and got approval from LLVM and GNU community, so I gonna merge this :)