[PATCH] D38596: Implement attribute target multiversioning

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'm not entirely happy with the AST representation you're using here. Allowing multiple declarations of the same entity to have (semantically distinct) bodies breaks our AST invariants, and will cause things like our PCH / modules support to fail. This can probably be made to work, but I'd like to see what that looks like before we commit to modeling a multiversioned function this way rather than, say, with different redeclaration chains for each version.

You should add tests for:

- the PCH / Modules case (particularly regarding what happens when merging multiple copies of the same set of functions, for instance when they're defined inline); I would expect this doesn't currently work with this patch, because we only ever load one function body per redeclaration chain
- multiversioned functions declared inline
- constant expression evaluation of calls to such functions (which should -- presumably -- either always use the default version or be rejected as non-constant)

I would also expect that asking a multiversioned `FunctionDecl` for its definition would only ever give you the `target("default")` definition or the one-and-only definition, rather than an arbitrary declaration that happens to have a body.



================
Comment at: lib/Basic/Targets/X86.cpp:1330
+  // ordering of the GCC Target attribute.
+  const auto TargetArray = {"avx512vpopcntdq",
+                            "avx5124fmaps",
----------------
hfinkel wrote:
> erichkeane wrote:
> > hfinkel wrote:
> > > How are we expected to maintain this array?
> > Ugg... I know.  I have no idea, it is ANOTHER implementation of this foolish list, just in a slightly different order.  We NEED to know the order, so we know how to order the comparisons.  They are generally arbitrary orderings, so I have no idea besides "with hard work".  If you have a suggestion, or perhaps a way to reorder one of the OTHER lists, I'd love to hear it.
> At the risk of a comment that will become stale at some point, is it possible to say something here like:
> 
>   // This list has to match the list in GCC. In GCC 6.whatever, the list is in path/to/file/in/gcc.c
> 
> At least that would give someone a clue of how to update this list if necessary.
> 
I don't see why this list would need to match the list in some GCC source file. What this list has to match is the set of targets we wish to support, which (for compatibility with GCC) should contain all those that GCC also supports. And it sounds like there are ordering constraints implied by the contents of this list. That's what you should be talking about here.


================
Comment at: lib/Basic/Targets/X86.cpp:1329
+  // implementation in in GCC 7.x in gcc/config/i386/i386.c
+  // ::get_builtin_code_for_version. This list is simplified from that source.
+  const auto TargetArray = {"avx512vpopcntdq",
----------------
Where did this table and code actually come from?


================
Comment at: lib/Basic/Targets/X86.cpp:1330
+  // ::get_builtin_code_for_version. This list is simplified from that source.
+  const auto TargetArray = {"avx512vpopcntdq",
+                            "knm",
----------------
Deducing `std::initializer_list` is an abomination. Use `const char *TargetArray[]` or similar here instead.


================
Comment at: lib/Basic/Targets/X86.cpp:1392
+                            "intel",
+                            "amd"};
+
----------------
Can you replace this list (and the repetition of it elsewhere in this file) with a `.def` file carrying the same information?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:421-423
+      Diags.Report(
+          cast<FunctionDecl>(GD.getDecl())->getFirstDecl()->getLocation(),
+          diag::err_target_without_default);
----------------
I'm not happy with this diagnostic being produced by IR generation. We should diagnose this at the point of first use of the multiversioned function, within `Sema`.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2134
+    return MangledName;
+  return (MangledName + "." + Append).str();
+}
----------------
CodeGen should not be inventing mangled names with external linkage. Please move this to the name mangler. Is this mangling scheme compatible with GCC? How will we avoid name collisions between this and other uses of dot suffixes?


================
Comment at: test/CodeGenCXX/attr-target-multiversion.cpp:21
+
+// CHECK: define i{{[0-9]+}} (%struct.S*)* @_ZN1S2mvEv.resolver
+// CHECK: ret i{{[0-9]+}} (%struct.S*)* @_ZN1S2mvEv.arch_sandybridge
----------------
This looks like it has the wrong linkage.


https://reviews.llvm.org/D38596