sdesmalen added a comment.
Thanks both for the detailed review and latest round of comments, I've tried to
address them all.
================
Comment at: clang/include/clang/AST/Type.h:3987
/// [implimits] 8 bits would be enough here.
- uint16_t NumExceptionType = 0;
+ uint16_t NumExceptionType;
+
----------------
aaron.ballman wrote:
> I think this should also be an `unsigned` bit-field so that it packs together
> with the new field added below. Otherwise we're liable to end up with padding
> between this field and the next bit-field (which will take a full allocation
> unit anyway). How about `unsigned NumExceptionType : 10;`? We're reducing the
> count by 6 bits, but.... I struggle to imagine code being impacted and we're
> still allowing more than the implementation limits require.
That's what I did initially in the patch where I limited `NumExceptionType` to
16bits (D152140), but there was the preference to have this be a uint16_t
instead. I've changed it back again to a bitfield though.
================
Comment at: clang/include/clang/Basic/Attr.td:2439
def ArmStreaming : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
let Spellings = [RegularKeyword<"__arm_streaming">];
----------------
rsandifo-arm wrote:
> In current main, this attribute is gated on TargetAArch64 rather than
> TargetAArch64SME. The reason is that, from a QoI point of view, I don't
> think we should require the `+sme` extension in order to accept an
> `__arm_streaming` type. `+sme` is instead required to compile a function
> with `__arm_streaming` type, or a call to a function with `__arm_streaming`
> type. IMO those are more CodeGen rather than Sema restrictions.
>
> E.g., if a header file contains
> ```
> void sme_foo(void) __arm_streaming;
> ```
> then an `__attribute__((target_version("sme")))` function should be able to
> call it. But we shouldn't require `+sme` to be passed on the command line in
> order to compile the declaration of `sme_foo`.
>
> I think the same principle applies to the other function-type attributes. In
> particular, `__arm_preserves_za` functions and `__arm_streaming_compatible`
> functions do not require SME to be present.
>
> I think we should also allow:
> ```
> __arm_locally_streaming void __attribute__((target_version("sme"))) ifunc() {
> … }
> ```
> since `__arm_locally_streaming` does not change the function signature.
>
> So I think all of these attributes should be gated on TargetAArch64, with
> checks for SME made elsewhere where necessary.
>
> (Unfortunately, since command-line options are out of scope for ACLE, I don't
> think it can really specify this.)
That's a fair point. This change was partially made in D152141, so I've updated
both patches to reflect the right behaviour.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6318
+def ArmSmeStreamingDocs : Documentation {
+ let Category = DocCatType;
+ let Content = [{
----------------
aaron.ballman wrote:
> There are enough of these attributes that I think we should consider adding
> an `ARM C Language Extensions for SME` documentation category:
> ```
> def DocCatACLE_SME : DocumentationCategory<"ARM C Language Extensions for
> SME"> {
> let Content = [{
> Put general information about what ACLE SME is here to give an overview of
> the functionality and a place to link to the official ACLE SME documentation
> for more in-depth details.
> }];
> }
> ```
> and then use this for the `Category` for each of the attributes so they group
> together.
@rsandifo-arm pointed out that I marked this comment as done, but hadn't done
it. I've fixed that now, and created a new category for the SME extensions.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3608-3609
+def err_sme_attr_mismatch : Error<
+ "function declared '%0' was previously declared '%1'"
+ " with different SME function attributes">;
def err_cconv_change : Error<
----------------
aaron.ballman wrote:
> rsandifo-arm wrote:
> >
> Also, remove the single quotes around %0 and %1 -- you're getting duplicate
> quotes in the test cases because of them.
Thanks! That explains the double quotes indeed.
================
Comment at: clang/lib/AST/TypePrinter.cpp:943
+ FunctionType::SME_PStateSMCompatibleMask) &&
+ !InsideCCAttribute)
+ OS << " __arm_streaming_compatible";
----------------
rsandifo-arm wrote:
> Are the `!InsideCCAttribute` conditions necessary? AIUI, InsideCCAttribute
> exists to suppress the default calling convention when printing the type to
> which an explicit CC is added. That doesn't apply to SME attributes, because
> they aren't modelled as traditional CCs, and because the default/normal case
> doesn't have any syntax associated with it.
You're right, they were not necessary! Thanks.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:9688
+ Changed == FunctionType::SME_PStateZAPreservedMask &&
+ (ToAttributes & FunctionType::SME_PStateZAPreservedMask);
+ return !DropsPreservesZA;
----------------
rsandifo-arm wrote:
> The meaning of “from” and “to” seem to be the opposite of what I'd initially
> assumed. For casts, I'd have expected the type of the object that is being
> casted to have FromType and the cast type (the type of the result) to be
> ToType. Dropping ZA would then mean that FromAttributes has
> `__arm_preserves_za` and ToType doesn't.
>
> I think that also makes sense for the virtual function check above. The
> overriding function has FromType and the function it overrides has ToType.
> We need to be able to convert from FromType to ToType (e.g. to initialise the
> vtable), but don't need to be able to go the other way.
Thanks for pointing out, that was indeed wrong. I've fixed it, and added a
test-case for the virtual function override for the __arm_preserves_za case.
================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:57
+
+// FIXME: Add invalid function pointer assignments such as assigning:
+// 1. A streaming compatible function to a normal function pointer,
----------------
rsandifo-arm wrote:
> Is it not possible to address this FIXME yet? Might be worth expanding the
> comment to say why if so.
This was a stale comment, these cases are checked later on in section:
//
// Check for incorrect conversions of function pointers with the attributes
//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127762/new/
https://reviews.llvm.org/D127762
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
