[InstrProf] Add the skipprofile attribute

[ellis]

Summary

As discussed in [0], this diff adds the skipprofile attribute to
prevent the function from being profiled while allowing profiled
functions to be inlined into it. The noprofile attribute remains
unchanged.

The noprofile attribute is used for functions where it is
dangerous to add instrumentation to while the skipprofile attribute is
used to reduce code size or performance overhead.

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

[paulkirth]

Do you expect the difference between noprofile and omitprofile to be confusing to users? I can certainly see how users could be confused by what the difference is between noprofile and omitprofile ...

Since what you want to communicate is "never profile"(which noprofile can probably communicate as is) and "never profile this, but allow inlining of profiled functions" (which I'm not sure omitprofile communicates), then maybe there is a more obvious way we could communicate that? Maybe neverprofile and nodirectprofile are more descriptive names? I don't love the idea of changing an existing attribute name, but we can transparently upgrade existing uses for backwards compatibility if we have to. What do you think?

I also think this will need to need to be supported in LLVM's passes, right? So the instrumentation passes in PGOInstrumentation.cpp (I can't remember if 'InstrProfiling.cpp` will need this too), and probably the Inlining passes too, right? I assume those will be follow up patches, but I don't see them in the current stack. Is that an accurate assumption?

BTW I like the direction of these patches, I think adding the ability to differentiate these cases will add a lot of value to the profiling runtimes + coverage. :)

[ellis]

In D130807#3688798, @paulkirth wrote:

Do you expect the difference between noprofile and omitprofile to be confusing to users? I can certainly see how users could be confused by what the difference is between noprofile and omitprofile ...

Since what you want to communicate is "never profile"(which noprofile can probably communicate as is) and "never profile this, but allow inlining of profiled functions" (which I'm not sure omitprofile communicates), then maybe there is a more obvious way we could communicate that? Maybe neverprofile and nodirectprofile are more descriptive names? I don't love the idea of changing an existing attribute name, but we can transparently upgrade existing uses for backwards compatibility if we have to. What do you think?

I agree that omitprofile probably doesn't communicate the correct meaning very well. I like nodirectprofile because it hopefully implies that indirect profiles are allowed. I'm wondering if anyone else has suggestions.

Is there any precedent for renaming function attributes? My assumption was that users would add the noprofile attribute to source mostly for correctness concerns and noprofile is easy to remember and already in use, so I'd rather not change it.

I also think this will need to need to be supported in LLVM's passes, right? So the instrumentation passes in PGOInstrumentation.cpp (I can't remember if 'InstrProfiling.cpp` will need this too), and probably the Inlining passes too, right? I assume those will be follow up patches, but I don't see them in the current stack. Is that an accurate assumption?

I actually do have a change in PGOInstrumentation.cpp that skips profiling functions with this new attribute. As far as I know, this stack has all the necessary code changes.

BTW I like the direction of these patches, I think adding the ability to differentiate these cases will add a lot of value to the profiling runtimes + coverage. :)

Thanks for the suggestion and feedback!

[paulkirth]

I agree that omitprofile probably doesn't communicate the correct meaning very well. I like nodirectprofile because it hopefully implies that indirect profiles are allowed. I'm wondering if anyone else has suggestions.

Is there any precedent for renaming function attributes? My assumption was that users would add the noprofile attribute to source mostly for correctness concerns and noprofile is easy to remember and already in use, so I'd rather not change it.

I agree with you that it's probably best to avoid changing the spelling of noprofile, but since you're making a change in the area, we may as well discuss if 1) that is still the best name, and 2) if we want to keep it.

I don't know if we've ever changed the spelling of a FnAttribute before. IIRC we've dropped them, and changed their format e.g., from being a boolean to taking a value or an enum. At least that's my memory, which seems to correspond to the autoupgrade stuff in BitcodeReader. But I think the project's stance on this(like most things) is that "if there's a good reason, then we should do it." At least if we can maintain our stated compatibility guarantees. This is a place where I think we should get additional feedback before proceeding though.

It might be worth asking about this on discourse. Even if we don't want to make such a change in these patches, it may be good for the project to have a policy (or at least guidelines) about changing attributes.

I actually do have a change in PGOInstrumentation.cpp that skips profiling functions with this new attribute. As far as I know, this stack has all the necessary code changes.

I can't believe I missed that. I looked three times before I commented, and blew right passed it every time. That's what happens when I look at code late on a Friday...

[ellis]

Rename omitprofile => skipprofile since this seems slightly better to me. Of course I'm still open to more suggestions.

Also, fix the profile-function-groups.c test.

[phosek]

LGTM

[ellis]

Rebase and update docs

clang/lib/CodeGen/CodeGenFunction.h

private:
   llvm::MDNode *createProfileWeightsForLoop(const Stmt *Cond,
                                             uint64_t LoopCount) const;
 
 public:
   /// Increment the profiler's counter for the given statement by \p StepV.
   /// If \p StepV is null, the default increment is 1.
   void incrementProfileCounter(const Stmt *S, llvm::Value *StepV = nullptr) {
     if (CGM.getCodeGenOpts().hasProfileClangInstr() &&
-        !CurFn->hasFnAttribute(llvm::Attribute::NoProfile))
+        !CurFn->hasFnAttribute(llvm::Attribute::NoProfile) &&
+        !CurFn->hasFnAttribute(llvm::Attribute::SkipProfile))
       PGO.emitCounterIncrement(Builder, S, StepV);
     PGO.setCurrentStmt(S);
   }
 
   /// Get the profiler's count for the given statement.
   uint64_t getProfileCount(const Stmt *S) {
     return PGO.getStmtCount(S).value_or(0);
   }

clang/lib/CodeGen/CodeGenPGO.cpp

if (const auto *CCD = dyn_cast<CXXConstructorDecl>(D))
         return;
   }
   if (isa<CXXDestructorDecl>(D) && GD.getDtorType() != Dtor_Base)
     return;
 
   CGM.ClearUnusedCoverageMapping(D);
   if (Fn->hasFnAttribute(llvm::Attribute::NoProfile))
     return;
+  if (Fn->hasFnAttribute(llvm::Attribute::SkipProfile))
+    return;
 
   setFuncName(Fn);
 
   mapRegionCounters(D);
   if (CGM.getCodeGenOpts().CoverageMapping)
     emitCounterRegionMapping(D);
   if (PGOReader) {
     SourceManager &SM = CGM.getContext().getSourceManager();

clang/test/CodeGen/profile-function-groups.c

-// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=0 -emit-llvm -S %s -o - | FileCheck %s --check-prefixes=CHECK,SELECT0
-// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=1 -emit-llvm -S %s -o - | FileCheck %s --check-prefixes=CHECK,SELECT1
-// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=2 -emit-llvm -S %s -o - | FileCheck %s --check-prefixes=CHECK,SELECT2
+// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=0 -emit-llvm -S %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,SELECT0
+// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=1 -emit-llvm -S %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,SELECT1
+// RUN: %clang -fprofile-generate -fprofile-function-groups=3 -fprofile-selected-function-group=2 -emit-llvm -S %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,SELECT2
 
 // Group 0
-// SELECT0-NOT: noprofile
+
 // SELECT1: noprofile
 // SELECT2: noprofile
 // CHECK: define {{.*}} @hoo()
 void hoo() {}
 
 // Group 1
 // SELECT0: noprofile
-// SELECT1-NOT: noprofile
+
 // SELECT2: noprofile
 // CHECK: define {{.*}} @goo()
 void goo() {}
 
 // Group 2
 // SELECT0: noprofile
 // SELECT1: noprofile
-// SELECT2-NOT: noprofile
+
 // CHECK: define {{.*}} @boo()
 void boo() {}

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 blocks inlining if the caller and callee have different values of this
+    attribute.
+``skipprofile``
+    This function attribute prevents instrumentation based profiling, used for
+    coverage or profile based optimization, from being added to a function. This
+    attribute does not restrict inlining, so instrumented instruction could end
+    up in 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/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_ELEMENTTYPE = 77,
   ATTR_KIND_DISABLE_SANITIZER_INSTRUMENTATION = 78,
   ATTR_KIND_NO_SANITIZE_BOUNDS = 79,
   ATTR_KIND_ALLOC_ALIGN = 80,
   ATTR_KIND_ALLOCATED_POINTER = 81,
   ATTR_KIND_ALLOC_KIND = 82,
   ATTR_KIND_PRESPLIT_COROUTINE = 83,
   ATTR_KIND_FNRETTHUNK_EXTERN = 84,
+  ATTR_KIND_SKIP_PROFILE = 85,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

llvm/include/llvm/IR/Attributes.td

 def NoSync : EnumAttr<"nosync", [FnAttr]>;
 
 /// Disable Indirect Branch Tracking.
 def NoCfCheck : EnumAttr<"nocf_check", [FnAttr]>;
 
 /// Function should not be instrumented.
 def NoProfile : EnumAttr<"noprofile", [FnAttr]>;
 
+/// This function should not be instrumented but it is ok to inline profiled
+// functions into it.
+def SkipProfile : EnumAttr<"skipprofile", [FnAttr]>;
+
 /// Function doesn't unwind stack.
 def NoUnwind : EnumAttr<"nounwind", [FnAttr]>;
 
 /// No SanitizeBounds instrumentation.
 def NoSanitizeBounds : EnumAttr<"nosanitize_bounds", [FnAttr]>;
 
 /// No SanitizeCoverage instrumentation.
 def NoSanitizeCoverage : EnumAttr<"nosanitize_coverage", [FnAttr]>;

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_NO_RETURN:
     return Attribute::NoReturn;
   case bitc::ATTR_KIND_NOSYNC:
     return Attribute::NoSync;
   case bitc::ATTR_KIND_NOCF_CHECK:
     return Attribute::NoCfCheck;
   case bitc::ATTR_KIND_NO_PROFILE:
     return Attribute::NoProfile;
+  case bitc::ATTR_KIND_SKIP_PROFILE:
+    return Attribute::SkipProfile;
   case bitc::ATTR_KIND_NO_UNWIND:
     return Attribute::NoUnwind;
   case bitc::ATTR_KIND_NO_SANITIZE_BOUNDS:
     return Attribute::NoSanitizeBounds;
   case bitc::ATTR_KIND_NO_SANITIZE_COVERAGE:
     return Attribute::NoSanitizeCoverage;
   case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
     return Attribute::NullPointerIsValid;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::NoReturn:
     return bitc::ATTR_KIND_NO_RETURN;
   case Attribute::NoSync:
     return bitc::ATTR_KIND_NOSYNC;
   case Attribute::NoCfCheck:
     return bitc::ATTR_KIND_NOCF_CHECK;
   case Attribute::NoProfile:
     return bitc::ATTR_KIND_NO_PROFILE;
+  case Attribute::SkipProfile:
+    return bitc::ATTR_KIND_SKIP_PROFILE;
   case Attribute::NoUnwind:
     return bitc::ATTR_KIND_NO_UNWIND;
   case Attribute::NoSanitizeBounds:
     return bitc::ATTR_KIND_NO_SANITIZE_BOUNDS;
   case Attribute::NoSanitizeCoverage:
     return bitc::ATTR_KIND_NO_SANITIZE_COVERAGE;
   case Attribute::NullPointerIsValid:
     return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

for (auto &F : M->functions()) {
       unsigned EndLine;
       if (!SP) continue;
       if (!functionHasLines(F, EndLine) || !isFunctionInstrumented(F))
         continue;
       // TODO: Functions using scope-based EH are currently not supported.
       if (isUsingScopeBasedEH(F)) continue;
       if (F.hasFnAttribute(llvm::Attribute::NoProfile))
         continue;
+      if (F.hasFnAttribute(llvm::Attribute::SkipProfile))
+        continue;
 
       // Add the function line number to the lines of the entry block
       // to have a counter for the function definition.
       uint32_t Line = SP->getLine();
       auto Filename = getFilename(SP);
 
       BranchProbabilityInfo *BPI = GetBPI(F);
       BlockFrequencyInfo *BFI = GetBFI(F);

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

static bool InstrumentAllFunctions(
   std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
   collectComdatMembers(M, ComdatMembers);
 
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;
     if (F.hasFnAttribute(llvm::Attribute::NoProfile))
       continue;
+    if (F.hasFnAttribute(llvm::Attribute::SkipProfile))
+      continue;
     auto &TLI = LookupTLI(F);
     auto *BPI = LookupBPI(F);
     auto *BFI = LookupBFI(F);
     instrumentOneFunc(F, &M, TLI, BPI, BFI, ComdatMembers, IsCS);
   }
   return true;
 }
 

llvm/lib/Transforms/Utils/CodeExtractor.cpp

if (Attr.isStringAttribute()) {
       case Attribute::StackProtectReq:
       case Attribute::StackProtectStrong:
       case Attribute::StrictFP:
       case Attribute::UWTable:
       case Attribute::VScaleRange:
       case Attribute::NoCfCheck:
       case Attribute::MustProgress:
       case Attribute::NoProfile:
+      case Attribute::SkipProfile:
         break;
       // These attributes cannot be applied to functions.
       case Attribute::Alignment:
       case Attribute::AllocatedPointer:
       case Attribute::AllocAlign:
       case Attribute::ByVal:
       case Attribute::Dereferenceable:
       case Attribute::DereferenceableOrNull:

llvm/test/Bitcode/attributes.ll

 define void @f86() nosanitize_bounds
 {
         ret void;
 }
 
 ; CHECK: define void @f87() [[FNRETTHUNKEXTERN:#[0-9]+]]
 define void @f87() fn_ret_thunk_extern { ret void }
 
+; CHECK: define void @f88() [[SKIPPROFILE:#[0-9]+]]
+define void @f88() skipprofile { ret void }
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { readnone }
 ; CHECK: attributes #3 = { readonly }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

 ; CHECK: attributes #46 = { vscale_range(1,8) }
 ; CHECK: attributes #47 = { vscale_range(1,0) }
 ; CHECK: attributes #48 = { nosanitize_coverage }
 ; CHECK: attributes #49 = { noprofile }
 ; CHECK: attributes #50 = { disable_sanitizer_instrumentation }
 ; CHECK: attributes #51 = { uwtable(sync) }
 ; CHECK: attributes #52 = { nosanitize_bounds }
 ; CHECK: attributes [[FNRETTHUNKEXTERN]] = { fn_ret_thunk_extern }
+; CHECK: attributes [[SKIPPROFILE]] = { skipprofile }
 ; CHECK: attributes #[[NOBUILTIN]] = { nobuiltin }

llvm/test/Transforms/GCOVProfiling/noprofile.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: mkdir -p %t && cd %t
 ; RUN: opt < %s -passes=insert-gcov-profiling -S | FileCheck %s
 
 ; Test that the noprofile attribute disables profiling.
 define dso_local i32 @no_instr(i32 %a) noprofile !dbg !9 {
 ; CHECK-LABEL: @no_instr(
 ; CHECK-NEXT:    ret i32 42, !dbg [[DBG6:![0-9]+]]
 ;
   ret i32 42, !dbg !27
 }
 
+; Test that the skipprofile attribute disables profiling.
+define dso_local i32 @skip_instr(i32 %a) skipprofile {
+; CHECK-LABEL: @skip_instr(
+; CHECK-NEXT:    ret i32 52
+;
+  ret i32 52
+}
+
 define dso_local i32 @instr(i32 %a) !dbg !28 {
 ; CHECK-LABEL: @instr(
 ; CHECK-NEXT:    [[GCOV_CTR:%.*]] = load i64, ptr @__llvm_gcov_ctr, align 4, !dbg [[DBG8:![0-9]+]]
 ; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[GCOV_CTR]], 1, !dbg [[DBG8]]
 ; CHECK-NEXT:    store i64 [[TMP1]], ptr @__llvm_gcov_ctr, align 4, !dbg [[DBG8]]
 ; CHECK-NEXT:    ret i32 42, !dbg [[DBG8]]
 ;
   ret i32 42, !dbg !44

llvm/test/Transforms/PGOProfile/noprofile.ll

 define i32 @test2() #0 {
 entry:
 ; CHECK-NOT: call void @llvm.instrprof.increment
   %0 = load i32, ptr @i, align 4
   %sub = sub i32 %0, 1
   ret i32 %sub
 }
 
+define i32 @test3() skipprofile {
+entry:
+; CHECK-NOT: call void @llvm.instrprof.increment
+  ret i32 101
+}
+
 attributes #0 = { noprofile }