[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Erich Keane via Phabricator via cfe-commits]

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