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