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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13317
+      continue;
+    Value *Idxs[] = {Builder.getInt32(0), Builder.getInt32(i - 1)};
+    Value *Features = Builder.CreateAlignedLoad(
----------------
FreddyYe wrote:
> Don't quite understand why we need to change from a i32* into <4 x i32>* and the first element here to be always zero.
Sorry, this global variable (array) should be <3 x i32>. Fixed.

This (strange) scheme is for GCC/libgcc compatibility: bits 0~31 are in `__cpu_model.__cpufeatures[0]` while bits 32~max are in `__cpu_features2[0..2]`


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13319
+    Value *Features = Builder.CreateAlignedLoad(
+        Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),
+        CharUnits::fromQuantity(4));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329