[CodeGen] Improve diagnostics related to target attributes

[GBuella]

Summary

When requirement imposed by target attributes on functions
are not satisfied, prefer printing those requirements, which
are explicitly mentioned in the attributes.

This makes such messages more useful, e.g. printing avx512f instead of avx2
in the following scenario:

$ cat foo.c
static inline void __attribute__((__always_inline__, __target__("avx512f")))
x(void)
{
}

int main(void)
{
	        x();
}
$ clang foo.c
foo.c:7:2: error: always_inline function 'x' requires target feature 'avx2', but would be inlined into function 'main' that is compiled without support for 'avx2'
        x();
	^
1 error generated.

bugzilla: https://bugs.llvm.org/show_bug.cgi?id=37338

[craig.topper]

lib/CodeGen/CodeGenFunction.cpp

2271

'&' should be in front of the variable name not after the type.

2289

Please run clang-format on this. There's some weird indentation happening.

2326

Can we call getAttr here and pass the result of that instead of the TargetDecl? We know the attribute exists. Then rename getFunctionTargetAttrs and make it only responsible for removing unknown features. Push its other call site back into the 'if' there. This removes the need to return an empty ParsedTargetAttr.

lib/CodeGen/CodeGenModule.cpp

5017

Open curly brace should be on previous line.

5027

!ParsedAttr.Features.empty()

[craig.topper]

lib/CodeGen/CodeGenFunction.cpp

2352

This and the next line are indented funny.

[craig.topper]

lib/CodeGen/CodeGenFunction.cpp

2348

Rather than walking the ParsedAttr.Features for each feature in the map. And having to shift the ReqFeatures vectors sometimes. How about doing this

-Walk through all features in ParsedAttr, for each feature with a +, query the callee feature map. If it's enabled there, push it to ReqFeatures.
-Walk through all features in the callee feature map and if enabled push those.

This will lead to duplicates in the list, but all the explicitly mentioned features will be listed first.

[craig.topper]

This looks pretty good to me. @echristo what do you think?

[craig.topper]

Ping @echristo

[echristo]

I think this will work, one inline comment. Might also be good to get a few different test cases, e.g. one where we're not seeing the alphabetically first as the minimum :)

lib/CodeGen/CodeGenModule.h

1085

Needs a block comment saying what it does :)

[GBuella]

Fixed a horrible bug in the patch. Adding a ref to temporary string is not a wise thing to do, so I had to remove this line:

ReqFeatures.push_back(F.substr(1));

Now the ReqFeatures vector can also refer to strings starting with '+'

Also, added a few tests and comments.

[craig.topper]

I think you can pass StringRef(F).substr(1). That won't create a temporary string. It will just create a StringRef pointing into the middle of an existing std::string stored in the parsed attributes.

[GBuella]

In D46541#1106743, @craig.topper wrote:

I think you can pass StringRef(F).substr(1). That won't create a temporary string. It will just create a StringRef pointing into the middle of an existing std::string stored in the parsed attributes.

Yes, that seems to work.

[craig.topper]

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

37

The 4 compare functions here are basically the same for the purpose of this check. What value do we get by testing all 4 of them?

[GBuella]

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

37

Which four more specifically?

[craig.topper]

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

37

_mm_cmp_ps, _mm_cmp_ss, _mm_cmp_pd, _mm_cmp_sd. Or the cases for NEED_AVX_2-5. They're all basically checking the same thing. There is nothing different about the command lines other than the -D NEED_AVX

[GBuella]

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

37

Yes, I guess we could just remove NEED_AVX_3,4,5

[craig.topper]

LGTM

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;
                            });
       });
 }
void CodeGenFunction::checkTargetFeatures(const CallExpr *E,
   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.
+
+    const TargetAttr *TD = TargetDecl->getAttr<TargetAttr>();
+    TargetAttr::ParsedTargetAttr ParsedAttr = CGM.filterFunctionTargetAttrs(TD);
+
     SmallVector<StringRef, 1> ReqFeatures;
     llvm::StringMap<bool> CalleeFeatureMap;
     CGM.getFunctionFeatureMap(CalleeFeatureMap, TargetDecl);
+
+    for (const auto &F : ParsedAttr.Features) {
+      if (F[0] == '+' && CalleeFeatureMap.lookup(F.substr(1)))
+        ReqFeatures.push_back(F.substr(1));
+    }
+
     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;
   }
 }
 
 void CodeGenFunction::EmitSanitizerStatReport(llvm::SanitizerStatKind SSK) {
   if (!CGM.getCodeGenOpts().SanitizeStats)
     return;
 

lib/CodeGen/CodeGenModule.h

public:
   /// function in the bitcode library!
   ///
   /// 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);
 
+  TargetAttr::ParsedTargetAttr filterFunctionTargetAttrs(const TargetAttr *TD);
+
   // Fills in the supplied string map with the set of target features for the
   // passed in function.
   void getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
                              const FunctionDecl *FD);
 
   StringRef getMangledName(GlobalDecl GD);
   StringRef getBlockMangledName(GlobalDecl GD, const BlockDecl *BD);
 

lib/CodeGen/CodeGenModule.cpp

if (auto CrossDsoTypeId = CreateCrossDsoCfiTypeId(MD))
                               llvm::ConstantAsMetadata::get(CrossDsoTypeId));
 
   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,
-                                          const FunctionDecl *FD) {
-  StringRef 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();
+TargetAttr::ParsedTargetAttr CodeGenModule::filterFunctionTargetAttrs(const TargetAttr *TD) {
+  assert(TD != nullptr);
 
-    ParsedAttr.Features.erase(
-        llvm::remove_if(ParsedAttr.Features,
-                        [&](const std::string &Feat) {
-                          return !Target.isValidFeatureName(
-                              StringRef{Feat}.substr(1));
-                        }),
-        ParsedAttr.Features.end());
+  return ParsedAttr;
+}
+
+
+// Fills in the supplied string map with the set of target features for the
+// passed in function.
+void CodeGenModule::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
+                                          const FunctionDecl *FD) {
+  StringRef TargetCPU = Target.getTargetOpts().CPU;
+  if (const auto *TD = FD->getAttr<TargetAttr>()) {
+    TargetAttr::ParsedTargetAttr ParsedAttr = filterFunctionTargetAttrs(TD);
 
     // Make a copy of the features as passed on the command line into the
     // beginning of the additional features from the function to override.
     ParsedAttr.Features.insert(ParsedAttr.Features.begin(),
                             Target.getTargetOpts().FeaturesAsWritten.begin(),
                             Target.getTargetOpts().FeaturesAsWritten.end());
 
     if (ParsedAttr.Architecture != "" &&
         Target.isValidCPUName(ParsedAttr.Architecture))
       TargetCPU = ParsedAttr.Architecture;
 
     // 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,

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

 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_SSE42
 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_AVX_1
 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_AVX_2
 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_AVX_3
 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_AVX_4
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -D NEED_AVX512f
 
 #define __MM_MALLOC_H
 #include <x86intrin.h>
 
 #if NEED_SSE42
 int baz(__m256i a) {
-  return _mm256_extract_epi32(a, 3); // expected-error {{always_inline function '_mm256_extract_epi32' requires target feature 'sse4.2', but would be inlined into function 'baz' that is compiled without support for 'sse4.2'}}
+  return _mm256_extract_epi32(a, 3); // expected-error {{always_inline function '_mm256_extract_epi32' requires target feature 'avx', but would be inlined into function 'baz' that is compiled without support for 'avx'}}
 }
 #endif
 
 #if NEED_AVX_1
 __m128 need_avx(__m128 a, __m128 b) {
   return _mm_cmp_ps(a, b, 0); // expected-error {{'__builtin_ia32_cmpps' needs target feature avx}}
 }
 #endif
 
 #if NEED_AVX_2
 __m128 need_avx(__m128 a, __m128 b) {
   return _mm_cmp_ss(a, b, 0); // expected-error {{'__builtin_ia32_cmpss' needs target feature avx}}
 }
 #endif
 
 #if NEED_AVX_3
 __m128d need_avx(__m128d a, __m128d b) {
   return _mm_cmp_pd(a, b, 0); // expected-error {{'__builtin_ia32_cmppd' needs target feature avx}}
 }
 #endif
 
 #if NEED_AVX_4
 __m128d need_avx(__m128d a, __m128d b) {
   return _mm_cmp_sd(a, b, 0); // expected-error {{'__builtin_ia32_cmpsd' needs target feature avx}}
 }
 #endif
+
+#if NEED_AVX512f
+unsigned short need_avx512f(unsigned short a, unsigned short b) {
+  return __builtin_ia32_korhi(a, b); // expected-error {{'__builtin_ia32_korhi' needs target feature avx512f}}
+}
+#endif

test/CodeGen/target-features-error.c

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