rsandifo-arm marked 7 inline comments as done. rsandifo-arm added a comment.
Thanks for the reviews! ================ Comment at: include/clang/Basic/AArch64SVEACLETypes.def:10 +// +// This file defines the database about various builtin singleton types. +// ---------------- rovka wrote: > You can be more specific :) Yeah, there were a few cut-&-pastos here, sorry. Hopefully fixed in the updated version. ================ Comment at: include/clang/Basic/AArch64SVEACLETypes.def:29 +#define SVE_VECTOR_TYPE(Name, Id, SingletonId, ElKind, ElBits, IsSigned, IsFP)\ + BUILTIN_TYPE(Name, Id, SingletonId) +#endif ---------------- rovka wrote: > Maybe use SVE_TYPE instead of BUILTIN_TYPE, to avoid any confusion? You > can't re-use a defition of BUILTIN_TYPE between this header and > BuiltinTypes.def anyway, since they both undefine it at the end. Yeah, that's definitely better, especially given the different number of arguments. Done in the updated version. ================ Comment at: lib/AST/ASTContext.cpp:6645 + case BuiltinType::Id: +#include "clang/Basic/AArch64SVEACLETypes.def" #define BUILTIN_TYPE(KIND, ID) ---------------- rovka wrote: > Here and in most other places: no need for 2 defines, you can just #define > BUILTIN_TYPE (or if you choose to rename it to SVE_TYPE) since it's the same > code for both vectors and predicates. Thanks, that shaved quite a lot of lines off the patch :-) ================ Comment at: lib/AST/ItaniumMangle.cpp:2680 + break; +#include "clang/Basic/AArch64SVEACLETypes.def" } ---------------- 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. ================ Comment at: lib/AST/MicrosoftMangle.cpp:2110 + llvm_unreachable("mangling an sve type not yet supported"); +#include "clang/Basic/AArch64SVEACLETypes.def" } ---------------- rovka wrote: > Should we have llvm_unreachable or report a proper error? I like the > unreachable if we're checking elsewhere that SVE isn't supported on Windows, > but I notice we report errors for some of the other types. Fixed to report the error, since this wouldn't be trapped earlier. ================ Comment at: lib/CodeGen/CGDebugInfo.cpp:709 + case BuiltinType::Id: \ + return nullptr; +#include "clang/Basic/AArch64SVEACLETypes.def" ---------------- 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. 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
