This is an archive of the discontinued LLVM Phabricator instance.

[Arm][AArch64] Setting IsX18ReservedByDefault() to true for Unknown OSes
Needs RevisionPublic

Authored by vmurali on Mar 1 2023, 5:05 PM.

Details

Summary

Arm manual says: X18 is the platform register and is reserved for the use of platform ABIs. This is an additional temporary register on platforms that don't assign a special meaning to it.

Source: https://developer.arm.com/documentation/den0024/a/The-ABI-for-ARM-64-bit-Architecture/Register-use-in-the-AArch64-Procedure-Call-Standard/Parameters-in-general-purpose-registers

Diff Detail

Event Timeline

vmurali created this revision.Mar 1 2023, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 5:05 PM
vmurali requested review of this revision.Mar 1 2023, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 5:05 PM
vmurali added a reviewer: pratlucas.

Added reviewers based on activity in the file.

Benoit added a comment.Mar 1 2023, 5:55 PM

I'm not legitimate as a reviewer here, but as a user I'll voice a big +1 for this. It is common to be using target triples with unknown OS, and without this patch, doing so results in very hard to debug issues in code that uses x18.

Please confirm that the CI failures are not due to this change and add/update an existing test case for this change. Presumably a triple of "aarch64-notarealos-gnueabi" will trigger this.

There is a chance someone out there is relying on this, but that seems like a risky position to begin with. If they are, they will probably just get less efficient code, so I don't object to the change.

There should be a release note for this, for those minority of cases. I know there are command line options for reserving registers (-ffixed-?), please check if those would allow folks to override this and if so add that to the note. "if you do not want this behaviour you can use....".

[Arm][AArch64]

In the context of llvm, ARM = Arm 32 bit, AArch64 = Arm 64 bit (I know, it's hardly elegant). So you just need [AArch64]. Ideally [llvm][AArch64].

Finally, do we know what RISCV does for this? They have a method by the same name and we might as well make clang consistent if we can.

dmgreen added a subscriber: dmgreen.Mar 2 2023, 3:09 AM

I don't think this is the right approach to take. As far as I understand this will effect any aarch64-none-eabi targets that are compiling for bare metal (as the tests show), causing a performance hit to any bare metal application. They shouldn't have to pay the price if they don't actually use x18 for anything, which many will not.

Yes I made a mistake here, I didn't think of "unknown" as including bare metal.

https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#general-purpose-registers

If the platform ABI has no such requirements, then it should use r18 as an additional temporary register. The platform ABI specification must document the usage for this register.

Which reads as a very "must opt into reserving x18" situation, rather than default to reserving it.

Potentially the better way to do this is to use the -ffixed-x18 option if you have an unknown OS but you know it reserves x18.

Also there is no -fno-fixed-x18 option. So users would have no way to opt out if we did this change.

I agree with @dmgreen, this doesn't seem to be the right default behaviour according to the ABI. I understand it can be useful for building cross-platform code, but "unknown" also affects other use cases such as bare metal targets.

As @DavidSpickett mentioned above, there are existing command line options that allow the user to opt-in to this behaviour, as recommended by the AAPCS64 document when creating platform-independent code:

Software developers creating platform-independent code are advised to avoid using r18 if at all possible. Most compilers provide a mechanism to prevent specific registers from being used for general allocation;

Benoit added a comment.Mar 2 2023, 7:19 AM

Thank you for the explanation regarding the current constraint that "unknown" includes in particular "bare metal". Do you think that it would make sense going forward to start distinguishing between "bare metal" and "unknown"? After all, knowing that the target is "bare metal" is information in its own right, and here it enables using x18.

Likewise, if there is no -fno-fixed-x18 flag and that is part of why the present diff is not immediately acceptable, maybe that could be motivation to add a -fno-fixed-x18 flag.

Just to say, I understand the immediate decision here but I also hope that can be resolved differently in the future.

Benoit added a comment.Mar 2 2023, 7:22 AM

As @DavidSpickett mentioned above, there are existing command line options that allow the user to opt-in to this behaviour, as recommended by the AAPCS64 document when creating platform-independent code:

Software developers creating platform-independent code are advised to avoid using r18 if at all possible. Most compilers provide a mechanism to prevent specific registers from being used for general allocation;

It isn't ideal that such an important opt-in isn't more discoverable --- most users only learn about this whole topic after debugging for hours the consequences of using x18 :-)

Also there is no -fno-fixed-x18 option. So users would have no way to opt out if we did this change.

Perhaps that can be fixed as an opt-in. But nevertheless, fixing all those bare metal tests doesn't seem like a feasible path for me right now.

Please confirm that the CI failures are not due to this change and add/update an existing test case for this change. Presumably a triple of "aarch64-notarealos-gnueabi" will trigger this.

If it's indeed only from "aarch64-notarealos-gnueabi", then that can be fixed more easily.

There is a chance someone out there is relying on this, but that seems like a risky position to begin with. If they are, they will probably just get less efficient code, so I don't object to the change.

There should be a release note for this, for those minority of cases. I know there are command line options for reserving registers (-ffixed-?), please check if those would allow folks to override this and if so add that to the note. "if you do not want this behaviour you can use....".

It is currently an opt-out. I can make it an opt-in.

[Arm][AArch64]

In the context of llvm, ARM = Arm 32 bit, AArch64 = Arm 64 bit (I know, it's hardly elegant). So you just need [AArch64]. Ideally [llvm][AArch64].

Aah thanks.

Finally, do we know what RISCV does for this? They have a method by the same name and we might as well make clang consistent if we can.

I haven't taken a look at RISCV.

As @DavidSpickett mentioned above, there are existing command line options that allow the user to opt-in to this behaviour, as recommended by the AAPCS64 document when creating platform-independent code:

Software developers creating platform-independent code are advised to avoid using r18 if at all possible.

Doesn't this imply x18 should be reserved by default unless they change the default to use it?

Thank you for the explanation regarding the current constraint that "unknown" includes in particular "bare metal". Do you think that it would make sense going forward to start distinguishing between "bare metal" and "unknown"? After all, knowing that the target is "bare metal" is information in its own right, and here it enables using x18.

+1 to creating such a distinction

Doesn't this imply x18 should be reserved by default unless they change the default to use it?

No. That text is mostly about building your platform independent source code, in the right way for the platform that will run it. That's why it highlights hand written assembly. You can easily tell a compiler to not use x18, but you can't make it translate x18 uses in inline assembly or assembly source files.

Thank you for the explanation regarding the current constraint that "unknown" includes in particular "bare metal". Do you think that it would make sense going forward to start distinguishing between "bare metal" and "unknown"? After all, knowing that the target is "bare metal" is information in its own right, and here it enables using x18.

I disagree with this because as soon as some unknown platform wants to break from what clang has decided the default should be, we will have to add a bunch of options to subtract parts of it.

The ABI already handles this by saying there are two "levels" of the rules:

  • The base AArch64 AAPCS. Which we use for bare metal right now, and is the basis for all the platforms.
  • Specific choices platforms can make on top of that, we have taught clang about these specific cases.

I don't see that breaking from the ABI structure here helps any. You can already do -target aarch64-whatever-eabi -ffixed-x18 and get what you need.

You can teach clang about your platform too, if you really want to only use 1 option. OpenHarmony is the most recent example.

I'm not legitimate as a reviewer here, but as a user I'll voice a big +1 for this. It is common to be using target triples with unknown OS, and without this patch, doing so results in very hard to debug issues in code that uses x18.

Actual users are absolutely welcome to chime in :) Far too often that voice is missing.

-ffixed-x18 really is your solution here if you want the bare metal ABI plus that one change. I do agree that it's annoying to have to find that out many hours into debugging. Perhaps we could better document that these options exist. Do you remember where you looked and what you read when you first chose your -target settings?

For instance, if one of those places would document what clang does when the platform is unknown that could help. "if the platform is not recognised, you will likely get the default/bare metal ABI choices for...some platforms will have extra options to control these choices such as....".

Doesn't this imply x18 should be reserved by default unless they change the default to use it?

No. That text is mostly about building your platform independent source code, in the right way for the platform that will run it. That's why it highlights hand written assembly. You can easily tell a compiler to not use x18, but you can't make it translate x18 uses in inline assembly or assembly source files.

Thank you for the explanation regarding the current constraint that "unknown" includes in particular "bare metal". Do you think that it would make sense going forward to start distinguishing between "bare metal" and "unknown"? After all, knowing that the target is "bare metal" is information in its own right, and here it enables using x18.

I disagree with this because as soon as some unknown platform wants to break from what clang has decided the default should be, we will have to add a bunch of options to subtract parts of it.

The ABI already handles this by saying there are two "levels" of the rules:

  • The base AArch64 AAPCS. Which we use for bare metal right now, and is the basis for all the platforms.
  • Specific choices platforms can make on top of that, we have taught clang about these specific cases.

I don't see that breaking from the ABI structure here helps any. You can already do -target aarch64-whatever-eabi -ffixed-x18 and get what you need.

You can teach clang about your platform too, if you really want to only use 1 option. OpenHarmony is the most recent example.

I'm not legitimate as a reviewer here, but as a user I'll voice a big +1 for this. It is common to be using target triples with unknown OS, and without this patch, doing so results in very hard to debug issues in code that uses x18.

Actual users are absolutely welcome to chime in :) Far too often that voice is missing.

-ffixed-x18 really is your solution here if you want the bare metal ABI plus that one change. I do agree that it's annoying to have to find that out many hours into debugging. Perhaps we could better document that these options exist. Do you remember where you looked and what you read when you first chose your -target settings?

For instance, if one of those places would document what clang does when the platform is unknown that could help. "if the platform is not recognised, you will likely get the default/bare metal ABI choices for...some platforms will have extra options to control these choices such as....".

Thanks for your explanation. We are trying to generate platform-agnostic binary, (i.e. the lowest common denominator) so we set the OS to unknown. But unfortunately, that means, on certain platforms the generated code won't work. We didn't realize x18 is a special register, that may get clobbered in just a few platforms during normal llvm compilation. We found the strange behavior running gdb/lldb, and then looked at the doc which said x18 is platform specific.

The workaround right now is we reserve x18 explicitly.

As far as suggestions for documentation, I am not sure where that would go for LLVM. Perhaps LLVM can scream at us when invoked for compilation for AArch64 platforms when x18 is not reserved when the OS is unknown, saying the generated binary can be wrong?

DavidSpickett requested changes to this revision.Mar 3 2023, 1:09 AM

We are trying to generate platform-agnostic binary, (i.e. the lowest common denominator) so we set the OS to unknown.

Right, so the missing link here is knowing that "unknown" == bare metal ABI wise.

As far as suggestions for documentation, I am not sure where that would go for LLVM. Perhaps LLVM can scream at us when invoked for compilation for AArch64 platforms when x18 is not reserved when the OS is unknown, saying the generated binary can be wrong?

It would scream in a whole bunch of builds I expect. If you can make a decent argument that people aren't using "unknown" to mean "bare metal", maybe we can do that. However the warning will have to be more general as there are many more platform choices in the ABI. Also in some situations it will not be wrong. So it's more a "you should use the bare metal target plus ABI options" warning.

This is the closest docs I found for your situation: https://clang.llvm.org/docs/CrossCompilation.html#general-cross-compilation-options-in-clang

The system name is generally the OS (linux, darwin), but could be special like the bare-metal “none”.

When a parameter is not important, it can be omitted, or you can choose unknown and the defaults will be used. If you choose a parameter that Clang doesn’t know, like blerg, it’ll ignore and assume unknown, which is not always desired, so be careful.

Finally, the ABI option is something that will pick default CPU/FPU, define the specific behaviour of your code (PCS, extensions), and also choose the correct library calls, etc.

Though it does not say what the undesired consequences might be. I think at least expanding that is a good idea.

This is why I ask what things did you read, because we can document things but if no one can find them, it's no use. So if you looked for specific terms or went to specific pages, we know the path we need to build.

(requiring changes because we are not going to accept the current implementation of this)

This revision now requires changes to proceed.Mar 3 2023, 1:09 AM
lenary resigned from this revision.Apr 13 2023, 9:58 AM