aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Type.h:4064
bool HasTrailingReturn : 1;
+ unsigned AArch64SMEAttributes : 8;
Qualifiers TypeQuals;
----------------
If we're taking up space in the extra bitfields, why do we need to also take up
space in the function prototype itself?
================
Comment at: clang/include/clang/Basic/Attr.td:2333
+def ArmStreamingCompatible : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
+ let Spellings = [GNU<"arm_streaming_compatible">];
----------------
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.)
================
Comment at: clang/include/clang/Basic/Attr.td:2345
+
+def ArmLocallyStreaming : DeclOrStmtAttr, TargetSpecificAttr<TargetAArch64SME>
{
+ let Spellings = [GNU<"arm_locally_streaming">];
----------------
Why is this a *statement* attribute?
================
Comment at: clang/include/clang/Basic/Attr.td:2322
+def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64>
{
+ let Spellings = [Clang<"arm_streaming_compatible">];
----------------
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?
================
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 "
----------------
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.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3542-3543
+def err_sme_attr_mismatch : Error<
+ "function declared '%0' was previously declared '%1'"
+ " with different SME function attributes.">;
def err_cconv_change : Error<
----------------
No trailing full stop in Clang diagnostics
================
Comment at: clang/lib/Sema/SemaType.cpp:7676
+ return false;
+ }
+
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > sdesmalen wrote:
> > > aaron.ballman wrote:
> > > > Unfortunately, type attributes do not yet have any of the automated
> > > > checking generated by tablegen, so there are no calls to
> > > > `Sema::checkCommonAttributeFeatures()`, which means you have to
> > > > manually check things like mutual exclusions, not accepting arguments,
> > > > etc.
> > > After some experimenting I found that:
> > > * When I add the `MutualExclusions` to Attr.td and compile `typedef
> > > __attribute__((arm_streaming, arm_streaming_compatible)) void (*ptrty1)
> > > (void);`, I still get the diagnostic for conflicting attributes 'for
> > > free', even without any code being added here to check for conflicting
> > > attributes. However, when I then apply it to a declaration (e.g. `void
> > > __attribute__((arm_streaming, arm_streaming_compatible)) foo(void);`), I
> > > get the diagnostics twice.
> > > * When I add some code here to mark the attribute as invalid, I get no
> > > diagnostic whatsoever unless I explicitly emit it here, rendering the use
> > > of `MutualExclusions` in Attr.td ineffective.
> > >
> > > Based on the above observation, I've chosen to keep the code in
> > > SemaDeclAttr.cpp and not use the `MutualExclusions`, because it generates
> > > the best diagnostics (an error + a note) and only emits it once. Let me
> > > know what you think.
> > Yeah, I can see why we'd hit that problem -- when we go to form the
> > function type to be able to make the declaration, we'd issue the warning
> > once, and then after forming the declaration and trying to apply attributes
> > to it, we'd issue the warning a second time. We do not have any other
> > `DeclOrTypeAttr` which has mutual exclusions.
> >
> > I don't want to lose the declarative nature of using `MutualExclusions` in
> > Attr.td unless there's no other choice; that's a helpful piece of
> > documentation to reviewers when reviewing additional attributes in the
> > future. If you elect to keep the attribute as a declaration and type
> > attribute, I think you'll need to try to fix this to work properly.
> By changing the attributes to become `TypeAttr`, the `MutualExclusions` no
> longer kick in at all (you mentioned this in a previous comment), so I've had
> to keep the manual code to check the attributes here. Are you okay with that?
Yeah, that is reasonable, thanks for checking it!
================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:213
+template short templated<short>(short);
+#endif
+
----------------
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?
}
```
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
