[InlineCost] Check for conflicting target attributes early

[kazu]

Summary

When we inline a callee into a caller, the compiler needs to make sure
that the caller supports a superset of instruction sets that the
callee is allowed to use. Normally, we check for the compatibility of
target features via functionsHaveCompatibleAttributes, but that
happens after we decide to honor call site attribute
Attribute::AlwaysInline. If the caller contains a call marked with
Attribute::AlwaysInline, which can happen with
attribute((flatten)) placed on the caller, the caller could end up
with code that cannot be lowered to assembly code.

This patch fixes the problem by checking the target feature
compatibility before we honor Attribute::AlwaysInline.

Fixes https://github.com/llvm/llvm-project/issues/62664

[tejohnson]

llvm/lib/Analysis/InlineCost.cpp

2931

Probably this should be a missed optimization remark?

2947–2952

Can we move this above the always inline handling and put the new checking in here?

[kazu]

llvm/lib/Analysis/InlineCost.cpp

2931

Oops. I didn't meant to publish the fprintf, but yes, I'll turn it into a missed optimization remark.

2947–2952

Could you elaborate this comment a little bit? I am not sure what you mean by "put the new checking in here".

This patch essentially peels the CalleeTTI.areInlineCompatible check off of functionsHaveCompatibleAttributes. If we are moving the rest of functionsHaveCompatibleAttributes above the always inline handling, then we might just call the original unmodified functionsHaveCompatibleAttributes above the always inline handling.

[tejohnson]

llvm/lib/Analysis/InlineCost.cpp

2947–2952

Oh I see that it is being removed from functionsHaveCompatibleAttributes. It seems better to simply move the functionsHaveCompatibleAttributes checking above the always inline handling. I don't know the history here though - were you able to figure out why always inline functions skip this checking?

[kazu]

llvm/lib/Analysis/InlineCost.cpp

2947–2952

If I simply moved functionsHaveCompatibleAttributes above the always inline handling (independently of this patch), then I would get:

Failed Tests (3):
  LLVM :: Transforms/Inline/attributes.ll
  LLVM :: Transforms/Inline/inline_noprofile.ll
  LLVM :: Transforms/SampleProfile/remarks.ll

So, people (or at least these tests) expect a very specific behavior.

If I just move the CalleeTTI.areInlineCompatible instead (as in this patch), then I don't get any test failure.

Since we do not merge target features upon inlining, we should reject an inlining opportunity if the caller is not allowed to use a superset of the instruction sets that the callee is allowed to use.

Here is a little bit of history:

In 2013, commit 2ad3698b70745826f73e7270fc173e056e879619 added the comment:

// Never inline functions with conflicting attributes (unless callee has                                                                                                   
// always-inline attribute).

along with checks for the compatibility of Attribute::SanitizeAddress, Attribute::SanitizeMemory, and Attribute::SanitizeThread. These are a precursor to what we call AttributeFuncs::areInlineCompatible today. Back then, we didn't have checks for target features yet.

In 2015, commit f99e1913ae449e7d7f95fc918f44a2a1116e03d4 added checks to see if the target-cpu and target-features attributes exactly match between the callee and the caller, respectively.

Later in 2015, commit e1002268798ecedadaa9cdf6cc52b26e446ef3a7 relaxed the rule on the target features. The caller just has to have a superset of instructions that the callee is allowed to use.

I haven't checked every single patch in this area, but the ones I looked at do not have anything to do with the intersection of Attribute::AlwaysInline and target features.

[kazu]

Added a check for a remark.

[kazu]

I've added a CHECK for a missed optimization remark. Please take a look. Thanks!

llvm/lib/Analysis/InlineCost.cpp

2931

It turns out that the failure reason is already emitted as a missed optimization remark up in the call hierarchy, so we don't need to emit one on our own here.

[kazu]

Friendly ping. I now emit a remark:

return InlineResult::failure("conflicting target attributes");

so users should be able to tell where they are missing potential optimization opportunities due to mismatching target attributes.

[tejohnson]

llvm/lib/Analysis/InlineCost.cpp

2947–2952

Thanks for the history. Can none of the other attributes now checked by functionsHaveCompatibleAttributes (after removing the target attribute checks) cause failures if we ignore then due to the always inline attribute? I suppose the original sanitizer attributes aren't really correctness as they just control what type of sanitization is applied. But I wonder about the others, e.g. TLI. Also, AttributeFuncs::areInlineCompatible now also checks things like the denorm mode - that also seems problematic to ignore.

[kazu]

Add a FIXME comment.

[kazu]

I've added a FIXME comment about other attributes that the always-inline attribute takes precedence over. Please take a look. Thanks!

[tejohnson]

lgtm

llvm/lib/Analysis/InlineCost.cpp

 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 /// Dump stats about this call's analysis.
 LLVM_DUMP_METHOD void InlineCostCallAnalyzer::dump() { print(dbgs()); }
 #endif
 
 /// Test that there are no attribute conflicts between Caller and Callee
 ///        that prevent inlining.
 static bool functionsHaveCompatibleAttributes(
-    Function *Caller, Function *Callee, TargetTransformInfo &TTI,
+    Function *Caller, Function *Callee,
     function_ref<const TargetLibraryInfo &(Function &)> &GetTLI) {
   // Note that CalleeTLI must be a copy not a reference. The legacy pass manager
   // caches the most recently created TLI in the TargetLibraryInfoWrapperPass
   // object, and always returns the same object (which is overwritten on each
   // GetTLI call). Therefore we copy the first result.
   auto CalleeTLI = GetTLI(*Callee);
-  return (IgnoreTTIInlineCompatible ||
-          TTI.areInlineCompatible(Caller, Callee)) &&
-         GetTLI(*Caller).areInlineCompatible(CalleeTLI,
+  return GetTLI(*Caller).areInlineCompatible(CalleeTLI,
                                              InlineCallerSupersetNoBuiltin) &&
          AttributeFuncs::areInlineCompatible(*Caller, *Callee);
 }
 
 int llvm::getCallsiteCost(const CallBase &Call, const DataLayout &DL) {
   int64_t Cost = 0;
   for (unsigned I = 0, E = Call.arg_size(); I != E; ++I) {
     if (Call.isByValArgument(I)) {
std::optional<InlineResult> llvm::getAttributeBasedInliningDecision(
   for (unsigned I = 0, E = Call.arg_size(); I != E; ++I)
     if (Call.isByValArgument(I)) {
       PointerType *PTy = cast<PointerType>(Call.getArgOperand(I)->getType());
       if (PTy->getAddressSpace() != AllocaAS)
         return InlineResult::failure("byval arguments without alloca"
                                      " address space");
     }
 
+  // Never inline functions with conflicting target attributes.
+  Function *Caller = Call.getCaller();
+  if (!IgnoreTTIInlineCompatible &&
+      !CalleeTTI.areInlineCompatible(Caller, Callee))
+    return InlineResult::failure("conflicting target attributes");
+
   // Calls to functions with always-inline attributes should be inlined
   // whenever possible.
   if (Call.hasFnAttr(Attribute::AlwaysInline)) {
     if (Call.getAttributes().hasFnAttr(Attribute::NoInline))
       return InlineResult::failure("noinline call site attribute");
 
     auto IsViable = isInlineViable(*Callee);
     if (IsViable.isSuccess())
       return InlineResult::success();
     return InlineResult::failure(IsViable.getFailureReason());
   }
 
   // Never inline functions with conflicting attributes (unless callee has
   // always-inline attribute).
-  Function *Caller = Call.getCaller();
-  if (!functionsHaveCompatibleAttributes(Caller, Callee, CalleeTTI, GetTLI))
+  // FIXME: functionsHaveCompatibleAttributes below checks for compatibilities
+  // of different kinds of function attributes -- sanitizer-related ones,
+  // checkDenormMode, no-builtin-memcpy, etc.  It's unclear if we really want
+  // the always-inline attribute to take precedence over these different types
+  // of function attributes.
+  if (!functionsHaveCompatibleAttributes(Caller, Callee, GetTLI))
     return InlineResult::failure("conflicting attributes");
 
   // Don't inline this call if the caller has the optnone attribute.
   if (Caller->hasOptNone())
     return InlineResult::failure("optnone attribute");
 
   // Don't inline a function that treats null pointer as valid into a caller
   // that does not have this attribute.

llvm/test/Transforms/Inline/target-features-vs-alwaysinline.ll

+; RUN: opt < %s -passes=inline -pass-remarks-missed=inline -S 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Make sure that we do not inline callee into caller.  If we inline
+; callee into caller, caller would end pu with AVX512 intrinsics even
+; though it is not allowed to use AVX512 instructions.
+; CHECK: remark: [[MSG:.*]] because it should never be inlined (cost=never): conflicting target attributes
+
+define void @caller(ptr %0) {
+; CHECK-LABEL: define void @caller
+; CHECK-SAME: (ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    call void @callee(ptr [[TMP0]], i64 0, i32 0) #[[ATTR2:[0-9]+]]
+; CHECK-NEXT:    ret void
+;
+  call void @callee(ptr %0, i64 0, i32 0) #1
+  ret void
+}
+
+define available_externally void @callee(ptr %0, i64 %1, i32 %2) #0 {
+; CHECK-LABEL: define available_externally void @callee
+; CHECK-SAME: (ptr [[TMP0:%.*]], i64 [[TMP1:%.*]], i32 [[TMP2:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:    [[TMP4:%.*]] = call <16 x float> @llvm.x86.avx512.min.ps.512(<16 x float> zeroinitializer, <16 x float> zeroinitializer, i32 0)
+; CHECK-NEXT:    store <16 x float> [[TMP4]], ptr [[TMP0]], align 1
+; CHECK-NEXT:    ret void
+;
+  %4 = call <16 x float> @llvm.x86.avx512.min.ps.512(<16 x float> zeroinitializer, <16 x float> zeroinitializer, i32 0)
+  store <16 x float> %4, ptr %0, align 1
+  ret void
+}
+
+declare <16 x float> @llvm.x86.avx512.min.ps.512(<16 x float>, <16 x float>, i32 immarg)
+
+attributes #0 = { "target-features"="+aes,+avx,+avx2,+avx512bw,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+crc32,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" }
+attributes #1 = { alwaysinline }