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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Cant help with the RT/LLVM parts, but the clang parts are pretty much right.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13278
+llvm::Value *
+CodeGenFunction::EmitX86CpuSupports(std::array<uint32_t, 4> FeatureMask) {
   Value *Result = Builder.getTrue();
----------------
Hmm... I guess size-wise this is on the edge of "const ref vs pass by value".  I think its fine now, but 'next time' this grows we'll have to think about making this a const-ref.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13311
+  llvm::Constant *CpuFeatures2 =
+      CGM.CreateRuntimeVariable(ATy, "__cpu_features2");
+  cast<llvm::GlobalValue>(CpuFeatures2)->setDSOLocal(true);
----------------
This won't double-create this if used more than 1x, right?  There doesn't need to be something like GetOrCreate... here?


================
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);
----------------
MaskRay wrote:
> 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..
I was previously using Lo_32/Hi_32 here (see the removed parts of EmitX86CpuSupports).  Were we using those transitively?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329