[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

Mon Oct 11 01:18:36 PDT 2021

rjmccall 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.
+
----------------
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.)

================
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.
+
----------------
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.

================
Comment at: clang/docs/BitIntABI.rst:47
+The ABI of ``_BitInt`` is expected to vary between architectures, but the
+following is a general ABI outline.
+
----------------
In general, this section is a good start, but I think you should split it into three sections:
- one section describing how `_BitInt` is laid out as a non-bit-field object;
- one section describing how a `_BitInt` is laid out as a bit-field, which I think needs to be much clearer than you've given below; and
- one section describing how a `_BitInt` is passed and returned from a function.

================
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``.
+
----------------
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.

================
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``.
----------------
BitInts not packing with non-BitInts is a surprising rule.

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

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

https://reviews.llvm.org/D108643