[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13319
+    Value *Features = Builder.CreateAlignedLoad(
+        Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),
+        CharUnits::fromQuantity(4));
----------------
FreddyYe wrote:
> MaskRay wrote:
> > FreddyYe wrote:
> > > Will function multi version also be fixed in this patch? https://gcc.godbolt.org/z/cafhs9qbG If so, need to add test in clang/test/CodeGen/attr-target-mv.c
> > The `target` attribute has strange semantics for overloading:
> > ```
> > int __attribute__((target("arch=skylake"))) foo(void) {return 0;}
> > int __attribute__((target("arch=x86-64"))) foo(void) {return 1;}
> > ```
> > is not allowed in C mode of GCC.
> > 
> > I think such use cases are not recommended in C++.
> > 
> > If we don't use overloading, `int __attribute__((target("arch=x86-64"))) foo(void) {return 1;}` is supported by Clang today and this patch does not intend to change anything about it.
> I think behavior for `target` attribute is not only overloading but also function multiversioning for redefined functions. And seems like C model of gcc haven't supported is due to it will target C23 standard: https://clang.llvm.org/docs/AttributeReference.html#target 
> Comparing to gcc, clang misses not only `target` attribute but also other cpuname/feature related features for "x86-64*". See https://gcc.godbolt.org/z/arhne35GG (Seems gcc defined x86-64* as not only cpu name but also feature name.) Anyway, this patch is targeting for `target_clones` attribute only. So no problem here.
> So no problem here.

Right. Not supporting C and requiring duplicate definitions makes `target` not really useful. 
https://gcc.gnu.org/pipermail/gcc-patches/2015-October/430585.html Jeff Law said: "I wasn't aware that multi-versioning was only implemented for C++, that seems fairly lame.  I hope I didn't approve that :-)"




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158329/new/

https://reviews.llvm.org/D158329