rjmccall added inline comments.
================ Comment at: lib/AST/ItaniumMangle.cpp:2680 + break; +#include "clang/Basic/AArch64SVEACLETypes.def" } ---------------- 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. 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
