sdesmalen added a comment.
Thanks for your patience reviewing this patch @aaron.ballman!
================
Comment at: clang/include/clang/AST/Type.h:4064
bool HasTrailingReturn : 1;
+ unsigned AArch64SMEAttributes : 8;
Qualifiers TypeQuals;
----------------
aaron.ballman wrote:
> If we're taking up space in the extra bitfields, why do we need to also take
> up space in the function prototype itself?
This field is added to ExtProtoInfo struct, which is used to populate the
ExtraBitfields in the `FunctionPrototype`'s AST node.
Sorry if this wasn't clear because the diff had no context. `arc diff` wasn't
working for me, so manually updated the patch, but then forgot to add full
context.
================
Comment at: clang/include/clang/Basic/Attr.td:2333
+def ArmStreamingCompatible : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
+ let Spellings = [GNU<"arm_streaming_compatible">];
----------------
aaron.ballman wrote:
> You are handling these as declaration attributes in SemaDeclAttr.cpp but
> declaring them to be type attributes here in Attr.td; that's not okay. If
> these are type attributes, there should not be code for them in
> SemaDeclAttr.cpp. (This is why I really think the design needs to be
> rethought; I think they should be type attributes only, but I think you're
> trying to match what your specification says.)
Okay, I've removed the handling in SemaDeclAttr, so that it is only handled in
SemaType.cpp.
But it still allows the attribute to be applied to any position in the function
declaration, i.e.
__attribute__((arm_streaming)) float foo1(int);
float __attribute__((arm_streaming)) foo2(int);
float foo3(int) __attribute__((arm_streaming));
Is that expected?
================
Comment at: clang/include/clang/Basic/Attr.td:2345
+
+def ArmLocallyStreaming : DeclOrStmtAttr, TargetSpecificAttr<TargetAArch64SME>
{
+ let Spellings = [GNU<"arm_locally_streaming">];
----------------
aaron.ballman wrote:
> Why is this a *statement* attribute?
You're right that it shouldn't be. I want it to just be a decl attribute and
don't want it to be either a Type or a Stmt attribute, but that doesn't seem to
exist.
I find their meaning quite confusing, because both allow me to limit their
applicability to Functions. What is the right class to derive from here?
================
Comment at: clang/include/clang/Basic/Attr.td:2322
+def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64>
{
+ let Spellings = [Clang<"arm_streaming_compatible">];
----------------
aaron.ballman wrote:
> sdesmalen wrote:
> > aaron.ballman wrote:
> > > sdesmalen wrote:
> > > > aaron.ballman wrote:
> > > > > `DeclOrTypeAttr` is only intended to be used for attributes which
> > > > > were historically a declaration attribute but are semantically type
> > > > > attributes (like calling conventions). It seems to me that each of
> > > > > these is purely a type attribute and we shouldn't allow them to be
> > > > > written in the declaration position. WDYT?
> > > > In this case, I believe the attribute is valid on both the type and the
> > > > declaration, because the SME ACLE explicitly specifies that the
> > > > attributes can be applied to both declarations and types.
> > > >
> > > > The
> > > > [[https://github.com/ARM-software/acle/pull/188/commits/ce878cf6c43fd824ffc634526e5dfec1ddc1839e#diff-516526d4a18101dc85300bc2033d0f86dc46c505b7510a7694baabea851aedfaR8283-R8294|specification]]
> > > > says:
> > > > ```Some of the attributes described in this section apply to function
> > > > types.
> > > > Their semantics are as follows:
> > > >
> > > > * If the attribute is attached to a function declaration or
> > > > definition,
> > > > it applies to the type of the function.
> > > >
> > > > * If the attribute is attached to a function type, it applies to that
> > > > type.
> > > >
> > > > * If the attribute is attached to a pointer to a function type, it
> > > > applies
> > > > to that function type.
> > > >
> > > > * Otherwise, the attribute is [ill-formed](#ill-formed).```
> > > > In this case, I believe the attribute is valid on both the type and the
> > > > declaration, because the SME ACLE explicitly specifies that the
> > > > attributes can be applied to both declarations and types.
> > >
> > > What are the chances that can change? Because there are problems here:
> > >
> > > > If the attribute is attached to a function declaration or definition,
> > > > it applies to the type of the function.
> > >
> > > This is egregiously opposed to the design of `[[]]` attributes in both C
> > > and C++. We came up with `DeclOrTypeAttr` for attributes that previously
> > > existed, but that is different than introducing new attributes. It's par
> > > for the course for `__attribute__` style attributes, so I don't worry so
> > > much there.
> > >
> > > What's the rationale for this confusing of a design? (e.g., is there some
> > > reason you basically have to do that, like historically accepting the
> > > attributes in both positions?)
> > The attribute must always apply to the type of the function, because we
> > can't have the streaming-property (or the properties on ZA) being lost
> > between function overrides or function pointer assignments. It's perhaps
> > similar to a calling convention, because the caller may have to set up
> > streaming- or ZA state before the call, and restore state after the call.
> >
> > I'm not too familiar with the different spellings/syntaxes and their
> > implications, so I've now limited the attribute to `GNU` style attributes
> > (`__attribute__((..))`) and to being a `TypeAttr`, with the exception of
> > the `arm_locally_streaming` attribute, because that one can only be applied
> > to function definitions and is not part of the type.
> >
> > I've also added some new tests to ensure the properties are correctly
> > propagated (using `decltype()`) and tests to ensure virtual function
> > overloads require the same attributes.
> >
> > Is this a step in the right direction? :)
> Switching to a GNU-only spelling sort of helps, but I still think this design
> is confused.
> ```
> * If the attribute is attached to a function declaration or definition,
> it applies to the type of the function.
> ```
> This seems like a misdesigned feature and I think it should be fixed in the
> specification. If it's an attribute that applies to the type of the function,
> you should not be allowed to specify it on the declaration; what is the
> benefit to allowing it in an unexpected position?
> If it's an attribute that applies to the type of the function, you should not
> be allowed to specify it on the declaration
Could you maybe give me an example here of what it looks like to apply the
attribute to the function *type* when it's not allowed on the *declaration*?
Does this all have to do with the //position// of the attribute in the
declaration statement? When writing a function declaration it defines both the
name and the type of that function, so there's no way to write a declaration
without also defining its type.
Maybe it helps if you have any pointers that explain the difference in syntax
between function declaration/function type attributes
(https://clang.llvm.org/docs/AttributeReference.html doesn't really address
this)
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2000
"overridden virtual function is here">;
+def err_conflicting_overriding_attributes : Error<
+ "virtual function %0 has different attributes "
----------------
aaron.ballman wrote:
> This error and the one below make me wonder whether an attribute spelling is
> the appropriate way to surface this functionality as opposed to a keyword.
> Attributes don't typically don't cause errors in these situations, but
> because these attributes are strongly coupled to their type system effects I
> can see why you want these to be errors.
> This error and the one below make me wonder whether an attribute spelling is
> the appropriate way to surface this functionality as opposed to a keyword.
I'm not sure I understand what you mean, do you have an example?
================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:213
+template short templated<short>(short);
+#endif
+
----------------
aaron.ballman wrote:
> This is an interesting start, but here are some more cases:
> ```
> template <typename Ty>
> struct S {
> static constexpr int value = 0;
> };
>
> template <>
> struct S<void (*)()> {
> static constexpr int value = 1;
> };
>
> template <>
> struct S<void (* __attribute__((arm_streaming)))()> {
> static constexpr int value = 2;
> };
>
> void func() __attribute__((arm_streaming)) {}
> void other_func() {}
>
> static_assert(C<decltype(+func)>::value == 2, "why are we picking the wrong
> specialization?");
> static_assert(C<decltype(+other_func)>::value == 1, "why are we picking the
> wrong specialization?");
> ```
> another interesting test would be to ensure that implicit instantiations do
> not lose the attribute from the primary template for codegen:
> ```
> template <typename Ty>
> void func(Ty) __attribute__((arm_streaming)) {}
>
> int main() {
> func(12); // Does this properly insert the correct IR instructions?
> }
> ```
Great suggestions, thanks! They both seem to be handled correctly.
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
