[PATCH] D38596: Implement attribute target multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Hi Richard, thanks for the review!  I'll make another attempt with this based on your feedback, though if you could clarify your suggestion, it would be greatly appreciated.

In https://reviews.llvm.org/D38596#909957, @rsmith wrote:

> 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.


I'd experimented with (but not succeeded with) simply preventing merging of the decls, but had a tough time keeping like-versions together for the purposes of the iFunc. Additionally, clarifying the 'call' later on became quite difficult. I can definitely give that another try though.

> 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

I'm not sure I know what one of those looks like... is there a good test that I can use to learn the modules implementation?

> - multiversioned functions declared inline

I don't see anything changing for this situation, but I imagine you have an issue in mind that I'm missing?

> - constant expression evaluation of calls to such functions (which should -- presumably -- either always use the default version or be rejected as non-constant)

I'd say they should always do the default version, but this isn't something I did any work on.  I'll look at that.

> 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.

Right, this is a big catch that I didn't think about...



================
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",
----------------
rsmith wrote:
> Where did this table and code actually come from?
I took the list of things recognized by GCC, ran them through godbolt to see the list.


================
Comment at: lib/Basic/Targets/X86.cpp:1392
+                            "intel",
+                            "amd"};
+
----------------
rsmith wrote:
> Can you replace this list (and the repetition of it elsewhere in this file) with a `.def` file carrying the same information?
I've been working with Craig trying to do something like this, though not with a .def file.  So far I've yet to come up with something signifiantly better, but I'm still working on it.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:421-423
+      Diags.Report(
+          cast<FunctionDecl>(GD.getDecl())->getFirstDecl()->getLocation(),
+          diag::err_target_without_default);
----------------
rsmith wrote:
> 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`.
Unfortunately, there is no real way to tell there.  A usage could happen before the 2nd definition, so it wouldn't know that it IS a multiversioning at that point.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2134
+    return MangledName;
+  return (MangledName + "." + Append).str();
+}
----------------
rsmith wrote:
> 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?
I can definitely extract this over to the name manglers.  This mangling scheme is exactly what GCC and ICC do currently.  I have no idea how it avoids collisions besides not being able to define the root name with another mechanism.


https://reviews.llvm.org/D38596