rjmccall added inline comments.
================
Comment at: lib/AST/ASTContext.cpp:1974
+ break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
}
----------------
Why do SVE predicates have 16-bit alignment? Should this be 128-bit
(16-*byte*)?
I guess these alignments are reasonable to hard-code here since they're
target-specific for now. That might be worth including in the comment.
================
Comment at: lib/AST/ItaniumMangle.cpp:2680
+ break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
}
----------------
rjmccall wrote:
> rsandifo-arm wrote:
> > rovka wrote:
> > > erik.pilkington wrote:
> > > > rsandifo-arm wrote:
> > > > > erik.pilkington wrote:
> > > > > > jfb wrote:
> > > > > > > @rjmccall you probably should review this part.
> > > > > > Sorry for the drive by comment, but: All of these mangling should
> > > > > > really be using the "vendor extension" production IMO:
> > > > > >
> > > > > > `<type> ::= u <source-name>`
> > > > > >
> > > > > > As is, these manglings intrude on the users's namespace, (i.e. if
> > > > > > they had a type named `objc_selector` or something), and confuse
> > > > > > demanglers which incorrectly assume these are substitutable (vendor
> > > > > > extension builtin types are substitutable too though, but that
> > > > > > should be handled here).
> > > > > It isn't obvious from the patch, but the SVE names that we're
> > > > > mangling are predefined names like __SVInt8_t. rather than
> > > > > user-facing names like svint8_t The predefined names and their
> > > > > mangling are defined by the platform ABI
> > > > > (https://developer.arm.com/docs/100986/0000), so it wouldn't be valid
> > > > > for another part of the implementation to use those names for
> > > > > something else.
> > > > >
> > > > > I realise you were making a general point here though, sorry.
> > > > >
> > > > The mangling in the document you linked does use the vendor extension
> > > > production here though, i.e. the example is `void f(int8x8_t)`, which
> > > > mangles to _Z1f**u10__Int8x8_t**. It is true that this shouldn't ever
> > > > collide with another mangling in practice, but my point is there isn't
> > > > any need to smuggle it into the mangling by pretending it's a user
> > > > defined type, when the itanium grammar and related tools have a special
> > > > way for vendors to add builtin types.
> > > I agree with Erik here, the example in the PCS document seems to suggest
> > > using u. I think either the patch needs to be updated or the document
> > > needs to be more clear about what the mangling is supposed to look like.
> > Thanks for highlighting this problem, and sorry for not noticing myself
> > when pointing you at the doc.
> >
> > Unfortunately, the specification and implementation already difer for the
> > Advanced SIMD types, with both clang and GCC omitting the 'u' despite the
> > spec saying it should be present. So we're considering changing the spec
> > to match what's now the de facto ABI.
> >
> > For SVE we do still have the opportunity to use 'u'. I've left it as-is
> > for now though, until we've reached a decision about whether to follow
> > existing practice for Advanced SIMD or whether to do what the spec says.
> These do seem more "builtin" than the SIMD types, but I don't think it deeply
> matters either way, since these are already reserved names.
Did you actually make the decision to use the `u` mangling?
================
Comment at: lib/AST/MicrosoftMangle.cpp:2073
+#define SVE_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
case BuiltinType::ShortAccum:
----------------
Comment isn't adding anything here.
================
Comment at: lib/AST/NSAPI.cpp:487
+#define SVE_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
case BuiltinType::BoundMember:
----------------
In this and other places, please follow nearby style about whether to place the
`case` expansion on the following line, as is done above for the OpenCL
extension types.
================
Comment at: lib/CodeGen/CGDebugInfo.cpp:709
+ case BuiltinType::Id: \
+ return nullptr;
+#include "clang/Basic/AArch64SVEACLETypes.def"
----------------
rsandifo-arm wrote:
> rovka wrote:
> > I don't really know this code, but I can't help but notice that nullptr is
> > only ever used for the void type. Is it safe to also use it for the SVE
> > types, or can we do something else instead?
> Fixed to report an error here too and return a "safe" value until the TODO is
> fixed. Also added a test.
This is fine as long as you're planning to at least fill in a hacky
implementation when you implement the rest of IRGen support.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62960/new/
https://reviews.llvm.org/D62960
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
