erichkeane added inline comments.
================
Comment at: include/clang/Basic/Attr.td:851
+ let Spellings = [GCC<"cpu_specific">];
+ let Args = [VariadicIdentifierArgument<"Cpus">];
+ let Subjects = SubjectList<[Function]>;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > Be sure to add a test for using this attribute with the C++ spelling, as
> > > I'm not certain how well we would parse something like
> > > `[[gnu::cpu_specific(ivybridge)]]` currently (it may just work, however).
> > >
> > > Also, why an identifier instead of a string literal?
> > I'll add it, presumably as 'clang::cpu_specific'. The decision for
> > string-literal vs identifier was made quite a few years before I was here
> > sadly. I believe the EDG FE doesn't make identifiers any more difficult so
> > the implementer here chose to make it that way.
> >
> > In this case, I'd very much like to keep it with the same implementation as
> > ICC, simply because users of this are already familiar with it in this form.
> > In this case, I'd very much like to keep it with the same implementation as
> > ICC, simply because users of this are already familiar with it in this form.
>
> The compatibility with ICC is important for the GNU-style attribute, but for
> the C++ spelling this is novel territory where there is no compatibility
> story. Another approach to consider is whether to accept identifiers or
> string literals depending on the spelling, but that might not be worth it.
I'd like to think about that... I could forsee accepting BOTH forms, simply
because it would slightly simplify the conversion from an attribute-target-mv
situation, though I'm not sure it is important enough to do.
================
Comment at: include/clang/Basic/AttrDocs.td:225-226
+as a parameter (like ``cpu_specific``). However, ``cpu_dispatch`` functions
+may not have a body in its definition. An empty definition is permissible for
+ICC compatibility, and all other definitions will have their body ignored.
+
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > How empty is empty? e.g., is it lexically empty (no tokens whatsoever) or
> > > notionally empty (empty after preprocessing)?
> > > ```
> > > __attribute__((cpu_dispatch(atom))) void multi_cpu_1(void) {
> > > /* Is this empty enough? */
> > > }
> > >
> > > __attribute__((cpu_dispatch(atom))) void multi_cpu_2(void) {
> > > #if 0
> > > #error How about this?
> > > #endif
> > > }
> > > ```
> > Well, ANY are permissible (even non-empty bodies...) since I issue a
> > warning if I detect that it is not empty. I detect it at the AST level, so
> > anything that doesn't add to the AST isn't counted against 'empty'. In
> > this case, those two are both empty.
> >
> > Do you have a suggestion on how I can word this more clearly?
> Would this be accurate?
> ```
> Functions marked with ``cpu_dispatch`` are not expected to be defined, only
> declared. If such a marked function has a definition, any side effects of the
> function are ignored; trivial function bodies are permissible for ICC
> compatibility.
> ```
> (Question: if this is true, should you also update
> `FunctionDecl::hasTrivialBody()` to return `true` for functions with this
> attribute?)
That is accurate and elegantly describes the behavior. I don't think it makes
sense to mark the body 'trivial' since it actually WILL have a body, its just
emitted based on the attribute rather than the user-defined body. I'd also
fear some of the side-effects of permitting someone to detect it as 'trivial'
for SFINAE purposes.
https://reviews.llvm.org/D47474
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
