[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Responding to comments :)



================
Comment at: include/clang/Basic/Attr.td:850
+def CpuSpecific : InheritableAttr {
+  let Spellings = [GCC<"cpu_specific">];
+  let Args = [VariadicIdentifierArgument<"Cpus">];
----------------
aaron.ballman wrote:
> Does GCC really support this spelling? I couldn't find documentation to suggest it, and a quick test suggests it's not supported there. Perhaps this should use the Clang spelling instead?
Yep, you're right.  I'd forgotten about that differentiation here.  Thanks!


================
Comment at: include/clang/Basic/Attr.td:851
+  let Spellings = [GCC<"cpu_specific">];
+  let Args = [VariadicIdentifierArgument<"Cpus">];
+  let Subjects = SubjectList<[Function]>;
----------------
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.


================
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:
> 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?


https://reviews.llvm.org/D47474