[InstrProf] Allow noprofile functions to be inlined

[ellis]

Summary

Allow callees with the noprofile attribute to be inlined into profiled callers.

We *keep* the restriction that profiled callees *cannot* be inlined into callers with the noprofile attribute.

This was discussed in [0].

Also, remove the areOutlineCompatible() function which appears to be
unused.

[0] https://discourse.llvm.org/t/why-does-the-noprofile-attribute-restrict-inlining/64108

[ellis]

Please let me know if there is a reason for preventing noprofile functions from being inlined into profiled functions. If there is one I'd like to understand it.

[MaskRay]

I think the update makes sense.

[nickdesaulniers]

Do you mind editing the description to use caller and callee rather than foo?

[nickdesaulniers]

llvm/lib/IR/Attributes.cpp

2042–2044

Could we create a new CompatRule fn for this? I imagine we might want to reuse such logic for other attributes in the future.

[ellis]

llvm/lib/IR/Attributes.cpp

2042–2044

IMO the CompatRule unnecessarily obfuscates the code. It took me a while to find this issue because I didn't see CompatRule on this attribute. Also, it requires reading several Attributes.* files to fully understand what's going on.

Instead, I could pull this out into a regular function and we can later add more calls with the other attributes. Let me know what you think!

[nickdesaulniers]

I observed no new build or runtime warnings when compiling the linux kernel with gcov enabled (CONFIG_GCOV_KERNEL=y + CONFIG_GCOV_PROFILE_ALL=y) or kcsan (CONFIG_KCSAN=y) (x86).

[ellis]

Create function to check attribute compatability so that we can easily do this for more attributes in the future.

[ellis]

In D130806#3694879, @nickdesaulniers wrote:

I observed no new build or runtime warnings when compiling the linux kernel with gcov enabled (CONFIG_GCOV_KERNEL=y + CONFIG_GCOV_PROFILE_ALL=y) or kcsan (CONFIG_KCSAN=y) (x86).

Thanks for checking!

[melver]

In D130806#3695022, @ellis wrote:

In D130806#3694879, @nickdesaulniers wrote:

I observed no new build or runtime warnings when compiling the linux kernel with gcov enabled (CONFIG_GCOV_KERNEL=y + CONFIG_GCOV_PROFILE_ALL=y) or kcsan (CONFIG_KCSAN=y) (x86).

Thanks for checking!

If a callee with the noprofile attribute is inlined into a profiled function, will it then also be profiled?
Is it possible that a noprofile function could receive instrumentation through inlining now?

If so, this is wrong and will cause problems. Perhaps for profiling instrumentation it works differently, but for the sanitizers this was an issue we fought for a while to get right everywhere: https://lore.kernel.org/lkml/[e-mail]/

llvm/test/Transforms/Inline/inline_noprofile.ll

22

noprofile (no space)

[ellis]

In D130806#3695858, @melver wrote:

In D130806#3695022, @ellis wrote:

In D130806#3694879, @nickdesaulniers wrote:

I observed no new build or runtime warnings when compiling the linux kernel with gcov enabled (CONFIG_GCOV_KERNEL=y + CONFIG_GCOV_PROFILE_ALL=y) or kcsan (CONFIG_KCSAN=y) (x86).

Thanks for checking!

If a callee with the noprofile attribute is inlined into a profiled function, will it then also be profiled?
Is it possible that a noprofile function could receive instrumentation through inlining now?

If so, this is wrong and will cause problems. Perhaps for profiling instrumentation it works differently, but for the sanitizers this was an issue we fought for a while to get right everywhere: https://lore.kernel.org/lkml/[e-mail]/

The callee won’t technically be profiled, but because it was inlined it could be mixed with instrumentation code. My reasoning was, if a function interacts badly when inlined into an instrumented function, then it probably shouldn’t be inlined at all.

[melver]

In D130806#3697318, @ellis wrote:

In D130806#3695858, @melver wrote:

...
If a callee with the noprofile attribute is inlined into a profiled function, will it then also be profiled?
Is it possible that a noprofile function could receive instrumentation through inlining now?

If so, this is wrong and will cause problems. Perhaps for profiling instrumentation it works differently, but for the sanitizers this was an issue we fought for a while to get right everywhere: https://lore.kernel.org/lkml/[e-mail]/

The callee won’t technically be profiled, but because it was inlined it could be mixed with instrumentation code. My reasoning was, if a function interacts badly when inlined into an instrumented function, then it probably shouldn’t be inlined at all.

As long as the code in the callee doesn't receive any instrumentation, I think we're fine; i.e. the instructions in the callee are not intermingled with instrumentation, and from the machine's point of view what's missing is the call overhead.

[ellis]

In D130806#3697423, @melver wrote:

In D130806#3697318, @ellis wrote:

In D130806#3695858, @melver wrote:

...
If a callee with the noprofile attribute is inlined into a profiled function, will it then also be profiled?
Is it possible that a noprofile function could receive instrumentation through inlining now?

If so, this is wrong and will cause problems. Perhaps for profiling instrumentation it works differently, but for the sanitizers this was an issue we fought for a while to get right everywhere: https://lore.kernel.org/lkml/[e-mail]/

The callee won’t technically be profiled, but because it was inlined it could be mixed with instrumentation code. My reasoning was, if a function interacts badly when inlined into an instrumented function, then it probably shouldn’t be inlined at all.

As long as the code in the callee doesn't receive any instrumentation, I think we're fine; i.e. the instructions in the callee are not intermingled with instrumentation, and from the machine's point of view what's missing is the call overhead.

I believe this patch does cause the instructions in the callee to "intermingle with instrumentation", so it might not be safe without source changes.

I've just created the skipprofile attribute which doesn't have this inlining restriction, so this patch isn't necessary. I've also updated the docs for noprofile to be more clear.

Thanks for clarifying this case and I'll abandon this diff now.

llvm/docs/LangRef.rst

``nomerge``
     user control over the tradeoff between code size and debug information
     precision.
 ``nonlazybind``
     This attribute suppresses lazy symbol binding for the function. This
     may make calls to the function faster, at the cost of extra program
     startup time if the function is not called during program startup.
 ``noprofile``
     This function attribute prevents instrumentation based profiling, used for
-    coverage or profile based optimization, from being added to a function,
-    even when inlined.
+    coverage or profile based optimization, from being added to a function. It
+    also prevents instrumented functions from being inlined into this function.
 ``noredzone``
     This attribute indicates that the code generator should not use a
     red zone, even if the target-specific ABI normally permits it.
 ``indirect-tls-seg-refs``
     This attribute indicates that the code generator should not use
     direct TLS access through segment registers, even if the
     target-specific ABI normally permits it.
 ``noreturn``

llvm/include/llvm/IR/Attributes.h

 /// include attributes like nocapture, which constrain the function
 /// implementation rather than the passed value.
 AttributeMask getUBImplyingAttributes();
 
 /// \returns Return true if the two functions have compatible target-independent
 /// attributes for inlining purposes.
 bool areInlineCompatible(const Function &Caller, const Function &Callee);
 
-
-/// Checks  if there are any incompatible function attributes between
-/// \p A and \p B.
-///
-/// \param [in] A - The first function to be compared with.
-/// \param [in] B - The second function to be compared with.
-/// \returns true if the functions have compatible attributes.
-bool areOutlineCompatible(const Function &A, const Function &B);
-
 /// Merge caller's and callee's attributes.
 void mergeAttributesForInlining(Function &Caller, const Function &Callee);
 
 /// Merges the functions attributes from \p ToMerge into function \p Base.
 ///
 /// \param [in,out] Base - The function being merged into.
 /// \param [in] ToMerge - The function to merge attributes from.
 void mergeAttributesForOutlining(Function &Base, const Function &ToMerge);

llvm/include/llvm/IR/Attributes.td

 def NoReturn : EnumAttr<"noreturn", [FnAttr]>;
 
 /// Function does not synchronize.
 def NoSync : EnumAttr<"nosync", [FnAttr]>;
 
 /// Disable Indirect Branch Tracking.
 def NoCfCheck : EnumAttr<"nocf_check", [FnAttr]>;
 
-/// Function should not be instrumented.
+/// This function should not be instrumented and profiled functions cannot be
+/// inlined into it.
 def NoProfile : EnumAttr<"noprofile", [FnAttr]>;
 
 /// Function doesn't unwind stack.
 def NoUnwind : EnumAttr<"nounwind", [FnAttr]>;
 
 /// No SanitizeBounds instrumentation.
 def NoSanitizeBounds : EnumAttr<"nosanitize_bounds", [FnAttr]>;
 

 def : CompatRule<"isEqual<SanitizeAddressAttr>">;
 def : CompatRule<"isEqual<SanitizeThreadAttr>">;
 def : CompatRule<"isEqual<SanitizeMemoryAttr>">;
 def : CompatRule<"isEqual<SanitizeHWAddressAttr>">;
 def : CompatRule<"isEqual<SanitizeMemTagAttr>">;
 def : CompatRule<"isEqual<SafeStackAttr>">;
 def : CompatRule<"isEqual<ShadowCallStackAttr>">;
 def : CompatRule<"isEqual<UseSampleProfileAttr>">;
-def : CompatRule<"isEqual<NoProfileAttr>">;
 
 class MergeRule<string F> {
   // The name of the function called to merge the attributes of the caller and
   // callee. The function's signature must match
   // "void(Function&, const Function &)", where the first parameter is the
   // reference to the caller and the second parameter is the reference to the
   // callee.
   string MergeFunc = F;

llvm/lib/IR/Attributes.cpp

#define ATTRIBUTE_STRBOOL(ENUM_NAME, DISPLAY_NAME)                             \
   };
 #include "llvm/IR/Attributes.inc"
 
 #define GET_ATTR_COMPAT_FUNC
 #include "llvm/IR/Attributes.inc"
 
 bool AttributeFuncs::areInlineCompatible(const Function &Caller,
                                          const Function &Callee) {
+  // For some attributes, we want to forbid inlining if the attribute is found
+  // in the caller but not the callee. For example, when a user adds the
+  // noprofile attribute we don't want to inline profiled functions.
+  auto AttrInCallerNotInCallee = [&](Attribute::AttrKind Kind) -> bool {
+    return Caller.hasFnAttribute(Kind) && !Callee.hasFnAttribute(Kind);
+  };
+  if (AttrInCallerNotInCallee(Attribute::NoProfile))
+    return false;
   return hasCompatibleFnAttrs(Caller, Callee);
 }
 
-bool AttributeFuncs::areOutlineCompatible(const Function &A,
-                                          const Function &B) {
-  return hasCompatibleFnAttrs(A, B);
-}
-
 void AttributeFuncs::mergeAttributesForInlining(Function &Caller,
                                                 const Function &Callee) {
   mergeFnAttrs(Caller, Callee);
 }
 
 void AttributeFuncs::mergeAttributesForOutlining(Function &Base,
                                                 const Function &ToMerge) {
 

llvm/test/Transforms/Inline/inline_noprofile.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes=inline %s -S -pass-remarks-missed=inline 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-INLINE
 ; RUN: opt -passes=always-inline %s -S | FileCheck %s
 
-; Test that we don't inline when caller and callee don't have matching
-; noprofile fn attrs.
+; Test that we don't inline profiled callees into noprofile callers.
 
 ; CHECK-INLINE: 'profile' not inlined into 'profile_caller' because it should never be inlined (cost=never): conflicting attributes
-; CHECK-INLINE: 'noprofile' not inlined into 'noprofile_caller' because it should never be inlined (cost=never): conflicting attributes
 
 define i32 @profile() { ret i32 42 }
-define i32 @noprofile() noprofile { ret i32 43 }
 define i32 @profile_aa() alwaysinline { ret i32 44 }
-define i32 @noprofile_aa() noprofile alwaysinline { ret i32 45 }
 
 define i32 @profile_caller() noprofile {
 ; CHECK-LABEL: @profile_caller(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @profile()
 ; CHECK-NEXT:    ret i32 44
 ;
   call i32 @profile()
   %2 = call i32 @profile_aa()
   ret i32 %2
 }
 
-define i32 @noprofile_caller() {
-; CHECK-LABEL: @noprofile_caller(
-; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @noprofile()
-; CHECK-NEXT:    ret i32 45
-;
-  call i32 @noprofile()
-  %2 = call i32 @noprofile_aa()
-  ret i32 %2
-}
-
-; Test that we do inline when caller and callee don't have matching
-; noprofile fn attrs, when CallInst is alwaysinline.
-define i32 @aa_callsite() {
+; Test that we do inline profiled callees into no profile callers when CallInst
+; is alwaysinline
+define i32 @aa_callsite() noprofile {
 ; CHECK-INLINE-LABEL: @aa_callsite(
-; CHECK-INLINE-NEXT:    ret i32 43
+; CHECK-INLINE-NEXT:    ret i32 42
 ;
-  %1 = call i32 @noprofile() alwaysinline
+  %1 = call i32 @profile() alwaysinline
   ret i32 %1
 }