[PATCH] D38596: Implement attribute target multiversioning

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38596#889697, @erichkeane wrote:

> In https://reviews.llvm.org/D38596#889682, @hfinkel wrote:
>
> > Can you explain briefly what is required of the system in order to support this (is it just ifunc support in the dynamic linker / loader)?
>
>
> Yep, this feature depends entirely on ifuncs.
>
> > In general, there are a number of places in this patch, at the Sema layer (including in the error messages), that talk about how this is an x86-only feature. As soon as someone adds support for a second architecture, this will cause unnecessary churn (and perhaps miss places resulting in stale comments). Can you create some callbacks, in TargetInfo or similar, so that we can just naturally make this work on other architectures?
>
> Yep, I have that enforced, with the hope that it would make it easier to identify all the places that this change needs to be made.  Currently, there are a few 'TargetInfo' features that need to be supported (isValidCPUName/isValidCPUFeature/compareCpusAndFeatures), the SEMA check relaxed (though I could perhaps add a TargetInfo::IsFunctionMultiVersioningSupported function if thats what you're saying?), and CodeGenFunction::FormResolverCondition/CodeGenFunction::EmitMultiVersionResolver ( which could easily have FormResolverCondition moved to TargetInfo, assuming we OK with a CodeGen file calling into a TargetInfo?).  Are these the changes you mean?


I added some comments inline.

> 
> 
>> 
>> 
>>> Function templates are NOT supported (not supported in GCC either).
>> 
>> Can you please elaborate on this? I'd like to see this feature, but I don't believe that we should introduce features that artificially force users to choose between good code design and technical capabilities. The fact that GCC does not support this feature on templates seems unfortunate, but not in itself a justification to replicate that restriction. I'm obviously fine with adding template functions in a follow-up commit, but I want to understand whether there's something in the design (of the feature or of your implementation approach) that makes this hard. I wouldn't want this to ship without template support.
> 
> I don't think there are limitations beyond my understanding of the code around redefinitions of templates.  I investigated for a while and was unable to figure it out over a few days, but that doesn't mean terribly much.  I gave up temporarily in exchange for getting the rest of the feature in (and the fact that GCC doesn't support it made it an explainable stopping point; additionally, none of our users of 'attribute-target' actually have C++ codebases).

I anticipate this will change.

>   I believe I'd be able to add function-template support with some additional work, and am definitely willing to do so.

Great, thanks!

> Thanks Hal, I appreciate your advice/feedback!  I realize this is a huge patch, so anything I can get to make this more acceptable is greatly appreciated.





================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9305
+    : Error<"function multiversioning with 'target' is only supported on "
+            "X86/x86_64 architectures">;
+def err_bad_multiversion_option
----------------
I'd not mention x86 here. As soon as we add support for another architecture, we'll need to reword this anyway.

  : Error<"function multiversioning with 'target' is not supported on this architecture and platform">;



================
Comment at: lib/Basic/Targets/X86.cpp:1295
       .Case("amdfam10h", true)
+      .Case("amdfam10", true)
       .Case("amdfam15h", true)
----------------
Can you please separate out these changes adding the amdfam10 target strings into a separate patch?


================
Comment at: lib/Basic/Targets/X86.cpp:1330
+  // ordering of the GCC Target attribute.
+  const auto TargetArray = {"avx512vpopcntdq",
+                            "avx5124fmaps",
----------------
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.



================
Comment at: lib/Sema/SemaDecl.cpp:9249
+
+  // Need to check isValidCPUName since it checks X86 vs X86-64 CPUs.  From
+  // there, the subset of valid ones is captured in validateCpuIs.
----------------
No need to mention x86 in this comment.


================
Comment at: lib/Sema/SemaDecl.cpp:9279
+                                    const FunctionDecl *NewFD) {
+  // FIXME: Implement for more than X86.  Requires CPUIs and CPUSupports
+  // for each new architecture.
----------------
No need to have x86 here in the comment or the check. Just add some function to TargetInfo:
  if (!Context.getTargetInfo().supportsFuncMultiversioning()) {
    Diag(



================
Comment at: test/CodeGenCXX/attr-target-multiversion.cpp:20
+}
+
+// CHECK: define i{{[0-9]+}} (%struct.S*)* @_ZN1S2mvEv.resolver
----------------
Tests for interactions with function pointers and virtual functions?


https://reviews.llvm.org/D38596