[PATCH] D62960: Add SVE opaque built-in types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm marked 7 inline comments as done.
rsandifo-arm added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:1974
+      break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
     }
----------------
rjmccall wrote:
> 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.
Yeah, in retrospect this is sorely lacking a comment.  The reason for using 16 is that there is one predicate bit for each vector byte, so the predicate size is a runtime multiple of 16 bits.  I've added a comment to say that and added a reference to the ABI that defines the alignments.


================
Comment at: lib/AST/ItaniumMangle.cpp:2680
+    break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
   }
----------------
rjmccall wrote:
> 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?
Yeah, we decided to stick with the 'u' as documented for SVE.  (It's too late for Advanced SIMD unfortunately.)


================
Comment at: lib/AST/MicrosoftMangle.cpp:2073
+#define SVE_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
   case BuiltinType::ShortAccum:
----------------
rjmccall wrote:
> Comment isn't adding anything here.
I've removed these comments from everything except Type.h, where having them matches the surrounding style.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:709
+  case BuiltinType::Id: \
+    return nullptr;
+#include "clang/Basic/AArch64SVEACLETypes.def"
----------------
rjmccall wrote:
> 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.
Hopefully it won't be too hacky, at least not in its final form. :-)  I've added a comment outlining how we handle debug info for these types, but explained why it isn't included yet.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960