[Target] Diagnose mismatch in required CPU for always_inline functions

[craig.topper]

Summary

If an always inline function requests a different CPU than its caller we should probably error.

If the callee CPU has features that the caller CPU doesn't we would already error for the feature mismatch, but it prints a misleading error about the first feature that mismatches.
If the callee CPU feature list a subset of the caller features we wouldn't error at all.

We also only error right now if the callee as a target attribute, but don't check anything if only the caller has a target attribute. This is consistent with our previous checking behavior, but we might want to fix that. I've left a TODO.

I've simplified some of GetCPUAndFeaturesAttributes since I needed to return the CPU string from getFunctionFeatureMap for the other callers anyway. This also saves us a second parse of the target attribute when adding attributes.

[echristo]

I like the idea of a front end warning, but had believed that a subset of cpu features should be allowed to be inlined into something that's a superset and it sounds like you don't agree? Your thoughts here?

-eric

[craig.topper]

From the backend codegen perspective it would be fine if the callees instruction features were a subset. So it really depends on why the user wrote the different arch on the callee in the first place. If they did it because of one of the various tuning flags in the backend that we set based on CPU name that information would be lost during the inlining. Maybe a warning would be more appropriate. My main goal was to give the user something more to go on when the instruction set features weren't a subset so we didn't just give them some random feature name that mismatched first.

Experiments with gcc showed that it throws an error for CPU name mismatch. Strangely it even gave an error if one function had a target attribute with an arch= and the command line had the same -march.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_arm_invalid_specialreg : Error<"invalid special register for builtin">;
 def err_invalid_cpu_supports : Error<"invalid cpu feature string for builtin">;
 def err_invalid_cpu_is : Error<"invalid cpu name for builtin">;
 def err_builtin_needs_feature : Error<"%0 needs target feature %1">;
 def err_function_needs_feature
     : Error<"always_inline function %1 requires target feature '%2', but would "
             "be inlined into function %0 that is compiled without support for "
             "'%2'">;
+def err_function_needs_cpu
+    : Error<"always_inline function %1 requires target CPU '%2', but would "
+            "be inlined into function %0 that is compiled for '%3'">;
 def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
   InGroup<ImplicitFunctionDeclare>, DefaultError;
 def warn_cstruct_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this "
   "%1 call is a pointer to record %2 that is not trivial to "
   "%select{primitive-default-initialize|primitive-copy}3">,
   InGroup<DiagGroup<"nontrivial-memaccess">>;
 def note_nontrivial_field : Note<

lib/CodeGen/CodeGenFunction.cpp

 void CGBuilderInserter::InsertHelper(
     llvm::Instruction *I, const llvm::Twine &Name, llvm::BasicBlock *BB,
     llvm::BasicBlock::iterator InsertPt) const {
   llvm::IRBuilderDefaultInserter::InsertHelper(I, Name, BB, InsertPt);
   if (CGF)
     CGF->InsertHelper(I, Name, BB, InsertPt);
 }
 
-static bool hasRequiredFeatures(const SmallVectorImpl<StringRef> &ReqFeatures,
-                                CodeGenModule &CGM, const FunctionDecl *FD,
-                                std::string &FirstMissing) {
-  // If there aren't any required features listed then go ahead and return.
-  if (ReqFeatures.empty())
-    return false;
-
-  // Now build up the set of caller features and verify that all the required
-  // features are there.
-  llvm::StringMap<bool> CallerFeatureMap;
-  CGM.getFunctionFeatureMap(CallerFeatureMap, FD);
-
-  // If we have at least one of the features in the feature list return
-  // true, otherwise return false.
-  return std::all_of(
-      ReqFeatures.begin(), ReqFeatures.end(), [&](StringRef Feature) {
-        SmallVector<StringRef, 1> OrFeatures;
-        Feature.split(OrFeatures, '|');
-        return std::any_of(OrFeatures.begin(), OrFeatures.end(),
-                           [&](StringRef Feature) {
-                             if (!CallerFeatureMap.lookup(Feature)) {
-                               FirstMissing = Feature.str();
-                               return false;
-                             }
-                             return true;
-                           });
-      });
-}
-
 // Emits an error if we don't have a valid set of target features for the
 // called function.
 void CodeGenFunction::checkTargetFeatures(const CallExpr *E,
                                           const FunctionDecl *TargetDecl) {
   // Early exit if this is an indirect call.
   if (!TargetDecl)
     return;
 
   // Get the current enclosing function if it exists. If it doesn't
   // we can't check the target features anyhow.
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl);
   if (!FD)
     return;
 
   // Grab the required features for the call. For a builtin this is listed in
   // the td file with the default cpu, for an always_inline function this is any
   // listed cpu and any listed features.
+  SmallVector<StringRef, 1> ReqFeatures;
+  // CalleeFeatureMap needs to be at this scope to prevent ReqFeatures from
+  // having dangling StringRefs.
+  llvm::StringMap<bool> CalleeFeatureMap;
+  StringRef RequiredCPU;
   unsigned BuiltinID = TargetDecl->getBuiltinID();
-  std::string MissingFeature;
   if (BuiltinID) {
-    SmallVector<StringRef, 1> ReqFeatures;
     const char *FeatureList =
         CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID);
     // Return if the builtin doesn't have any required features.
     if (!FeatureList || StringRef(FeatureList) == "")
       return;
     StringRef(FeatureList).split(ReqFeatures, ',');
-    if (!hasRequiredFeatures(ReqFeatures, CGM, FD, MissingFeature))
-      CGM.getDiags().Report(E->getLocStart(), diag::err_builtin_needs_feature)
-          << TargetDecl->getDeclName()
-          << CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID);
-
   } else if (TargetDecl->hasAttr<TargetAttr>()) {
-    // Get the required features for the callee.
-    SmallVector<StringRef, 1> ReqFeatures;
-    llvm::StringMap<bool> CalleeFeatureMap;
-    CGM.getFunctionFeatureMap(CalleeFeatureMap, TargetDecl);
+    CGM.getFunctionCPUAndFeatureMap(CalleeFeatureMap, RequiredCPU, TargetDecl);
     for (const auto &F : CalleeFeatureMap) {
       // Only positive features are "required".
       if (F.getValue())
         ReqFeatures.push_back(F.getKey());
     }
-    if (!hasRequiredFeatures(ReqFeatures, CGM, FD, MissingFeature))
-      CGM.getDiags().Report(E->getLocStart(), diag::err_function_needs_feature)
-          << FD->getDeclName() << TargetDecl->getDeclName() << MissingFeature;
+  }
+  // TODO: What if the caller has a TargetAttr, but the callee doesn't.
+
+  // Now build up the set of caller features and verify that all the required
+  // features are there.
+  llvm::StringMap<bool> CallerFeatureMap;
+  StringRef CallerCPU;
+  CGM.getFunctionCPUAndFeatureMap(CallerFeatureMap, CallerCPU, FD);
+
+  // Make sure the CPUs match unless this is a builtin.
+  if (!BuiltinID && CallerCPU != RequiredCPU) {
+    CGM.getDiags().Report(E->getLocStart(), diag::err_function_needs_cpu)
+        << FD->getDeclName() << TargetDecl->getDeclName() << RequiredCPU
+        << CallerCPU;
+    return;
+  }
+
+  // If we have all of the required features, we're done.
+  std::string FirstMissing;
+  if (llvm::all_of(ReqFeatures,
+                   [&](StringRef Feature) {
+                     SmallVector<StringRef, 1> OrFeatures;
+                     Feature.split(OrFeatures, '|');
+                     return llvm::any_of(OrFeatures,
+                                         [&](StringRef Feature) {
+                                           if (!CallerFeatureMap.lookup(Feature)) {
+                                             FirstMissing = Feature.str();
+                                             return false;
+                                           }
+                                           return true;
+                                         });
+                   }))
+    return;
+
+  // We have a missing feature, diagnose it.
+  if (BuiltinID) {
+    CGM.getDiags().Report(E->getLocStart(), diag::err_builtin_needs_feature)
+        << TargetDecl->getDeclName()
+        << CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID);
+  } else {
+        << FD->getDeclName() << TargetDecl->getDeclName() << FirstMissing;
   }
 }
 
 void CodeGenFunction::EmitSanitizerStatReport(llvm::SanitizerStatKind SSK) {
   if (!CGM.getCodeGenOpts().SanitizeStats)
     return;
 
   llvm::IRBuilder<> IRB(Builder.GetInsertBlock(), Builder.GetInsertPoint());

lib/CodeGen/CodeGenModule.h

public:
   ///
   /// With the exception of fast-math attrs, this will only make the attributes
   /// on the function more conservative.  But it's unsafe to call this on a
   /// function which relies on particular fast-math attributes for correctness.
   /// It's up to you to ensure that this is safe.
   void AddDefaultFnAttrs(llvm::Function &F);
 
   // Fills in the supplied string map with the set of target features for the
-  // passed in function.
-  void getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
+  // passed in function. Also fills in the target CPU.
+  void getFunctionCPUAndFeatureMap(llvm::StringMap<bool> &FeatureMap,
+                                   StringRef &TargetCPU,
                                    const FunctionDecl *FD);
 
   StringRef getMangledName(GlobalDecl GD);
   StringRef getBlockMangledName(GlobalDecl GD, const BlockDecl *BD);
 
   void EmitTentativeDefinition(const VarDecl *D);
 
   void EmitVTable(CXXRecordDecl *Class);
 

lib/CodeGen/CodeGenModule.cpp

bool CodeGenModule::GetCPUAndFeaturesAttributes(const Decl *D,
   StringRef TargetCPU = getTarget().getTargetOpts().CPU;
   std::vector<std::string> Features;
   const auto *FD = dyn_cast_or_null<FunctionDecl>(D);
   FD = FD ? FD->getMostRecentDecl() : FD;
   const auto *TD = FD ? FD->getAttr<TargetAttr>() : nullptr;
   bool AddedAttr = false;
   if (TD) {
     llvm::StringMap<bool> FeatureMap;
-    getFunctionFeatureMap(FeatureMap, FD);
+    getFunctionCPUAndFeatureMap(FeatureMap, TargetCPU, FD);
 
     // Produce the canonical string for this set of features.
     for (const llvm::StringMap<bool>::value_type &Entry : FeatureMap)
       Features.push_back((Entry.getValue() ? "+" : "-") + Entry.getKey().str());
-
-    // Now add the target-cpu and target-features to the function.
-    // While we populated the feature map above, we still need to
-    // get and parse the target attribute so we can get the cpu for
-    // the function.
-    TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
-    if (ParsedAttr.Architecture != "" &&
-        getTarget().isValidCPUName(ParsedAttr.Architecture))
-      TargetCPU = ParsedAttr.Architecture;
   } else {
     // Otherwise just add the existing target cpu and target features to the
     // function.
+    TargetCPU = getTarget().getTargetOpts().CPU;
     Features = getTarget().getTargetOpts().Features;
   }
 
   if (TargetCPU != "") {
     Attrs.addAttribute("target-cpu", TargetCPU);
     AddedAttr = true;
   }
   if (!Features.empty()) {
void CodeGenModule::AddVTableTypeMetadata(llvm::GlobalVariable *VTable,
   if (NeedAllVtablesTypeId()) {
     llvm::Metadata *MD = llvm::MDString::get(getLLVMContext(), "all-vtables");
     VTable->addTypeMetadata(Offset.getQuantity(), MD);
   }
 }
 
 // Fills in the supplied string map with the set of target features for the
 // passed in function.
-void CodeGenModule::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
+void
+CodeGenModule::getFunctionCPUAndFeatureMap(llvm::StringMap<bool> &FeatureMap,
+                                           StringRef &TargetCPU,
                                            const FunctionDecl *FD) {
-  StringRef TargetCPU = Target.getTargetOpts().CPU;
+  TargetCPU = Target.getTargetOpts().CPU;
   if (const auto *TD = FD->getAttr<TargetAttr>()) {
     // If we have a TargetAttr build up the feature map based on that.
     TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
 
     ParsedAttr.Features.erase(
         llvm::remove_if(ParsedAttr.Features,
                         [&](const std::string &Feat) {
                           return !Target.isValidFeatureName(

test/CodeGen/target-cpu-error-2.c

+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -target-cpu nehalem
+int __attribute__((target("arch=silvermont"), always_inline)) foo(int a) {
+  return a + 4;
+}
+int bar() {
+  return foo(4); // expected-error {{always_inline function 'foo' requires target CPU 'silvermont', but would be inlined into function 'bar' that is compiled for 'nehalem'}}
+}
+

test/CodeGen/target-cpu-error-3.c

+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -target-cpu silvermont
+int __attribute__((target("sse4.2"), always_inline)) foo(int a) {
+  return a + 4;
+}
+int __attribute__((target("arch=nehalem"))) bar() {
+  return foo(4); // expected-error {{always_inline function 'foo' requires target CPU 'silvermont', but would be inlined into function 'bar' that is compiled for 'nehalem'}}
+}
+

test/CodeGen/target-cpu-error.c

+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o -
+int __attribute__((target("arch=silvermont"), always_inline)) foo(int a) {
+  return a + 4;
+}
+int __attribute__((target("arch=nehalem"))) bar() {
+  return foo(4); // expected-error {{always_inline function 'foo' requires target CPU 'silvermont', but would be inlined into function 'bar' that is compiled for 'nehalem'}}
+}
+