[Clang] Emit error when caller cannot meet target feature requirement from always-inlining callee

[qiucf]

Summary

Currently clang emits error when both always_inline and target attributes are on callee, but caller doesn't have some feature:

// RUN: %clang_cc1 -triple powerpc64le -target-feature -htm

__attribute__((always_inline))
__attribute__((target("htm")))
void foo() {}

void bar() { foo(); }

// error: always_inline function 'foo' requires target feature 'htm', but would be inlined into function 'bar' that is compiled without support for 'htm'

But when the always_inline attribute is on caller, clang has no diagnose. If any builtin or inline asm really need the feature, backend will crash.

// RUN: %clang_cc1 -triple powerpc64le -target-feature +htm

__attribute__((always_inline))
void foo() {
  // No error, but uncommenting line below triggers ICE
  // __builtin_ttest();
}

__attribute__((target("no-htm")))
void bar() { foo(); }

This patch will fix the second case.

[qiucf]

Gentle ping

[erichkeane]

Thanks for your patience, I was buried trying to get the 16.0 release done. This LGTM!

[erichkeane]

Hi-
We've actually seen some regressions internally thanks to this patch, which boil down to:

https://godbolt.org/z/WEer4M6ha

// compiled with -march=skylake
__attribute__((always_inline)) inline void foo(){}

__attribute__((target("arch=core-avx2")))
void bar() {
foo();
}

I believe @craig.topper and I spent a long time on these things, but I'm now questioning what arch= is supposed to do in this case. We're compiling bar as if it is an avx2 function, so it cannot call foo, which has additional features. However, they were not 'disabled'. A part of me thinks that the arch=... should mean "has at least these features", and the bug is thus in the code that sets the features list.

What does everyone think? attn: @FreddyYe

[erichkeane]

In D143479#4243090, @erichkeane wrote:

Hi-
We've actually seen some regressions internally thanks to this patch, which boil down to:

https://godbolt.org/z/WEer4M6ha

// compiled with -march=skylake
__attribute__((always_inline)) inline void foo(){}

__attribute__((target("arch=core-avx2")))
void bar() {
foo();
}

I believe @craig.topper and I spent a long time on these things, but I'm now questioning what arch= is supposed to do in this case. We're compiling bar as if it is an avx2 function, so it cannot call foo, which has additional features. However, they were not 'disabled'. A part of me thinks that the arch=... should mean "has at least these features", and the bug is thus in the code that sets the features list.

What does everyone think? attn: @FreddyYe

Based on GCC's behavior: https://godbolt.org/z/fxWzPTT9P I suspect our behavior is consistent/correct now, and the 'regressions' are to be expected, since GCC diagnoses the same things we do (just nicer, albeit, less informative).

[qiucf]

In D143479#4243143, @erichkeane wrote:

Based on GCC's behavior: https://godbolt.org/z/fxWzPTT9P I suspect our behavior is consistent/correct now, and the 'regressions' are to be expected, since GCC diagnoses the same things we do (just nicer, albeit, less informative).

This is expected. But we can do better like GCC to find 'real target requirements from called intrinsics' of a function, which may be a larger change though.

clang/lib/CodeGen/CGCall.cpp

RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
     // We can only guarantee that a function is called from the correct
     // context/function based on the appropriate target attributes,
     // so only check in the case where we have both always_inline and target
     // since otherwise we could be making a conditional call after a check for
     // the proper cpu features (and it won't cause code generation issues due to
     // function based code generation).
     if (TargetDecl->hasAttr<AlwaysInlineAttr>() &&
-        TargetDecl->hasAttr<TargetAttr>())
+        (TargetDecl->hasAttr<TargetAttr>() ||
+         (CurFuncDecl && CurFuncDecl->hasAttr<TargetAttr>())))
       checkTargetFeatures(Loc, FD);
 
     // Some architectures (such as x86-64) have the ABI changed based on
     // attribute-target/features. Give them a chance to diagnose.
     CGM.getTargetCodeGenInfo().checkFunctionCallABI(
         CGM, Loc, dyn_cast_or_null<FunctionDecl>(CurCodeDecl), FD, CallArgs);
   }
 

clang/lib/CodeGen/CodeGenFunction.cpp

if (!llvm::all_of(ReqFeatures, [&](StringRef Feature) {
       if (!CallerFeatureMap.lookup(Feature)) {
         MissingFeature = Feature.str();
         return false;
       }
       return true;
     }))
       CGM.getDiags().Report(Loc, diag::err_function_needs_feature)
           << FD->getDeclName() << TargetDecl->getDeclName() << MissingFeature;
+  } else if (!FD->isMultiVersion() && FD->hasAttr<TargetAttr>()) {
+    llvm::StringMap<bool> CalleeFeatureMap;
+    CGM.getContext().getFunctionFeatureMap(CalleeFeatureMap, TargetDecl);
+
+    for (const auto &F : CalleeFeatureMap) {
+      if (F.getValue() && (!CallerFeatureMap.lookup(F.getKey()) ||
+                           !CallerFeatureMap.find(F.getKey())->getValue()))
+        CGM.getDiags().Report(Loc, diag::err_function_needs_feature)
+            << FD->getDeclName() << TargetDecl->getDeclName() << F.getKey();
+    }
   }
 }
 
 void CodeGenFunction::EmitSanitizerStatReport(llvm::SanitizerStatKind SSK) {
   if (!CGM.getCodeGenOpts().SanitizeStats)
     return;
 
   llvm::IRBuilder<> IRB(Builder.GetInsertBlock(), Builder.GetInsertPoint());

clang/test/Sema/ppc-attr-target-inline.c

+// REQUIRES: powerpc-registered-target
+// RUN: %clang_cc1 -triple powerpc64le -target-feature +htm -fsyntax-only -emit-llvm %s -verify
+
+__attribute__((always_inline))
+int test1(int *x) {
+  *x = __builtin_ttest();
+  return *x;
+}
+
+__attribute__((target("no-htm")))
+int test2(int *x) {
+  *x = test1(x); // expected-error {{always_inline function 'test1' requires target feature 'htm', but would be inlined into function 'test2' that is compiled without support for 'htm'}}
+  return 0;
+}