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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
