[PATCH] D145131: [Arm][AArch64] Setting IsX18ReservedByDefault() to true for Unknown OSes

Fri Mar 3 01:09:50 PST 2023

DavidSpickett requested changes to this revision.
DavidSpickett added a comment.
This revision now requires changes to proceed.

> 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)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145131/new/

https://reviews.llvm.org/D145131