[InstrProfiling] No runtime hook for unused funcs

[gulfem]

Summary

CoverageMappingModuleGen generates a coverage mapping record
even for unused functions with internal linkage, e.g.
static int foo() { return 100; }
Clang frontend eliminates such functions, but InstrProfiling pass
still pulls in profile runtime since there is a coverage record.
Fuchsia uses runtime counter relocation, and pulling in profile
runtime for unused functions causes a linker error:
undefined hidden symbol: __llvm_profile_counter_bias.
Since 389dc94d4be7, we do not hook profile runtime for the binaries
that none of its translation units have been instrumented in Fuchsia.
This patch extends that for the instrumented binaries that
consist of only unused functions.

[phosek]

LGTM

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

569–573

This could be combined into a single statement.

[gulfem]

Addressed phosek's comment.

[davidxl]

Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing.

[gulfem]

In D122336#3405835, @davidxl wrote:

Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing.

@davidxl,
I originally did that (not generating coverage records) because I think it is a cleaner way.
But, after reading https://reviews.llvm.org/D43794, it's my understanding is that we might be generating coverage record for unused functions for TAPI.
@vsk can clarify that. If there is no such restriction anymore, I can change that.

[vsk]

Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.

it's my understanding is that we might be generating coverage record for unused functions for TAPI.

Coverage function records are emitted for unused functions because the tooling needs to know which file/line ranges require a "0" execution count.
CC'ing @cishida for tapi expertise. My (possibly outdated) understanding is that for tapi, we always emit the runtime hook because it analyzes compilation flags to determine the expected exported symbol set.

[gulfem]

In D122336#3406186, @vsk wrote:

Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.

it's my understanding is that we might be generating coverage record for unused functions for TAPI.

Coverage function records are emitted for unused functions because the tooling needs to know which file/line ranges require a "0" execution count.

Thanks for the background @vsk. That means that we need to generate a coverage record even for unused functions.
Do you have any objection to the current solution (not pulling in profile runtime for such cases)?

[vsk]

So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected.

[gulfem]

In D122336#3406843, @vsk wrote:

So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected.

I don't think it should impact TAPI because it relies on needsRuntimeHookUnconditionally function, which only returns false for Fuchsia.
So, this patch only affects the behavior of pulling in profile runtime for unused functions in Fuchsia.

bool needsRuntimeHookUnconditionally(const Triple &TT) {
  // On Fuchsia, we only need runtime hook if any counters are present.
  if (TT.isOSFuchsia())
    return false;

  return true;
}

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

569

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

That said, nested if is usually written as a single if with &&

[gulfem]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

569

Thanks for the tip @MaskRay, and I'm going to fix that in a following patch.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

569

It would be helpful to add a comment in the code reflecting the review discussions.

[gulfem]

@davidxl,
I added comments in https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2

@MaskRay,
I followed the style guide for nested if statements in https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2

clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp

+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

bool InstrProfiling::run(
   NamesVar = nullptr;
   NamesSize = 0;
   ProfileDataMap.clear();
   CompilerUsedVars.clear();
   UsedVars.clear();
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
     MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
       M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
+  if (!containsProfilingIntrinsics(M)) {
+    if (!CoverageNamesVar || !NeedsRuntimeHook) {
       return MadeChange;
+    }
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
   // target value sites to enter it as field in the profile data variable.
   for (Function &F : M) {
     InstrProfIncrementInst *FirstProfIncInst = nullptr;
     for (BasicBlock &BB : F)
       for (auto I = BB.begin(), E = BB.end(); I != E; I++)
         if (auto *Ind = dyn_cast<InstrProfValueProfileInst>(I))