[PATCH] D62960: SVE opaque type for C intrinsics demo

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


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


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960