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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
