ARM-software / abi-aa

Application Binary Interface for the Arm® Architecture
Other
938 stars 188 forks source link

Adding specification to handle the new C2x _BitInt type #191

Closed mmalcomson closed 1 year ago

mmalcomson commented 1 year ago

Here we add mappings from the new _BitInt type to machine types.

All that I believe needs to be changed in the ABI documents themselves are entries in the Mapping tables for the PCS rules. I explain in the commit messages below the reasoning that this is all that is needed.

This PR also contains a rationale document. As discussed I'm putting the initial proposed document up and going to add the comments I've already heard onto this github request (to keep the discussion as in the open as possible).

workingjubilee commented 1 year ago

Regarding _BitInt, I would like to raise a concern about whether C's _BitInt(128) and AArch64's __int128 will be compatible in terms of layout, alignment, and parameter passing. If they are not, it may significantly complicate ABI handling and type definitions, especially for other languages that are not C, and on AArch64, FFI compatibility with __int128 is much more important than on x86-64 because it is used in the "hardware ABI" (it appears a lot in the layout of essential types). It also seems like a programmer footgun in general if they are inconsistent, even for just normal C code, as well as its relevance for STXP/LDXP.

It's been a while since I've read the AAPCS64 last so apologies if this has already been addressed and the spec as-is will guarantee that, it wasn't immediately obvious from my skimming of this PR whether this was the case.

I agree that I don't think these will be performance-critical types at the larger sizes, however.

mmalcomson commented 1 year ago

@workingjubilee Thanks for the feedback!

W.r.t. the _BitInt(128) yeah, the current spec does have them compatible with AArch64 __int128, but it's still good to hear that it was on someones mind since it increases our confidence in that choice.

The "performance critical" statement was a bit of a clumsy glossing-over of some analysis on my part. I'm in the process of adding the details of the analysis to the rationale document, hopefully that would make it clear what the current thought process is -- I'd appreciate hearing your thoughts on whether the adjustment makes sense and is clear when I post it.

Cheers!

workingjubilee commented 1 year ago

Aha, excellent!

and on AArch64 _BitInt(128) has the same alignment and size as a __int128 which is the largest type defined on that platform. This falls out of the fact that double-register size maps to the largest integral Fundamental Data Type defined on both platforms.

Ah, good, this was the part I missed. Thank you!

I'll be happy to look at things when you follow up!

stuij commented 1 year ago

@rearnsha

mmalcomson commented 1 year ago

Just pinging everyone that has been watching this. Things seem to have settled on a decision here -- if there are still objections that I've missed could someone raise them again.

Otherwise could someone @rearnsha @smithp35 @stuij have a double-check that everything seems fine to them and approve this?

mmalcomson commented 1 year ago

Please do avoid using latin contractions (e.g., i.e, and etc) they're not good for non-native speakers.

Noted -- thanks for the info!

mmalcomson commented 1 year ago

@sallyarmneale Thanks for the reviews -- I've updated all the things you pointed at and upon the re-read found some places that I felt could be better worded as well. N.b. to anyone else reading -- I also re-ordered the reasons in a few places. A while ago Wilco pointed out that the ordering of reasons gave an impression that we weighted the behaviour around atomics higher than we actually did, and on re-reading it seems I had only re-ordered some of the mentions.