[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman closed this revision. aaron.ballman added a comment. Thanks all! I've commit the renaming in 6c75ab5f66b403f7ca67e86aeed3a58abe10570b and have started a new review for the generic ABI document in https://review

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. I'm sorry for holding this up for so long by just not responding to your pings. Yes, I have no objection to you going forward with this patch, since we're still explicitly saying that it's not yet ABI-stable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1086

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#3158232 , @aaron.ballman wrote: > In D108643#3140927 , @aaron.ballman > wrote: > >> In D108643#3093323 , >> @aaron.ballman wro

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#3140927 , @aaron.ballman wrote: > In D108643#3093323 , @aaron.ballman > wrote: > >> In D108643#3106438 , >> @aaron.ballman wro

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#3093323 , @aaron.ballman wrote: > In D108643#3106438 , @aaron.ballman > wrote: > >> Ping > > Ping. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108643/

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. Ping 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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. Herald added a subscriber: asavonic. Ping. 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.or

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. Yes, I think it would be fine to fork off a conversation about what the ABI should be; I didn't meant to completely derail this patch. To be frank, my expectation was that more of this was already settled and documented, so we just needed to add that documentation and

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#3057417 , @rjmccall wrote: > I think the immediate way forward is that we can continue to document this as > an experimental feature with an unstable ABI, just with a new name. We do > not need to tie these thi

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. In D108643#3057417 , @rjmccall wrote: > In D108643#3055244 , @erichkeane > wrote: > ! In D108643#3054443 , @rjmccall wrote: >>

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

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 definiti

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3055244 , @erichkeane wrote: >>> ! In D108643#3054443 , @rjmccall >>> wrote: >>> ! In D108643#3027473 , @erichkeane wrote

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

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.

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. >> ! In D108643#3054443 , @rjmccall >> wrote: >> >>> ! In D108643#3027473 , >>> @erichkeane wrote: >>> ! In D108643#3026550 , @rjmccall

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3027473 , @erichkeane wrote: > In D108643#3026550 , @rjmccall > wrote: > >> I think it would be better to provide a generic ABI specification that is >> designed to "lower"

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. In D108643#3026550 , @rjmccall wrote: > I think it would be better to provide a generic ABI specification that is > designed to "lower" `_BitInt` types into more basic concepts that ABIs > typically already have rules for. I

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. I think it would be better to provide a generic ABI specification that is designed to "lower" `_BitInt` types into more basic concepts that ABIs typically already have rules for. It would be appropriate to note at the top that this is only a generic specification and

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. FYI: @craig.topper noticed my ping above and tells me he at least doesn't agree with my understanding of the hallway discussion we had a few years ago. I am hopeful he can correct my memory/interpretation of the conversation. CHANGES SINCE LAST ACTION https://rev

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. > Okay. Sorry if I came down on you personally, I know what it's like to be in > the middle on things like this Thank you, I very much appreciate that. > I'm not sure if there's a way to get LLVM to treat loaded values as only > having N valid bits. > > Do you have

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3000556 , @erichkeane wrote: > In D108643#3000540 , @rjmccall > wrote: > >> > > I don't work on the microcode, it is just what I was told when we asked about > this. SO un

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. In D108643#3000540 , @rjmccall wrote: > The question is whether you can rely on extension at places that receive an > arbitrary ABI-compatible value, like function parameters or loads. If nobody > who receives such a value c

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3000223 , @erichkeane wrote: > In D108643#3000193 , @rjmccall > wrote: > >> In D108643#2999776 , @erichkeane >> wrote: >> ! In

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added subscribers: craig.topper, andrew.w.kaylor. erichkeane added a comment. In D108643#3000193 , @rjmccall wrote: > In D108643#2999776 , @erichkeane > wrote: > >>> ! In D108643#2965852

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#2999776 , @erichkeane wrote: >> ! In D108643#2965852 , @rjmccall >> wrote: >> >> The choice that high bits are unspecified rather than extended is an >> interesting one. Ca

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. > ! In D108643#2965852 , @rjmccall > wrote: > > The choice that high bits are unspecified rather than extended is an > interesting one. Can you speak to that? That's good for +, -, *, &, |, ^, > <<, and narrowing conversion

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

hjl.tools added a comment. In D108643#2999724 , @aaron.ballman wrote: > In D108643#2991995 , @hjl.tools > wrote: > The choice that high bits are unspecified rather than extended is an interesting one.

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#2991995 , @hjl.tools wrote: >>> The choice that high bits are unspecified rather than extended is an >>> interesting one. Can you speak to that? That's good for +, -, *, &, |, ^, >>> <<, and narrowing conversi

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

hjl.tools added a comment. >> The choice that high bits are unspecified rather than extended is an >> interesting one. Can you speak to that? That's good for +, -, *, &, |, ^, >> <<, and narrowing conversions, but bad for ==, <, /, >>, and widening >> conversions. > > I've added @hjl.tools to

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a reviewer: hjl.tools. aaron.ballman added a subscriber: hjl.tools. aaron.ballman added a comment. In D108643#2965852 , @rjmccall wrote: > In D108643#2964740 , @aaron.ballman > wrote: > >> In

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#2964740 , @aaron.ballman wrote: > In D108643#2964190 , @rjmccall > wrote: > >> I agree with James; I know you've reached out to the Itanium ABI group about >> mangling, but

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#2964740 , @aaron.ballman wrote: > 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-AB

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In 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 repre

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. 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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

jyknight added inline comments. 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 whethe

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. LGTM, pending some level of Itanium ABI blessing. 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.llv

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added inline comments. Comment at: clang/docs/ReleaseNotes.rst:145 +- The mangling of the ``_ExtInt(N)`` datatype has changed in both the Microsoft + ABI and Itanium ABI. erichkeane wrote: > Hrm... not a huge fan that this still claims that ``_Ex

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added a comment. Please feel free to wordsmith my ABI change comment. I don't feel strongly other than trying to make it clear that `_ExtInt` is no longer a type/types. A question for other reviewers: Do we feel strongly enough to try to keep the old mangling around via the `clang-

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1489 + InGroup>; +def warn_prec2x_compat_bit_int : Warning< + "'_BitInt' is incompatible with C standards before C2x">, erichkeane wrote: > should this be warn_cp

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:107 +- Clang now supports the _BitInt(N) family of bit-precise integer types from + C23. This type was previously exposed as _ExtInt(N), which is still supported This section should prob

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman removed subscribers: aheejin, MaskRay, jholewinski, dschuff, sdardis, nemanjai, jvesely, nhaehnle, sbc100, jgravelle-google, kbarton, fedor.sergeev, kosarev, asb, rbar, johnrusso, simoncook, sabuasal, niosHD, martong, jrtc27, zzheng, edward-jones, atanasyan, rogfer01, MartinMosbeck