[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

[James Y Knight via Phabricator via cfe-commits]

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 whether the target supported it or not; I think this should be
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > Concur on the fixme.  I would expect after this lands that an llvm-dev discussion happen to do this alert, and have us remove this pretty quickly (a release or two?)
> > To clarify: This should be removed at the beginning of a release-cycle (along with an llvm-dev notice) so that we have as much time as possible in trunk to react/find issues.
> We're basically at the beginning of Clang 14 (13 isn't out the door yet), so I am sort of tempted to alert llvm-dev now and remove it as part of this review. However, if people think that's too harsh, I'm happy to wait as well.
Since we already return true for all in-tree targets, removing this is effectively a no-op.

However...

Previously, this was a clang extension, so the ABI didn't really matter, as long as it was functional and not broken. But now, it's a standard feature, which gives (or, at least SHOULD give) a much stronger expectation as to compiler-interoperability and ABI documentation.

So -- where's the ABI specification for how _BitInt is represented in memory (size, alignment, ordering/padding location), and how it's passed/returned from functions? Presumably that needs to be part of each platform's psABI -- ideally before the feature is enabled on each platform, right?

I realize that such idealism may be somewhat impractical, here in the real world, since some platforms' psABIs appear to be effectively unmaintained. But is it going to be added to all those which //are// being actively maintained?

It would be really quite lovely if this didn't end up like the situation we have with _Atomic. (Where the new feature was implemented and deployed by compilers, without ever hashing out ABI issues. And, so, _Atomic ABI remains undocumented and incompatible between implementations.)


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3958
+void CXXNameMangler::mangleType(const BitIntType *T) {
+  // FIXME: this is proposed to the Itanium ABI group but not yet accepted.
+  // 5.1.5.2 Builtin types
----------------
There ought to be a ItaniumDemangle.h change corresponding to this, too (As a separate review is fine, too.)


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:3338
 
-void MicrosoftCXXNameMangler::mangleType(const ExtIntType *T, Qualifiers,
+void MicrosoftCXXNameMangler::mangleType(const BitIntType *T, Qualifiers,
                                          SourceRange Range) {
----------------
It seems unlikely that this will be the official mangling for MS platforms, since it has a name __clang in it, right? How do we plan to deal with that future ABI issue?


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

https://reviews.llvm.org/D108643