[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:13318-13320
+    Value *Idxs[] = {Builder.getInt32(0), Builder.getInt32(i - 1)};
+    Value *Features = Builder.CreateAlignedLoad(
+        Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),
----------------
pengfei wrote:
> MaskRay wrote:
> > 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`.
> I assume GCC seldom links to older libgcc, but there's no guarantee Clang can link to latest libgcc.
> And we cannot assume user won't use it once the method provided.
> I don't have a good idea for it, but I think we should write the requirement done in release note or somewhere if we cannot find a better way.
The way we treat `__cpu_features2` as an array is compatible with older libgcc providing scalar `__cpu_features2`, as long as we don't access the high bits (`FEATURE_X86_64_{BASELINE,V2,V3,V4}`).

* When `arch=x86-64` is used, this patch changes a compiler crash to (if newer libgcc, work; if older libgcc, runtime crash)
* When `arch=x86-64*` is unused, the same as before.


================
Comment at: compiler-rt/lib/builtins/cpu_model.c:167
+  FEATURE_WP,
+  FEATURE_LZCNT = 57,
+  FEATURE_MOVBE,
----------------
FreddyYe wrote:
> ` = 57` redundant
Thanks. Minor issue, I've fixed it locally and will only upload a new diff if other things need changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329