[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/docs/BitIntABI.rst:90
+width. e.g., a ``signed _BitField(7)`` whose representation width is ``8`` bits
+cannot store values less than ``-64`` or greater than ``63``.
+
----------------
It might be worthwhile to pull these definitions into a common Definitions section, since I think they'll apply in most of the sections.  You should be explicit about the two parameters, like:

> This generic ABI is expressed in terms of two parameters which must be determined by the platform ABI:
>
> - `MaxFundamentalWidth` is the bit-width...
>
> - `chunk_t` is the type of...
>
> For the convenience of the following specification, other quantities are derived from these:

The paragraph about `RepWidth` could be clearer.  Maybe:

> ``RepWidth(N)`` is the *representation width* of a ``_BitInt`` of width ``N``.  If ``N <= MaxFundamentalWidth``, then ``RepWidth(N)`` is the bit-width of the lowest-rank fundamental integer type (excluding ``_Bool``) with at least ``N`` bits.  Otherwise, ``RepWidth(N)`` is the least multiple of the bit-width of ``chunk_t`` which is at least ``N``.  It is therefore always true that ``N <= RepWidth(N)``.  When ``N < RepWidth(N)``, a ``_BitInt(N)`` is represented exactly as if it were extended to a ``_BitInt(RepWidth(N))`` of  the same signedness.  The value is held in the least significant bits, and the excess (most significant) bits have unspecified values.

The sentence about undefined behavior conflicts with the excess bits taking unspecified values.


================
Comment at: clang/docs/BitIntABI.rst:145
+assumption that it improves performance for a larger set of operations than
+sign or zero extension.
----------------
Hmm.  How about:

> Excess bits
> -----------
> 
> When ``N < RepWidth(N)``, the ABI has three natural alternatives:
> - The value is kept in the least-significant bits, and the excess (most significant) bits are unspecified.
> - The value is kept in the least-significant bits, and the excess (most significant) bits are required to be a proper zero- or sign-extension of the value (as appropriate for the signedness of the type).
> - The value is left-shifted into the most-significant bits, and the excess (least significant) bits are required to be zero.
>
> Each of these has trade-offs.  Leaving the most-significant bits unspecified allows addition, subtraction, multiplication, bitwise complement, left shifts, and narrowing conversions to avoid adjusting these bits in their results.  Forcing the most-significant bits to be properly extended allows comparisons, division, right shifts, and widening conversions to avoid adjusting these bits in their operands.  Keeping the value left-shifted is good for both addition and comparison, but other operations (especially conversions) become more complex, and the representation is less "natural", which can complicate interacting with other systems.  Furthermore, having unspecified bits means that bitwise equality can be false even when semantic equality holds, but not having unspecified bits means that there are "trap representations" which can lead to undefined behavior.
>
> This ABI leaves the most-significant bits unspecified out of a belief that doing so should optimize the most common operations and avoid the most complexity in practice.


================
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``.
----------------
aaron.ballman wrote:
> 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.
Right, it's fine to start by just documenting what we've got.


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

https://reviews.llvm.org/D108643