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