[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

Mon Oct 11 09:44:55 PDT 2021

aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.

================
Comment at: clang/docs/BitIntABI.rst:17
+This is only intended as a generic specification. The actual details for any
+particular platform should be codified in that platform's ABI specification.
+
----------------
rjmccall wrote:
> I think you should recommend that ABIs codify that by reference to your specification.  Perhaps:
> 
> > This document provides a generic ABI specification for the C23 `_BitInt` feature, expressed in terms of concepts common to low-level platform ABIs.  This document does not directly constrain any platform, as the ABI used for `_BitInt` on a platform is determined by that platform's ABI specification.  It is expected that platform ABIs will codify the ABI for the `_BitInt` feature on their platform using this specification, either by reference or by copying the text of this specification into the platform ABI specification.
> 
> Now, I am assuming that this specification will be complex enough that ABI maintainers won't want to just copy into their specifications.  Maybe I'm wrong about that, but I think it will be fairly long by the time it's finished — not, like, Itanium C++ ABI long, but it might be 3-4 pages.  If we want to support including this by reference, we need to think about what that reference looks like.  I think there are two key parts of that: first, it should explicitly name a version of this document, and second, it should give  the parameters that we've listed out below.  Basically, someone should be able to see that and have no doubt about what the ABI is.
> 
> I would suggest this recommended reference:
> 
> > Platform ABIs which include this specification by reference should explicitly give a version and the parameters listed below.  For example: "The ABI for `_BitInt` types is specified by version 1 of the specification found at https://clang.llvm.org/docs/BitIntABI.html, using 64 as the maximum fundamental width and `long` as the chunk  type."
> 
> To make versioning work, the version history should (1) explicitly say what version this is and (2) be limited to major semantic versions, where you actually change the intended ABI rather than just making editorial changes.  (If you never need to do that, great, but you should set up to make it possible.)
Thanks, these are good suggestions; I've applied them.

================
Comment at: clang/docs/BitIntABI.rst:42
+``sizeof(Array) / sizeof(__BitInt(N))`` must have the usual semantics for
+determining the number of elements in an array.
+
----------------
rjmccall wrote:
> On first read, I thought this paragraph was telling me that `sizeof` behaves differently for `_BitInt` objects.  On second read, I think you're trying to tell me that it *doesn't* behave differently and that arrays of `_BitInt` might contain padding between elements.  However, that's true of all objects of `_BitInt` that aren't bit-fields, so I think this paragraph is a little misleading.
> 
> Perhaps:
> 
> > `_BitInt` types are ordinary object types and may be used anywhere an object type can be, such as a struct field, union field, or array element.  In addition, they are integer types and may be used as the type of a bit-field.  Like any other type, a `_BitInt` object may require more memory than its stated bit width in order to satisfy the requirements of byte (or higher) alignment.  In other words, the width of a `_BitInt` affects the semantics of operations on the value; it is not a guarantee that the value will be "packed" into exactly that many bits in memory.
You're correct -- thanks, your new words are far more clear than my old ones.

================
Comment at: clang/docs/BitIntABI.rst:71
+the original width. e.g., a ``signed _BitField(7)`` whose representation width
+is ``8`` bits cannot store values less than ``-64`` or greater than ``63``.
+
----------------
rjmccall wrote:
> Oh, is sign/zero extension the ABI you want to recommend?  I wasn't trying to lead you in either direction on this point; I just wanted to make sure this was thought through.
> 
> There should be a non-normative rationale / alternatives considered section at the end that makes an argument for the ABI's recommendations here.
> Oh, is sign/zero extension the ABI you want to recommend? I wasn't trying to lead you in either direction on this point; I just wanted to make sure this was thought through.

We're still nailing this bit down. I personally do not care what the answer is, and I have no idea how we would ever measure to determine what the "right" answer is because. My understanding of the usage pattern for this type is people expect to use it similar to `int`, so it'll be used for basically all of the operations roughly equally. Based on that, I don't think there is a right answer. So I'm switching back to "unspecified" on the assumption that it gives better performance for a larger set of operations (I'm assuming people "do math" with these types more than they "do logic" (comparisons) with them). But I'll definitely add information to the rationale section.

> There should be a non-normative rationale / alternatives considered section at the end that makes an argument for the ABI's recommendations here.

Can do.

================
Comment at: clang/docs/BitIntABI.rst:90
+``_BitInt`` should not be packed together with an adjacent bit-field of a non-
+``_BitInt`` type. ``_BitInt(N)`` and ``_BitInt(M)`` types should pack together
+when ``N != M``.
----------------
rjmccall wrote:
> BitInts not packing with non-BitInts is a surprising rule.
> 
> How does this packing work when allocation units differ in size?
> BitInts not packing with non-BitInts is a surprising rule.

Hmmm, I thought I had tested and found that unused bits from an oversized _BitInt would not pack together, but now I can't seem to get that behavior. It looks like we *do* pack things into the unused bits. https://godbolt.org/z/4Pavvbeqe

It seems to me that there's no reason to NOT pack into those unused bits, so I think that's the behavior we'd like. I'll correct the docs.

> How does this packing work when allocation units differ in size?

Are you wondering about a case like?
```
struct S {
  unsigned _BitInt(33) i : 8; // 8 byte allocation unit
  unsigned _BitInt(32) j : 8; // 4 byte allocation unit
};
```
or something else? I think the current behavior here is largely as a result of code review feedback on the original implementation work. At least, this patch isn't changing the behavior from whatever was deemed appropriate for `_ExtInt` originally. But we should find those rules out and document them here in the ABI doc.

================
Comment at: clang/docs/ReleaseNotes.rst:131
+  ``_BitInt(N)`` is supported as an extension in older C modes and in all C++
+  modes.
+
----------------
rjmccall wrote:
> Should we document that this is still considered an experimental feature, in the sense of not yet officially having a stable ABI?
Not a bad idea, I've added some additional words here, in the ABI section, and in LanguagateExtensions.rst so the information is widely available.

================
Comment at: clang/docs/ReleaseNotes.rst:166
+  previously used the ``_ExtInt`` types will now be mangled to instead use the
+  ``_BitInt(N)`` spelling in both the Microsoft and Itanium ABIs.
+
----------------
rjmccall wrote:
> Probably more accurate to now say something like:
> 
> ```
> The ``_ExtInt(N)`` extension has been standardized in C2X as ``_BitInt(N)``.  The mangling of this type in C++ has accordingly changed: under the Microsoft ABI it is now mangled using the ``_BitInt`` spelling, and under the Itanium ABI it is now mangled using a dedicated production.
> ```
SGTM

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

https://reviews.llvm.org/D108643