Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax gp could be platform specific register rather than reserved for… #371

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

kito-cheng
Copy link
Collaborator

@kito-cheng kito-cheng commented Mar 22, 2023

… linker relaxation

The usage of gp register has discussed serveral times (e.g. #298 and #305), and it reserved as special register used for linker relaxation, that could be improve perfomance and code size.

However it come with some limitation, like it can't applicable on shared libraries, and it also not work well when program come with large datas since the relaxable range is only +-2KiB.

Some platform like Haiku never use gp in the whole system, and also this might not useful in some baremetal system with specialized memory layout, so we might consider to release the gp usage with a non-hard-ABI-breakage way.

@appujee
Copy link

appujee commented Mar 24, 2023

LGTM. Thanks for proposing this.

For reference: More discussion around this has been going on in: https://lists.riscv.org/g/sig-toolchains/topic/97736679#548 and #370

@rui314
Copy link
Collaborator

rui314 commented Mar 24, 2023

Different platforms might want to use the "platform register" for different purposes, no? If so, just defining 1 for the platform register cannot prevent incompatible object files to be linked to a single executable. We might want to define a value for each specific usage.

@kito-cheng
Copy link
Collaborator Author

@rui314 good point, I add 2 range for standard and non-stanadrd use used range, and also one for undefined one.

@rui314
Copy link
Collaborator

rui314 commented Mar 27, 2023

I'm not sure what the undefined purpose is. Maybe we just want to reserve 2-1024 for the standard uses and 1025-2047 for nonstandard uses?

@kito-cheng
Copy link
Collaborator Author

My first thought about that undefined purpose is kind of reserved for nonstandard use, but we already defined one range for nonstandard, so yeah, that should be just 2-1024 for the standard uses and 1025-2047 for nonstandard uses

@cmuellner
Copy link
Collaborator

We discussed this in the SIG Toolchain call today and agreed that this is the right solution for a RISC-V platform register as it represents the solution with the least expected ABI compatibility hassles.

riscv-elf.adoc Outdated
global pointer relaxation, reserved platform register or a temporary register.

[horizontal]
0:: This object use gp as global pointer relaxation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value is defined for unused gp?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if gp is unused, is it safe to assume that gp is used for global pointer relaxation? As this has been the default behavior.

Can there be cases where gp is allowed for global pointer relaxation and no such opportunities were found in the binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be cases where gp is allowed for global pointer relaxation and no such opportunities were found in the binary?

In theory we can made one program with a symbol with huge size (large then 4k) and point __global_pointer$ to the middle of the symbol, then the global pointer relaxation will have no way to relax anything, but that case should not happened in generally since __global_pointer$ is default computed by a magic (or heuristic) expression with some arithmetic with sbss/sdata.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value is defined for unused gp?

I guess you are intend to tag the linker output as that if gp relaxation disabled? so that we can know this object has perform gp relaxation or not?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if gp is unused, is it safe to assume that gp is used for global pointer relaxation?

It seems not. Operating system may not initialize GP with __global_pointer and leave it with zero or some undefined value. Haiku for example do not define or use __global_pointer because it do not distinguish executables and shared libraries.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Add unused case
  • Drop undefined case
  • Made 3~1023 as reserved for future standard defined platform register.

NOTE: GP as SCS will defined in separated PR.

@MaskRay
Copy link
Collaborator

MaskRay commented Mar 30, 2023

Honestly I don't feel good to add Tag_RISCV_gp_reg_usage now. There are many C++ dialect affecting -f options, yet we don't have compatibility attributes for them. AArch64 has SHT_AARCH64_ATTRIBUTES, but it isn't really used.

If this patch simply states that gp is platform specific, this patch will look good to me.

@kito-cheng
Copy link
Collaborator Author

Honestly I don't feel good to add Tag_RISCV_gp_reg_usage now. There are many C++ dialect affecting -f options, yet we don't have compatibility attributes for them. AArch64 has SHT_AARCH64_ATTRIBUTES, but it isn't really used.

IMO that could be used to sanity check and identity some information how this object/executable compiled, otherwise the only way is writing some magic script/tools to check that like google/android-riscv64#77 mentioned, a binary validator...

-frecord-command-line might be another way, but some info. might not able to know from command line since some setting might be implied by default configuration or target triple, so that's not a reliable way.

There is many C++ dialect, one recent happened example is relative vtables ABI for C++, I didn't ask to add a tag to identify that is because it's still experimental (-fexperimental-relative-c++-abi) (I didn't express this at that PR I know, but that's what I thought and I should say that out at that time...).

Anyway, my point is we should track/record those information if possible, adding attributes could help tool to having better diagnosis message instead of silently broken, otherwise people don't have too much clue to tracking what happened when screw up things.

@MaskRay
Copy link
Collaborator

MaskRay commented Mar 30, 2023

When a source file uses inline assembly to initialize gp, it is impossible to determine how the user intends to use gp, unless the code adds a metadata section manually. For global pointer relaxation, the use case is resolved in the final link phase. It is not necessary, and should not be required, to use a driver option to prevent the compiler from creating an attribute or not indicating GP relaxation.

While a compatibility-detecting feature may be useful, I am skeptical about its necessity and may lean toward not implementing it at all. If such a feature is deemed necessary, it can be proposed separately. In my experience, adding compatibility-detecting features in binutils/glibc leads to inconvenience for several other ports, so waiting before implementing such a feature may be valuable.

As for the specific issue of global pointer relaxation, it could be included in a RISC-V Linux ABI, but I prefer that global pointer relaxation remains as the current GNU ld convention (in other words, it can be default to NOT do this in the future).

glibc and musl initialize gp very early. This is compatible with application using gp for another purpose with --no-relax-gp.

@MaskRay
Copy link
Collaborator

MaskRay commented Mar 30, 2023

For Android and Fuchsia, their decisions don't conflict with Linux ABI and they are less confined by Linux ABI decisions. They control the whole platform, the toolchain and every software piece. Regular Linux distributions have a more loose model and compatibility is likely more important. If we did something inconvenient, it would be difficult to drop all the stuff. I think there is a practical lesson to learn from AArch64 that they postpone (don't do) the AArch32 attributes thing.

@kito-cheng kito-cheng changed the title [RFC] Relax gp could be platform specific register rather than reserved for… Relax gp could be platform specific register rather than reserved for… Apr 4, 2023
@kito-cheng
Copy link
Collaborator Author

@MaskRay Thanks for sharing your thought , split Tag_RISCV_gp_reg_usage out, and that also made this PR become no PoC required, and that's kind of big change to RISC-V ABI, so I would like to get you explicitly LGTM :)

@kito-cheng kito-cheng requested a review from rui314 April 4, 2023 06:59
@kito-cheng
Copy link
Collaborator Author

@asb @luismarques although gp only used in linker relaxation stage, so per our policy that only require consensus from linker gurus, but it's a big change so I would like to make sure RISC-V LLVM maintainer is OK with this.

@rui314 you are linker expert, and I would like all linker expert is happy with this change.

@Nelson1225 I need your opinion as binutils expert :)

@jrtc27 FreeBSD didn't use gp relaxation, so I guess you are fine with this change, but I would like you give explicitly LGTM to this one if possible :)

@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 4, 2023

FreeBSD supports gp relaxation if you use BFD, it just uses LLD by default and so disables it in Clang (but should maybe reenable it now it's on LLD 15).

@asb
Copy link
Collaborator

asb commented Apr 6, 2023

@asb @luismarques although gp only used in linker relaxation stage, so per our policy that only require consensus from linker gurus, but it's a big change so I would like to make sure RISC-V LLVM maintainer is OK with this.

I think although platforms can of course deviate from the standard ABI however they wish, having a suggested register for platform specific purposes makes sense. GP is convenient for this purpose and there seem to be no strong concerns about it, so all LGTM.

I think you need to update the commit message so it no longer claims to introduce a new tag (as this was dropped for now I believe).

riscv-cc.adoc Outdated Show resolved Hide resolved
@asb
Copy link
Collaborator

asb commented Apr 6, 2023

I've left a suggested wording change for the actual patch part of this (inspired partially by #374). I don't think we should equate gp just with gp relaxation, as it might e.g. be used in hand-written assembly.

Copy link
Contributor

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with the premise of this PR. A platform might choose to designate any register as used for an alternate purpose. No matter which register is chosen, it affects the ABI somehow.

@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 6, 2023

No matter which register is chosen, it affects the ABI somehow.

Which is why we're constraining it so it's not a free-for-all. Otherwise you end up in a situation where something like OpenSSL needs a different copy of its crypto assembly routines for each platform because it needs to avoid each platform's reserved register. The whole point of a specification like a psABI is to impose order on the chaos so everyone agrees on what is going on; if you want to allow any register to be reserved by any platform and remain standards-compliant then why bother having an ABI at all?

@aswaterman
Copy link
Contributor

aswaterman commented Apr 6, 2023

@jrtc27 indeed I perceive the intent to reserve gp to be that of a free-for-all promulgated by people who hate linker relaxation as a concept. This is not a process; it's a mob.

@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 6, 2023

@jrtc27 indeed I perceive the intent to reserve gp to be that of a free-for-all promulgated by people who hate linker relaxation as a concept. This is not a process; it's a mob.

This is not constructive, nor is it factually accurate. Please take this rhetoric elsewhere; it has no place in the RISC-V community.

@asb
Copy link
Collaborator

asb commented Apr 6, 2023

@jrtc27 indeed I perceive the intent to reserve gp to be that of a free-for-all promulgated by people who hate linker relaxation as a concept. This is not a process; it's a mob.

Hi Andrew. I don't really have a dog in this fight, but I personally think there is at least some value in giving a hint to people defining platform ABIs which register might be appropriate to use if they need one for their purposes. The fact other ABIs like AArch64 include such a recommendation perhaps provides some support for this point of view. Before further discussing which register to specify, I wanted to check if you disagree that's there's any value at all in giving such a hint?

As to which register to specify - it's not a conversation I've personally been involved in, but all parties so far seem happy that for their use cases, repurposing gp wouldn't be a significant loss. I think its unfortunate if this discussion has become caught up in other suggestions about removing or deprecating gp linker relaxation on standard platforms like Linux, because I think that's a completely completely separate debate (and one I won't comment on because I haven't done the necessary research). Just to make sure I understand your concern properly - is it that choosing gp for this platform specific register might be seized upon to force a decision in a particular direction in any future debate about gp relaxations?

@aswaterman
Copy link
Contributor

Before further discussing which register to specify, I wanted to check if you disagree that's there's any value at all in giving such a hint?

Of course, I agree there's value in doing so.

is it that choosing gp for this platform specific register might be seized upon to force a decision in a particular direction in any future debate about gp relaxations?

That was precisely my point, although I failed to articulate it politely.

@asb
Copy link
Collaborator

asb commented Apr 6, 2023

is it that choosing gp for this platform specific register might be seized upon to force a decision in a particular direction in any future debate about gp relaxations?

That was precisely my point, although I failed to articulate it politely.

It's probably a good thing to discuss this concern explicitly though I think.

So for my part, my LGTM on this specific change is not indicative of any point of view on the best use of GP in the standard ABI as already implemented. I suspect there will be recurrent discussions about it because if you want to scavenge a register for another purpose, GP is going to be near the top of the list. But I see this PR as separate to any such discussion.

@appujee
Copy link

appujee commented Apr 6, 2023

@jrtc27 indeed I perceive the intent to reserve gp to be that of a free-for-all promulgated by people who hate linker relaxation as a concept. This is not a process; it's a mob.

AIUI, the range of gp is small (+/- 2kb) and probably that's why we haven't seen compiler optimizations leveraging that. To the best of my knowledge, none of the Android libraries use gp. If you have ideas on how gp can help in platforms like Android, it will be good to share such that we can realize the trade-offs being made here.

OTOH, re-purposing gp for SCS essentially frees one register for general usage. This can have measurable performance improvements among large variety of workloads, specially when RISC isa is pretty reduced and register pressure is going to be an issue.

@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 6, 2023

@jrtc27 indeed I perceive the intent to reserve gp to be that of a free-for-all promulgated by people who hate linker relaxation as a concept. This is not a process; it's a mob.

AIUI, the range of gp is small (+/- 2kb) and probably that's why we haven't seen compiler optimizations leveraging that. To the best of my knowledge, none of the Android libraries use gp. If you have ideas on how gp can help in platforms like Android, it will be good to share such that we can realize the trade-offs being made here.

GP relaxation is only usable by executables, not libraries, since it takes a constant value throughout execution and thus libraries would compete for its use (though I guess there's an interesting question of whether one could designate a specific library as being the One True GP User, like libc). If I remember correctly, someone from Google in the call on Monday (Elliott?) said that ~everything is a library on Android, which would explain why GP relaxation is not of much use to them? (Though perhaps something like ART could then be blessed as the One True GP User...)

@ilovepi
Copy link
Contributor

ilovepi commented Apr 6, 2023

@aswaterman I'm not sure where you stand on this change, given that you've approved it now, but your comments seem to conflict with that sentiment. Can you provide some clarification on your stance and any lingering concerns?

@aswaterman
Copy link
Contributor

Although I remain concerned that this choice might be speciously used as a pretext to stop using gp relaxations in existing ABIs, I agree with @asb that this PR can go through without addressing that matter.

Copy link

@appujee appujee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM the intent of the patch. As I mentioned in the review, it will be nice to have some explanation on the a) use case b) requirements on the toolchain, and c) risks of using gp for platform-specific purpose without a tag.

@kito-cheng
Copy link
Collaborator Author

kito-cheng commented Apr 7, 2023

@aswaterman I guess I might give you impression that I am not support GP relaxation in #298, but I believe you know that I did know the value of GP relaxation in specific areas, especially on the small bare-metal system, and it also gives us some extra spec score, so I am not intending to deprecate the GP relaxation.

For the SCS, the ideal case is using Zisslpcfi extension, but that would be an issue since it’s not ready now, and also it still require several psABI works (not only SCS, but also landing pad stuffs), so we need to figure out a way to make it moving forward, and GP is the relative best candidate so far.

And of course that’s kind of a mistake that we didn’t learn from other platforms - we didn’t reserve a platform register at the beginning, picking any other general purpose register as platform register would be more painful at this moment.

Anyway, I have worked in the RISC-V world for so many years with you, I also want RISC-V to grow up :)

… linker relaxation

The usage of gp register has discussed serveral times (e.g. #298), and it
reserved as special register used for linker relaxation, that could be
improve perfomance and code size.

However it come with some limitation, like it can't applicable on shared
libraries, and it also not work well when program come with large datas
since the relaxable range is only +-2KiB.

Some platform like FreeBSD and Haiku never use gp in the whole system, and also
this might not useful in some baremetal system with specialized memory layout,
so we might consider to release the gp usage with a non-hard-ABI-breakage way.

Co-authored-by: Alex Bradbury <[email protected]>
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Rebase
  • Apply @asb's suggesion

@kito-cheng
Copy link
Collaborator Author

Thanks everyone, It seems we have converged on this topic, I would like to wait few more day to make sure no further comment before merge :)

@X547
Copy link

X547 commented Apr 11, 2023

GP relaxation is only usable by executables, not libraries, since it takes a constant value throughout execution and thus libraries would compete for its use (though I guess there's an interesting question of whether one could designate a specific library as being the One True GP User, like libc).

PowerPC ABI have similar concept to GP register (TOC register R2) that also works in shared libraries by using special library calling convention with switching TOC register to address of called shared library and switch back on return.

If I remember correctly, someone from Google in the call on Monday (Elliott?) said that ~everything is a library on Android, which would explain why GP relaxation is not of much use to them?

The same for Haiku. Executables and shared libraries are the same except NULL entry point is allowed for shared libraries, but not for executables.

Copy link
Collaborator

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. The new wording looks good to me.

topperc added a commit to llvm/llvm-project that referenced this pull request Apr 13, 2023
…R_RISCV_LO12_S.

This implements support for relaxing these relocations to use the GP
register to compute addresses of globals in the .sdata and .sbss
sections.

This feature is off by default and must be enabled by passing
--relax-gp to the linker.

The GP register might not always be the "global pointer". It can
be used for other purposes. See discussion here
riscv-non-isa/riscv-elf-psabi-doc#371

Reviewed By: MaskRay

Differential Revision: https://reviews.llvm.org/D143673
@kito-cheng
Copy link
Collaborator Author

PowerPC ABI have similar concept to GP register (TOC register R2) that also works in shared libraries by using special library calling convention with switching TOC register to address of called shared library and switch back on return.

@X547 thanks for provide this info, that's interesting idea we could consider in embedded ABI I think.

@kito-cheng kito-cheng merged commit a484e84 into master Apr 14, 2023
@kito-cheng kito-cheng deleted the relax-gp-reg branch April 14, 2023 08:24
@kito-cheng
Copy link
Collaborator Author

Thanks everyone again :)

@MaskRay
Copy link
Collaborator

MaskRay commented Apr 14, 2023

PowerPC ABI have similar concept to GP register (TOC register R2) that also works in shared libraries by using special library calling convention with switching TOC register to address of called shared library and switch back on return.

@X547 thanks for provide this info, that's interesting idea we could consider in embedded ABI I think.

@X547 I don't think PowerPC64 ELFv2 TOC is a good design:) (I have many lld patches in this area) See my summary of TOC on https://maskray.me/blog/2023-02-26-linker-notes-on-power-isa

For a call target which may resolve to a different translation unit (e.g. non-definition declaration, hidden visibility definition), the compiler inserts a NOP after the branch instruction.

Then we'd also need thunks when the caller and the callee may change GP.

@X547
Copy link

X547 commented Apr 14, 2023

the compiler inserts a NOP after the branch instruction.

Can be relaxed, no?

@MaskRay
Copy link
Collaborator

MaskRay commented Apr 14, 2023

the compiler inserts a NOP after the branch instruction.

Can be relaxed, no?

It can, but I am unsure about the linker complexity.

Their mere existence makes a tail call no longer a tail cal. I have summarized all sorts of maintenance costs on https://maskray.me/blog/2023-02-26-linker-notes-on-power-isa

flemairen6 pushed a commit to Xilinx/llvm-project that referenced this pull request May 10, 2023
…R_RISCV_LO12_S.

This implements support for relaxing these relocations to use the GP
register to compute addresses of globals in the .sdata and .sbss
sections.

This feature is off by default and must be enabled by passing
--relax-gp to the linker.

The GP register might not always be the "global pointer". It can
be used for other purposes. See discussion here
riscv-non-isa/riscv-elf-psabi-doc#371

Reviewed By: MaskRay

Differential Revision: https://reviews.llvm.org/D143673
@kito-cheng
Copy link
Collaborator Author

#387 for adding new tag for recording gp usage

veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request Aug 21, 2024
…R_RISCV_LO12_S.

This implements support for relaxing these relocations to use the GP
register to compute addresses of globals in the .sdata and .sbss
sections.

This feature is off by default and must be enabled by passing
--relax-gp to the linker.

The GP register might not always be the "global pointer". It can
be used for other purposes. See discussion here
riscv-non-isa/riscv-elf-psabi-doc#371

Reviewed By: MaskRay

Differential Revision: https://reviews.llvm.org/D143673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants