[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

Wed Aug 25 04:13:33 PDT 2021

aaron.ballman added a comment.

In D108643#2964190 <https://reviews.llvm.org/D108643#2964190>, @rjmccall wrote:

> I agree with James; I know you've reached out to the Itanium ABI group about mangling, but ABI involvement needs to mean also reaching out and getting psABI agreement about representation.  I would suggest proposing a generic ABI, getting consensus on that generic ABI with other implementors, and then running that generic ABI past as many platform ABI groups as you can get in touch with.

I've already reached out to the psABI maintainers and have started those discussions before ever considering the mangling questions: https://gitlab.com/x86-psABIs/x86-64-ABI/-/commit/21622785649f2cbacb596d135a417171f52ad539 Note that there are some changes expected to that wording (the "bit-aligned" stuff should say "byte-aligned" and we're missing information about bit-fields), so if there are additional changes we'd like to see made, now's a good time to make them.

If you have comments or concerns with what's proposed there, I can pass along feedback to H.J. about potential changes we'd like to see. However, once it's in psABI, I'm not certain what other platform ABI groups even exist -- that's a bit out of my wheelhouse, so help getting those discussions moving would be very much appreciated.

> I think the most logical generic ABI is:
>
> - Let `MaxFundamentalWidth` be the (target-chosen) bit-width of the largest fundamental integer type that can be used to represent a `_BitInt`.  Typically this will be the largest integer type supported by the ABI, but some targets may want to use a smaller limit.  At the very least, this needs to be locked down in the ABI; the ABI for `_BitInt(256)` shouldn't change if the ABI adds new support for  an `int256_t` type.
> - Let `chunk_t` be the (target-chosen) fundamental integer type that will be used to store the components of a `_BitInt` that is wider than `MaxFundamentalWidth`.  This should be an integer type that the architecture comfortably supports overflow operations on, and it will typically be the full width of a GPR.
> - Let `ChunkWidth` be defined as `CHAR_BITS * sizeof(chunk_t)`.
> - If `N < RepWidth(N)`, then signed/unsigned `_BitInt(N)` has the same representation as `_BitInt(RepWidth(N))`, with the value held in the least significant bits and sign/zero-extended to the full width.  Values must be in the proper range of the original type; that is, it is undefined behavior if e.g. a `signed _BitInt(7)` does not hold a value in the range of `-64 ..< 63`.  Hereafter we will assume that `N == RepWidth(N)`.
> - If `N <= MaxFundamentalWidth`, then signed/unsigned `_BitInt(N)` has the same representation as the smallest fundamental integer type (least rank, if material) of at least `N` bits and the same signedness.  `_Bool` is not considered in this selection.
> - Otherwise, signed/unsigned `_BitInt(N)` has the same representation as a `struct` containing a single field of type `chunk_t[N / sizeof(chunk_t)]`, where the element at index `i` stores bits `i*ChunkWidth ..< (i+1)*ChunkWidth` of the integer.  That is, the array is always stored in little-endian order, which should have better memory-access properties regardless of the endianness of the target.  (The individual elements still have natural host endianness, of course.)

I believe that's what's already been proposed for psABI, but if I've missed something, please let me know.

> Some targets that don't pass/return small structs efficiently as a matter of course may want to treat smallish (but still non-fundamental) `_BitInt`s specially in their calling convention.
>
> I think using a uniform, GPR-sized chunk makes more sense than trying to use a smaller chunk that might eliminate some padding.  I could imagine that a 64-bit platform that supports 32-bit overflow checking might consider represent `_BitInt(96)` with three 32-bit chunks instead of two 64-bit chunks, and that would indeed make an array of them pack better, but it would also mean that e.g. addition would take three operations instead of two.
>
> You can't have bit-fields of `_BitInt` type, can you?  If people want that, or want a truly packed `_BitInt` where `sizeof(_BitInt(56)) == 7`, it's going to add a lot of complexity.

You can have bit-fields of _BitInt type (and we even tell you when you do dumb things with it https://godbolt.org/z/sTKKdb1o1), but I'm not seeing where the complexity comes in from that (there was a thought that referencing the existing bit-field wording with a mention about the width of _BitInts would be sufficient). But these are not truly packed -- `sizeof(_BitInt(56) == 8`.

================
Comment at: clang/include/clang/Basic/TargetInfo.h:581
   /// limitation is put into place for ABI reasons.
-  virtual bool hasExtIntType() const {
+  /// FIXME: _BitInt is a required type in C23, so there's not much utility in
+  /// asking whether the target supported it or not; I think this should be
----------------
jyknight wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > erichkeane wrote:
> > > > Concur on the fixme.  I would expect after this lands that an llvm-dev discussion happen to do this alert, and have us remove this pretty quickly (a release or two?)
> > > To clarify: This should be removed at the beginning of a release-cycle (along with an llvm-dev notice) so that we have as much time as possible in trunk to react/find issues.
> > We're basically at the beginning of Clang 14 (13 isn't out the door yet), so I am sort of tempted to alert llvm-dev now and remove it as part of this review. However, if people think that's too harsh, I'm happy to wait as well.
> Since we already return true for all in-tree targets, removing this is effectively a no-op.
> 
> However...
> 
> Previously, this was a clang extension, so the ABI didn't really matter, as long as it was functional and not broken. But now, it's a standard feature, which gives (or, at least SHOULD give) a much stronger expectation as to compiler-interoperability and ABI documentation.
> 
> So -- where's the ABI specification for how _BitInt is represented in memory (size, alignment, ordering/padding location), and how it's passed/returned from functions? Presumably that needs to be part of each platform's psABI -- ideally before the feature is enabled on each platform, right?
> 
> I realize that such idealism may be somewhat impractical, here in the real world, since some platforms' psABIs appear to be effectively unmaintained. But is it going to be added to all those which //are// being actively maintained?
> 
> It would be really quite lovely if this didn't end up like the situation we have with _Atomic. (Where the new feature was implemented and deployed by compilers, without ever hashing out ABI issues. And, so, _Atomic ABI remains undocumented and incompatible between implementations.)
> So -- where's the ABI specification for how _BitInt is represented in memory (size, alignment, ordering/padding location), and how it's passed/returned from functions? Presumably that needs to be part of each platform's psABI -- ideally before the feature is enabled on each platform, right?

https://gitlab.com/x86-psABIs/x86-64-ABI/-/commit/21622785649f2cbacb596d135a417171f52ad539 is a work in progress, so one step ahead on that.

> It would be really quite lovely if this didn't end up like the situation we have with _Atomic. (Where the new feature was implemented and deployed by compilers, without ever hashing out ABI issues. And, so, _Atomic ABI remains undocumented and incompatible between implementations.)

Agreed! WG14 asked me explicitly to start the ABI discussions now, and I've done so to the best of my ability. Unfortunately, I'm not an ABI expert and so I don't know who all to reach out to (I've talked to psABI and Itanium folks, that's it). Help in this area would be very much appreciated.

================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3958
+void CXXNameMangler::mangleType(const BitIntType *T) {
+  // FIXME: this is proposed to the Itanium ABI group but not yet accepted.
+  // 5.1.5.2 Builtin types
----------------
jyknight wrote:
> There ought to be a ItaniumDemangle.h change corresponding to this, too (As a separate review is fine, too.)
+1, I can do that as a follow-up.

================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:3338

-void MicrosoftCXXNameMangler::mangleType(const ExtIntType *T, Qualifiers,
+void MicrosoftCXXNameMangler::mangleType(const BitIntType *T, Qualifiers,
                                          SourceRange Range) {
----------------
jyknight wrote:
> It seems unlikely that this will be the official mangling for MS platforms, since it has a name __clang in it, right? How do we plan to deal with that future ABI issue?
With an ABI break. I waffled on this one -- we could leave it as `ExtInt` instead of `BitInt` and only deal with the ABI break once, but I believe the number of existing users of `_ExtInt` on Windows platforms to be vanishingly small enough to not count as much of an ABI break today.

As best I can tell, Microsoft sort of does their own thing for mangling whenever they get around to it, so my thinking was that doing the rename to BitInt now at least makes the mangling intelligible for users of the standard feature from Clang. But I don't feel strongly about it.

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

https://reviews.llvm.org/D108643