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