[PATCH] D38596: Implement attribute target multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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?

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

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


https://reviews.llvm.org/D38596