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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits