Feedback/direction Review for cpu-dispatch emit stage into GlobalDecl

[erichkeane]

Summary

In the feedback in D51650 (https://reviews.llvm.org/D51650),
@rsmith points out an implementation detail of CPU-Dispatch
is improperly done. I'd like to fix this here before rebasing
target_clones on the patch resulting from this feedback.
This is a first-run at making the GlobalDecl contain the index
of the multiversion version.

The other suggestion @rsmith made was to make getting the MV-type
more streamlined. I've been thinking of ways to do that, and will
likely attempt to fix it before target_clones goes in with a rebase.

There are two big points of feedback that I'd really appreciate:
1- I suspect that something serious needs to be done on GlobalDecl.
It currently holds a 2 bit value in a PointerIntPair, which obviously
won't work for the index. I added an unsigned, but am open to
suggestions on how to store this better.

2- CodeGenModule::getFunctionFeatureMap takes a FunctionDecl, but the
features are dependent on the GlobalDecl now. I traced usages upwards,
and many cases don't have GlobalDecl in its call-tree (since it comes
from a callee). I'm not sure it matters, since only the emit-call should
require this info, but any suggestions are greatly appreciated.

[erichkeane]

@aaron.ballman or @rsmith any chance you guys could give me a hand on the direction here? I'm growing GlobalDecl by quite a bit, so I want to make sure this is both permissible and the right way to go. Additionally, feedback on how to get a GlobalDecl down to getFunctionFeatureMap would be appreciated.

Thanks!
-Erich

[erichkeane]

Superseding with a version that seems to work

include/clang/AST/GlobalDecl.h

 namespace clang {
 
 /// GlobalDecl - represents a global declaration. This can either be a
 /// CXXConstructorDecl and the constructor type (Base, Complete).
 /// a CXXDestructorDecl and the destructor type (Base, Complete) or
 /// a VarDecl, a FunctionDecl or a BlockDecl.
 class GlobalDecl {
   llvm::PointerIntPair<const Decl *, 2> Value;
+  unsigned MultiVersionIndex = 0;
 
   void Init(const Decl *D) {
     assert(!isa<CXXConstructorDecl>(D) && "Use other ctor with ctor decls!");
     assert(!isa<CXXDestructorDecl>(D) && "Use other ctor with dtor decls!");
 
     Value.setPointer(D);
   }
 
 public:
   GlobalDecl() = default;
   GlobalDecl(const VarDecl *D) { Init(D);}
-  GlobalDecl(const FunctionDecl *D) { Init(D); }
+  GlobalDecl(const FunctionDecl *D, unsigned MVIndex = 0)
+      : MultiVersionIndex(MVIndex) {
+    Init(D);
+  }
   GlobalDecl(const BlockDecl *D) { Init(D); }
   GlobalDecl(const CapturedDecl *D) { Init(D); }
   GlobalDecl(const ObjCMethodDecl *D) { Init(D); }
   GlobalDecl(const OMPDeclareReductionDecl *D) { Init(D); }
   GlobalDecl(const CXXConstructorDecl *D, CXXCtorType Type) : Value(D, Type) {}
   GlobalDecl(const CXXDestructorDecl *D, CXXDtorType Type) : Value(D, Type) {}
 
   GlobalDecl getCanonicalDecl() const {
     GlobalDecl CanonGD;
     CanonGD.Value.setPointer(Value.getPointer()->getCanonicalDecl());
     CanonGD.Value.setInt(Value.getInt());
+    CanonGD.MultiVersionIndex = MultiVersionIndex;
 
     return CanonGD;
   }
 
   const Decl *getDecl() const { return Value.getPointer(); }
 
   CXXCtorType getCtorType() const {
     assert(isa<CXXConstructorDecl>(getDecl()) && "Decl is not a ctor!");
     return static_cast<CXXCtorType>(Value.getInt());
   }
 
   CXXDtorType getDtorType() const {
     assert(isa<CXXDestructorDecl>(getDecl()) && "Decl is not a dtor!");
     return static_cast<CXXDtorType>(Value.getInt());
   }
 
+  unsigned getMultiVersionIndex() const {
+    assert(isa<FunctionDecl>(getDecl()) &&
+           !isa<CXXConstructorDecl>(getDecl()) &&
+           !isa<CXXDestructorDecl>(getDecl()) &&
+           "Decl is not a plain FunctionDecl!");
+    return MultiVersionIndex;
+  }
+
   friend bool operator==(const GlobalDecl &LHS, const GlobalDecl &RHS) {
-    return LHS.Value == RHS.Value;
+    return LHS.Value == RHS.Value &&
+           LHS.MultiVersionIndex == RHS.MultiVersionIndex;
   }
 
   void *getAsOpaquePtr() const { return Value.getOpaqueValue(); }
 
   static GlobalDecl getFromOpaquePtr(void *P) {
     GlobalDecl GD;
     GD.Value.setFromOpaqueValue(P);
     return GD;
   }
 
   GlobalDecl getWithDecl(const Decl *D) {
     GlobalDecl Result(*this);
     Result.Value.setPointer(D);
     return Result;
   }
+
+  GlobalDecl getWithMultiVersionIndex(unsigned Index) {
+    assert(isa<FunctionDecl>(getDecl()) &&
+           !isa<CXXConstructorDecl>(getDecl()) &&
+           !isa<CXXDestructorDecl>(getDecl()) &&
+           "Decl is not a plain FunctionDecl!");
+    GlobalDecl Result(*this);
+    Result.MultiVersionIndex = Index;
+    return Result;
+  }
 };
 
 } // namespace clang
 
 namespace llvm {
 
   template<> struct DenseMapInfo<clang::GlobalDecl> {
     static inline clang::GlobalDecl getEmptyKey() {

include/clang/Basic/Attr.td

 }
 
 def CPUSpecific : InheritableAttr {
   let Spellings = [Clang<"cpu_specific">];
   let Args = [VariadicIdentifierArgument<"Cpus">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [CPUSpecificCPUDispatchDocs];
   let AdditionalMembers = [{
-    unsigned ActiveArgIndex = 0;
-
-    IdentifierInfo *getCurCPUName() const {
-      return *(cpus_begin() + ActiveArgIndex);
+    IdentifierInfo *getCPUName(unsigned Index) const {
+      return *(cpus_begin() + Index);
     }
   }];
 }
 
 def CPUDispatch : InheritableAttr {
   let Spellings = [Clang<"cpu_dispatch">];
   let Args = [VariadicIdentifierArgument<"Cpus">];
   let Subjects = SubjectList<[Function]>;

lib/CodeGen/CodeGenModule.h

private:
 
   /// Set function attributes for a function declaration.
   void SetFunctionAttributes(GlobalDecl GD, llvm::Function *F,
                              bool IsIncompleteFunction, bool IsThunk);
 
   void EmitGlobalDefinition(GlobalDecl D, llvm::GlobalValue *GV = nullptr);
 
   void EmitGlobalFunctionDefinition(GlobalDecl GD, llvm::GlobalValue *GV);
+  void EmitMultiVersionFunctionDefinition(GlobalDecl GD, llvm::GlobalValue *GV);
+
   void EmitGlobalVarDefinition(const VarDecl *D, bool IsTentative = false);
   void EmitAliasDefinition(GlobalDecl GD);
   void emitIFuncDefinition(GlobalDecl GD);
   void emitCPUDispatchDefinition(GlobalDecl GD);
   void EmitObjCPropertyImplementations(const ObjCImplementationDecl *D);
   void EmitObjCIvarInitializations(ObjCImplementationDecl *D);
 
   // C++ related functions.

lib/CodeGen/CodeGenModule.cpp

 static std::string getCPUSpecificMangling(const CodeGenModule &CGM,
                                           StringRef Name) {
   const TargetInfo &Target = CGM.getTarget();
   return (Twine('.') + Twine(Target.CPUSpecificManglingCharacter(Name))).str();
 }
 
 static void AppendCPUSpecificCPUDispatchMangling(const CodeGenModule &CGM,
                                                  const CPUSpecificAttr *Attr,
+                                                 unsigned CPUIndex,
                                                  raw_ostream &Out) {
   // cpu_specific gets the current name, dispatch gets the resolver.
   if (Attr)
-    Out << getCPUSpecificMangling(CGM, Attr->getCurCPUName()->getName());
+    Out << getCPUSpecificMangling(CGM, Attr->getCPUName(CPUIndex)->getName());
   else
     Out << ".resolver";
 }
 
 static void AppendTargetMangling(const CodeGenModule &CGM,
                                  const TargetAttr *Attr, raw_ostream &Out) {
   if (Attr->isDefaultVersion())
     return;
if (MC.shouldMangleDeclName(ND)) {
     } else {
       Out << II->getName();
     }
   }
 
   if (const auto *FD = dyn_cast<FunctionDecl>(ND))
     if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
       if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion())
-        AppendCPUSpecificCPUDispatchMangling(
-            CGM, FD->getAttr<CPUSpecificAttr>(), Out);
+        AppendCPUSpecificCPUDispatchMangling(CGM,
+                                             FD->getAttr<CPUSpecificAttr>(),
+                                             GD.getMultiVersionIndex(), Out);
       else
         AppendTargetMangling(CGM, FD->getAttr<TargetAttr>(), Out);
     }
 
   return Out.str();
 }
 
 void CodeGenModule::UpdateMultiVersionNames(GlobalDecl GD,
if (const auto *CD = dyn_cast<CXXConstructorDecl>(CanonicalGD.getDecl())) {
     if (!getTarget().getCXXABI().hasConstructorVariants()) {
       CXXCtorType OrigCtorType = GD.getCtorType();
       assert(OrigCtorType == Ctor_Base || OrigCtorType == Ctor_Complete);
       if (OrigCtorType == Ctor_Base)
         CanonicalGD = GlobalDecl(CD, Ctor_Complete);
     }
   }
 
-  const auto *FD = dyn_cast<FunctionDecl>(GD.getDecl());
-  // Since CPUSpecific can require multiple emits per decl, store the manglings
-  // separately.
-  if (FD &&
-      (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion())) {
-    const auto *SD = FD->getAttr<CPUSpecificAttr>();
-
-    std::pair<GlobalDecl, unsigned> SpecCanonicalGD{
-        CanonicalGD,
-        SD ? SD->ActiveArgIndex : std::numeric_limits<unsigned>::max()};
-
-    auto FoundName = CPUSpecificMangledDeclNames.find(SpecCanonicalGD);
-    if (FoundName != CPUSpecificMangledDeclNames.end())
-      return FoundName->second;
-
-    auto Result = CPUSpecificManglings.insert(
-        std::make_pair(getMangledNameImpl(*this, GD, FD), SpecCanonicalGD));
-    return CPUSpecificMangledDeclNames[SpecCanonicalGD] = Result.first->first();
-  }
-
   auto FoundName = MangledDeclNames.find(CanonicalGD);
   if (FoundName != MangledDeclNames.end())
     return FoundName->second;
 
   // Keep the first result in the case of a mangling collision.
   const auto *ND = cast<NamedDecl>(GD.getDecl());
   auto Result =
       Manglings.insert(std::make_pair(getMangledNameImpl(*this, GD, ND), GD));
bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   // This happens in glibc's btowc and in some configure checks.
   return !isTriviallyRecursive(F);
 }
 
 bool CodeGenModule::shouldOpportunisticallyEmitVTables() {
   return CodeGenOpts.OptimizationLevel > 0;
 }
 
+void CodeGenModule::EmitMultiVersionFunctionDefinition(GlobalDecl GD,
+                                                       llvm::GlobalValue *GV) {
+  const auto *FD = cast<FunctionDecl>(GD.getDecl());
+
+  if (FD->isCPUSpecificMultiVersion()) {
+    auto *Spec = FD->getAttr<CPUSpecificAttr>();
+    for (unsigned I = 0; I < Spec->cpus_size(); ++I)
+      EmitGlobalFunctionDefinition(GD.getWithMultiVersionIndex(I), nullptr);
+    // Requires multiple emits.
+  } else
+    EmitGlobalFunctionDefinition(GD, GV);
+}
+
 void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
   const auto *D = cast<ValueDecl>(GD.getDecl());
 
   PrettyStackTraceDecl CrashInfo(const_cast<ValueDecl *>(D), D->getLocation(),
                                  Context.getSourceManager(),
                                  "Generating code for declaration");
 
-  if (isa<FunctionDecl>(D)) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     // At -O0, don't generate IR for functions with available_externally
     // linkage.
     if (!shouldEmitFunction(GD))
       return;
 
     if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
       // Make sure to emit the definition(s) before we emit the thunks.
       // This is necessary for the generation of certain thunks.
       if (const auto *CD = dyn_cast<CXXConstructorDecl>(Method))
         ABI->emitCXXStructor(CD, getFromCtorType(GD.getCtorType()));
       else if (const auto *DD = dyn_cast<CXXDestructorDecl>(Method))
         ABI->emitCXXStructor(DD, getFromDtorType(GD.getDtorType()));
+      else if (FD->isMultiVersion())
+        EmitMultiVersionFunctionDefinition(GD, GV);
       else
         EmitGlobalFunctionDefinition(GD, GV);
 
       if (Method->isVirtual())
         getVTables().EmitThunks(GD);
 
       return;
     }
 
+    if (FD->isMultiVersion())
+      return EmitMultiVersionFunctionDefinition(GD, GV);
     return EmitGlobalFunctionDefinition(GD, GV);
   }
 
   if (const auto *VD = dyn_cast<VarDecl>(D))
     return EmitGlobalVarDefinition(VD, !VD->hasDefinition());
 
   llvm_unreachable("Invalid argument to EmitGlobalDefinition()");
 }
llvm::Type *ResolverType = llvm::FunctionType::get(
       false);
   auto *ResolverFunc = cast<llvm::Function>(
       GetOrCreateLLVMFunction(ResolverName, ResolverType, GlobalDecl{},
                               /*ForVTable=*/false));
 
   SmallVector<CodeGenFunction::CPUDispatchMultiVersionResolverOption, 10>
       Options;
   const TargetInfo &Target = getTarget();
+  unsigned Index = 0;
   for (const IdentifierInfo *II : DD->cpus()) {
     // Get the name of the target function so we can look it up/create it.
     std::string MangledName = getMangledNameImpl(*this, GD, FD, true) +
                               getCPUSpecificMangling(*this, II->getName());
-    llvm::Constant *Func = GetOrCreateLLVMFunction(
-        MangledName, DeclTy, GD, /*ForVTable=*/false, /*DontDefer=*/false,
+    llvm::Constant *Func = GetGlobalValue(MangledName);
+
+    if (!Func) {
+
+      Func = GetOrCreateLLVMFunction(
+          MangledName, DeclTy, GD.getWithMultiVersionIndex(Index),
+          /*ForVTable=*/false, /*DontDefer=*/false,
           /*IsThunk=*/false, llvm::AttributeList(), ForDefinition);
+    }
+
     llvm::SmallVector<StringRef, 32> Features;
     Target.getCPUSpecificCPUDispatchFeatures(II->getName(), Features);
     llvm::transform(Features, Features.begin(),
                     [](StringRef Str) { return Str.substr(1); });
     Features.erase(std::remove_if(
         Features.begin(), Features.end(), [&Target](StringRef Feat) {
           return !Target.validateCpuSupports(Feat);
         }), Features.end());
     Options.emplace_back(cast<llvm::Function>(Func),
                          CodeGenFunction::GetX86CpuSupportsMask(Features));
+    ++Index;
   }
 
   llvm::sort(
       Options.begin(), Options.end(),
       std::greater<CodeGenFunction::CPUDispatchMultiVersionResolverOption>());
   CodeGenFunction CGF(*this);
   CGF.EmitCPUDispatchMultiVersionResolver(ResolverFunc, Options);
 }
void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
   SetLLVMFunctionAttributesForDefinition(D, Fn);
 
   if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>())
     AddGlobalCtor(Fn, CA->getPriority());
   if (const DestructorAttr *DA = D->getAttr<DestructorAttr>())
     AddGlobalDtor(Fn, DA->getPriority());
   if (D->hasAttr<AnnotateAttr>())
     AddGlobalAnnotations(D, Fn);
-
-  if (D->isCPUSpecificMultiVersion()) {
-    auto *Spec = D->getAttr<CPUSpecificAttr>();
-    // If there is another specific version we need to emit, do so here.
-    if (Spec->ActiveArgIndex + 1 < Spec->cpus_size()) {
-      ++Spec->ActiveArgIndex;
-      EmitGlobalFunctionDefinition(GD, nullptr);
-    }
-  }
 }
 
 void CodeGenModule::EmitAliasDefinition(GlobalDecl GD) {
   const auto *D = cast<ValueDecl>(GD.getDecl());
   const AliasAttr *AA = D->getAttr<AliasAttr>();
   assert(AA && "Not an alias?");
 
   StringRef MangledName = getMangledName(GD);
if (const auto *TD = FD->getAttr<TargetAttr>()) {
     // Now populate the feature map, first with the TargetCPU which is either
     // the default or a new one from the target attribute string. Then we'll use
     // the passed in features (FeaturesAsWritten) along with the new ones from
     // the attribute.
     Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU,
                           ParsedAttr.Features);
   } else if (const auto *SD = FD->getAttr<CPUSpecificAttr>()) {
     llvm::SmallVector<StringRef, 32> FeaturesTmp;
-    Target.getCPUSpecificCPUDispatchFeatures(SD->getCurCPUName()->getName(),
+    /*Target.getCPUSpecificCPUDispatchFeatures(SD->getCurCPUName()->getName(),
                                              FeaturesTmp);
     std::vector<std::string> Features(FeaturesTmp.begin(), FeaturesTmp.end());
     Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU, Features);
+    */// TODO! Re-enable if we can figure out what the current version is.
   } else {
     Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU,
                           Target.getTargetOpts().Features);
   }
 }
 
 llvm::SanitizerStatReport &CodeGenModule::getSanStats() {
   if (!SanStats)

test/CodeGen/attr-cpuspecific.c

 // CHECK: call void @__cpu_indicator_init
 // CHECK: ret void ()* @TwoVersions.Z
 // CHECK: ret void ()* @TwoVersions.S
 // CHECK: call void @llvm.trap
 // CHECK: unreachable
 
 __attribute__((cpu_specific(ivybridge)))
 void TwoVersions(void){}
-// CHECK: define void @TwoVersions.S() #[[S]]
+// CHECK: define void @TwoVersions.S()
+// TODO: #[[S]]
 
 __attribute__((cpu_specific(knl)))
 void TwoVersions(void){}
 // CHECK: define void @TwoVersions.Z() #[[K:[0-9]+]]
 
 __attribute__((cpu_specific(ivybridge, knl)))
 void TwoVersionsSameAttr(void){}
 // CHECK: define void @TwoVersionsSameAttr.S() #[[S]]
-// CHECK: define void @TwoVersionsSameAttr.Z() #[[K]]
+// CHECK: define void @TwoVersionsSameAttr.Z()
+// TODO: #[[K]]
 
 __attribute__((cpu_specific(atom, ivybridge, knl)))
 void ThreeVersionsSameAttr(void){}
 // CHECK: define void @ThreeVersionsSameAttr.O() #[[O:[0-9]+]]
 // CHECK: define void @ThreeVersionsSameAttr.S() #[[S]]
-// CHECK: define void @ThreeVersionsSameAttr.Z() #[[K]]
+// CHECK: define void @ThreeVersionsSameAttr.Z()
+// TODO #[[K]]
 
 void usages() {
   SingleVersion();
   // CHECK: @SingleVersion.ifunc()
   TwoVersions();
   // CHECK: @TwoVersions.ifunc()
   TwoVersionsSameAttr();
   // CHECK: @TwoVersionsSameAttr.ifunc()

 // CHECK: define void ()* @HasGeneric.resolver()
 // CHECK: call void @__cpu_indicator_init
 // CHECK: ret void ()* @HasGeneric.Z
 // CHECK: ret void ()* @HasGeneric.S
 // CHECK: ret void ()* @HasGeneric.O
 // CHECK: ret void ()* @HasGeneric.A
 // CHECK-NOT: call void @llvm.trap
 
-// CHECK: attributes #[[S]] = {{.*}}"target-features"="+avx,+cmov,+f16c,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave"
-// CHECK: attributes #[[K]] = {{.*}}"target-features"="+adx,+avx,+avx2,+avx512cd,+avx512er,+avx512f,+avx512pf,+bmi,+cmov,+f16c,+fma,+lzcnt,+mmx,+movbe,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave"
-// CHECK: attributes #[[O]] = {{.*}}"target-features"="+cmov,+mmx,+movbe,+sse,+sse2,+sse3,+ssse3,+x87"
+__attribute__((cpu_specific(atom, ivybridge))) void HasGeneric(void) {}
+// CHECK: define void @HasGeneric.O()
+// TODO: #[[O]]
+// CHECK: define void @HasGeneric.S()
+// TODO: #[[S]]
+
+// XCHECK: attributes #[[S]] = {{.*}}"target-features"="+avx,+cmov,+f16c,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave"
+// XCHECK: attributes #[[K]] = {{.*}}"target-features"="+adx,+avx,+avx2,+avx512cd,+avx512er,+avx512f,+avx512pf,+bmi,+cmov,+f16c,+fma,+lzcnt,+mmx,+movbe,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave"
+// XCHECK: attributes #[[O]] = {{.*}}"target-features"="+cmov,+mmx,+movbe,+sse,+sse2,+sse3,+ssse3,+x87"