[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:13272
+  uint64_t Mask = llvm::X86::getCpuSupportsMask(FeatureStrs);
+  uint32_t FeaturesMask[4] = {uint32_t(Mask), uint32_t(Mask >> 32), 0, 0};
+  return EmitX86CpuSupports(FeaturesMask);
----------------
pengfei wrote:
> Use `Lo_32(Mask)`, `Hi_32(Mask)`?
Lo_32/Hi_32 are currently unused in clang/. Using this needs MathExtras.h. I am unsure it's worthwhile..


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13318-13320
+    Value *Idxs[] = {Builder.getInt32(0), Builder.getInt32(i - 1)};
+    Value *Features = Builder.CreateAlignedLoad(
+        Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),
----------------
pengfei wrote:
> Do we have problem when linking with old version of libgcc?
If we don't use `__cpu_features2[1..3]`, no problem with older libgcc.

The way GCC upgraded `__cpu_features2` to an array is compatible with scalar `__cpu_features2`.


================
Comment at: compiler-rt/lib/builtins/cpu_model.c:808
+  if (HasExtLeaf1) {
+    if (ECX & 1)
+      setFeature(FEATURE_LAHF_LM);
----------------
pengfei wrote:
> pengfei wrote:
> > Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The same below.
> Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The same below.
Thanks for the cpuid.h idea, but I think likely no. MSVC does not provide cpuid.h. It would be nice if we don't allow MSVC to build the file (we can say that `LLVM_ENABLE_PROJECTS='...compiler-rt'` is not allowed and LLVM_ENABLE_RUNTIMES or Clang should be used, but I think we are not there yet).

Unfortunately I am also unsure whether anyone uses MSVC to build this file, but there are Windows related macros like `FIXME: For MSVC, we should make a function pointer global in` in this file...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329