[InstrProfiling][ELF] Make __profd_ private if the function does not use value profiling

[MaskRay]

Summary

On ELF, the D103372 optimization can apply to more cases. There are two
prerequisites for making __profd_ private:

• __profc_ keeps __profd_ live under compiler/linker GC
• __profd_ is not referenced by code

The first is satisfied because all counters/data are in a section group (either
comdat any or comdat noduplicates). The second requires that the function
does not use value profiling.

Regarding the second point: __profd_ may be referenced by other text sections
due to inlining. There will be a linker error if a prevailing text section
references the non-prevailing local symbol.

With this change, a stage 2 (-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR)
clang is 4.2% smaller (1-169620032/177066968).

[rnk]

We are observing some crashes in coverage, but we haven't root caused them yet: crbug.com/1216811
So I guess we need to keep waiting for the dust to settle. @aeubanks was able to repro the crash there, but it's not deterministic.

[aeubanks]

In D103717#2806311, @rnk wrote:

We are observing some crashes in coverage, but we haven't root caused them yet: crbug.com/1216811
So I guess we need to keep waiting for the dust to settle. @aeubanks was able to repro the crash there, but it's not deterministic.

This ended up being unrelated

[MaskRay]

I think the previous Instrumentation/InstrProfiling.cpp changes can be considered stable now. So, ping:)

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

930

Is the lld specific behavior, or it applies to gold and ld.bfd?

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

930

It is generic for ELF, applicable to all the linkers.

Other binary formats may have limitation.

[rnk]

lgtm

llvm/test/Transforms/PGOProfile/indirect_call_profile.ll

91–92

I don't think we need to append hashes to symbols if they will ultimately be private. Seems like future work, though.

[davidxl]

lgtm.

[nathanchance]

This patch breaks a two stage instrumented build::

$ cmake \
    -B build/stage1 \
    -G Ninja \
    -Wno-dev \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=clang \
    -DCMAKE_CXX_COMPILER=clang++ \
    -DLLVM_CCACHE_BUILD=ON \
    -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;lld" \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=lld \
    llvm

$ ninja -C build/stage1

$ export PATH=$PWD/build/stage1/bin:$PATH

$ cmake \
    -B build/stage2 \
    -G Ninja \
    -Wno-dev \
    -DCLANG_TABLEGEN=$(command -v clang-tblgen) \
    -DCMAKE_AR=$(command -v llvm-ar) \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=$(command -v clang) \
    -DCMAKE_CXX_COMPILER=$(command -v clang++) \
    -DCMAKE_RANLIB=$(command -v llvm-ranlib) \
    -DLLVM_BUILD_INSTRUMENTED=IR \
    -DLLVM_BUILD_RUNTIME=OFF \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_PROJECTS="clang;lld" \
    -DLLVM_ENABLE_TERMINFO=OFF \
    -DLLVM_ENABLE_WARNINGS=OFF \
    -DLLVM_TABLEGEN=$(command -v llvm-tblgen) \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=$(command -v ld.lld) \
    llvm

$ ninja -C build/stage2
...
ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 232 more times

ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 8 more times
...

[MaskRay]

In D103717#2826247, @nathanchance wrote:

This patch breaks a two stage instrumented build::

$ cmake \
    -B build/stage1 \
    -G Ninja \
    -Wno-dev \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=clang \
    -DCMAKE_CXX_COMPILER=clang++ \
    -DLLVM_CCACHE_BUILD=ON \
    -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;lld" \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=lld \
    llvm

$ ninja -C build/stage1

$ export PATH=$PWD/build/stage1/bin:$PATH

$ cmake \
    -B build/stage2 \
    -G Ninja \
    -Wno-dev \
    -DCLANG_TABLEGEN=$(command -v clang-tblgen) \
    -DCMAKE_AR=$(command -v llvm-ar) \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=$(command -v clang) \
    -DCMAKE_CXX_COMPILER=$(command -v clang++) \
    -DCMAKE_RANLIB=$(command -v llvm-ranlib) \
    -DLLVM_BUILD_INSTRUMENTED=IR \
    -DLLVM_BUILD_RUNTIME=OFF \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_PROJECTS="clang;lld" \
    -DLLVM_ENABLE_TERMINFO=OFF \
    -DLLVM_ENABLE_WARNINGS=OFF \
    -DLLVM_TABLEGEN=$(command -v llvm-tblgen) \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=$(command -v ld.lld) \
    llvm

$ ninja -C build/stage2
...
ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 232 more times

ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 8 more times
...

Thanks for reporting the issue. Reverted in 5798be84580be233e4cf34c08ceec8f79e80502e (which has detailed descriptions)
I made a mistake in my stage-2 testing so I wasn't actually testing stage-2 so failed to catch the static value profiler counter issue...

[MaskRay]

I need to add a test case, but this is the sketch.

I am not sure whether this value profiling detection is correct.

[MaskRay]

A stage-2 -DLLVM_BUILD_INSTRMENTED=IR clang is 4.2% smaller (1-169620032/177066968) with this patch.

[MaskRay]

add test

[MaskRay]

llvm/test/Transforms/PGOProfile/indirect_call_profile.ll

91–92

It doesn't. __profd_foo3.[[FOO3_HASH:[0-9]+]] has no harm because such a private linkage variable will not need a symbol table entry.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

933

Add a comment here about NS == 0 condition?

[MaskRay]

improve a comment

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

890

should this code by guarded by whether value profile is enabled?

929–937

What is the root cause of the failure when NS == 0 condition is not used?

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

890

For performance?

The code below iterates over the array unconditionally

for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
  Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);

so I think having another loop should be fine.

929–937

5798be84580be233e4cf34c08ceec8f79e80502e has the message.

Say a.cc has a prevailing comdat and b.cc's is non-prevailing.

b.cc's linkonce_odr comdat function may get inlined into a function not in a.cc.
It is a linker error for the prevailing function to reference a discarded STB_LOCAL symbol (__profd_ is non-prevailing and thus discarded)

If we don't change the linkage, __profd_ is STB_GLOBAL STV_HIDDEN. It is fine to reference such a non-local symbol, which will bind to a.cc's copy.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

890

Basically in that case, there is no need to create the variable in the first place.

929–937

When ValueProfileStaticAlloc is false, NS will also be zero, but the prf_data will still be referenced -- will it create the same linker error?

If that is the case, I think a better condition for ELF should be :

if (ValueProfileStaticAlloc && NS == 0 || !DataReferencedByCode) ...

!DataReferencedByCode is a bigger hammer to catch all.

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

890

The loop has 2 iterations, so I think should be fine.

for (uint32_t Kind = 0; Kind <= 1; ++Kind)

We could use zeroinitializer but that would require that we have other means to know @llvm.instrprof.value.profile isn't used.

929–937

computeNumValueSiteCounts visits every @llvm.instrprof.value.profile instruction.

If computeNumValueSiteCounts is called, NS will be non-zero. So I guess it is a good indicator, regardless of ValueProfileStaticAlloc.

[davidxl]

Correction. With the way NS is unconditionally computed, using NS==0 check is fine, but I think it is cleaner (easier to read) perhaps to check ValuesVar nullness when static allocation is on or fallback to global option check.

[davidxl]

On second thought, NS == 0 is a reasonablly good indicator.

[MaskRay]

Use ValuesVar instead of NS

[davidxl]

See my prior comment -- your previous revision is sound and LGTM. If we go with this version, we do need to examine whether static allocation is on or not. Sorry for the trouble.

[MaskRay]

fix condition and improve test

[davidxl]

lgtm.

[MaskRay]

switch to the NS==0 approach after seeing See my prior comment -- your previous revision is sound and LGTM. If we go with this version, we do need to examine whether static allocation is on or not. Sorry for the trouble.
(did not notice in my previous test update...)

[davidxl]

The LGTM remains the same.

[MaskRay]

minor adjustment to comment and test

[MaskRay]

Thanks for review:) I feel that I understand value profiling better now.

I have a plan to use label differences (i.e. PC-relative relocations on ELF) (D104556) to save sizeof(Elf64_Rela) * |__profd_| = 24 * instrumented_functions bytes.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   CounterPtr->setAlignment(Align(8));
   MaybeSetComdat(CounterPtr);
   CounterPtr->setLinkage(Linkage);
 
   auto *Int8PtrTy = Type::getInt8PtrTy(Ctx);
   // Allocate statically the array of pointers to value profile nodes for
   // the current function.
   Constant *ValuesPtrExpr = ConstantPointerNull::get(Int8PtrTy);
-  if (ValueProfileStaticAlloc && !needsRuntimeRegistrationOfSectionRange(TT)) {
-    uint64_t NS = 0;
-    for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
-      NS += PD.NumValueSites[Kind];
-    if (NS) {
-      ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
-
-      auto *ValuesVar =
-          new GlobalVariable(*M, ValuesTy, false, Linkage,
-                             Constant::getNullValue(ValuesTy),
-                             getVarName(Inc, getInstrProfValuesVarPrefix()));
-      ValuesVar->setVisibility(Visibility);
-      ValuesVar->setSection(
-          getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
-      ValuesVar->setAlignment(Align(8));
-      MaybeSetComdat(ValuesVar);
-      ValuesPtrExpr =
-          ConstantExpr::getBitCast(ValuesVar, Type::getInt8PtrTy(Ctx));
-    }
-  }
+  if (NS > 0 && ValueProfileStaticAlloc &&
+      !needsRuntimeRegistrationOfSectionRange(TT)) {
+    auto *ValuesVar = new GlobalVariable(
+        *M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
 
   // Create data variable.
   auto *Int16Ty = Type::getInt16Ty(Ctx);
   auto *Int16ArrayTy = ArrayType::get(Int16Ty, IPVK_Last + 1);
   Type *DataTypes[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) LLVMType,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
   auto *DataTy = StructType::get(Ctx, makeArrayRef(DataTypes));
 
   Constant *FunctionAddr = shouldRecordFunctionAddr(Fn)
                                ? ConstantExpr::getBitCast(Fn, Int8PtrTy)
                                : ConstantPointerNull::get(Int8PtrTy);
 
   Constant *Int16ArrayVals[IPVK_Last + 1];
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
 
   Constant *DataVals[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
-  // If code never references data variables (the symbol is unneeded), and
-  // linker GC cannot discard data variables while the text section is retained,
-  // data variables can be private. This optimization applies on COFF and ELF.
-  if (!DataReferencedByCode && !TT.isOSBinFormatMachO()) {
+  // If the data variable is not referenced by code (if we don't emit
+  // @llvm.instrprof.value.profile, NS will be 0), and the counter keeps the
+  // data variable live under linker GC, the data variable can be private. This
+  // optimization applies to ELF.
+  //
+  // On COFF, we additionally require that __profd_ is not a comdat leader, i.e.
+  // DataReferencedByCode is false.
+  if (NS == 0 && (TT.isOSBinFormatELF() ||
+                  (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
     Linkage = GlobalValue::PrivateLinkage;
     Visibility = GlobalValue::DefaultVisibility;
   }
   auto *Data =
       new GlobalVariable(*M, DataTy, false, Linkage,
                          ConstantStruct::get(DataTy, DataVals), DataVarName);
   Data->setVisibility(Visibility);
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));

llvm/test/Transforms/PGOProfile/indirect_call_profile.ll

 ; RUN: opt < %s -pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
 ; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
 ; RUN: opt < %s -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefix=LOWER
+; RUN: opt < %s -passes=pgo-instr-gen,instrprof -vp-static-alloc=false -S | FileCheck %s --check-prefix=LOWER
 
 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"
 
-$foo3 = comdat any
+$novp_inline = comdat any
+$vp_inline = comdat any
 
 @bar = external global void ()*, align 8
+
+; GEN: @__profn_novp_inline = linkonce_odr hidden constant [11 x i8] c"novp_inline"
 ; GEN: @__profn_foo = private constant [3 x i8] c"foo"
+; GEN: @__profn_vp_inline = linkonce_odr hidden constant [9 x i8] c"vp_inline"
+
+;; Test that a linkonce function's address is recorded.
+;; We allow a linkonce profd to be private if the function does not use value profiling.
+; LOWER:      @__profd_novp_inline.[[HASH:[0-9]+]] = private global {{.*}} @__profc_novp_inline.[[HASH]]
+; LOWER-SAME:   i8* bitcast (void ()* @novp_inline to i8*)
+; LOWER:      @__profd_foo = private {{.*}} @__profc_foo
+
+;; __profd_vp_inline.[[#]] is referenced by code and may be referenced by other
+;; text sections due to inlining. It can't be local because a linker error would
+;; occur if a prevailing text section references the non-prevailing local symbol.
+; LOWER:      @__profd_vp_inline.[[FOO_HASH:[0-9]+]] = linkonce_odr hidden {{.*}} @__profc_vp_inline.[[FOO_HASH]]
+; LOWER-SAME:   i8* bitcast (void ()* @vp_inline to i8*)
+
+define linkonce_odr void @novp_inline() comdat {
+  ret void
+}
 
 define void @foo() {
 entry:
+; GEN: @foo()
 ; GEN: entry:
-; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[FOO_HASH:[0-9]+]], i32 1, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[#FOO_HASH:]], i32 1, i32 0)
   %tmp = load void ()*, void ()** @bar, align 8
 ; GEN: [[ICALL_TARGET:%[0-9]+]] = ptrtoint void ()* %tmp to i64
-; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[FOO_HASH]], i64 [[ICALL_TARGET]], i32 0, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[#FOO_HASH]], i64 [[ICALL_TARGET]], i32 0, i32 0)
+; LOWER: call void @__llvm_profile_instrument_target(i64 %1, i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [2 x i16] }* @__profd_foo to i8*), i32 0)
+  call void %tmp()
+  ret void
+}
+
+define linkonce_odr void @vp_inline() comdat {
+entry:
+; GEN: @vp_inline()
+; GEN: entry:
+; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @__profn_vp_inline, i32 0, i32 0), i64 [[#FOO_HASH:]], i32 1, i32 0)
+  %tmp = load void ()*, void ()** @bar, align 8
+; GEN: [[ICALL_TARGET:%[0-9]+]] = ptrtoint void ()* %tmp to i64
+; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @__profn_vp_inline, i32 0, i32 0), i64 [[#FOO_HASH]], i64 [[ICALL_TARGET]], i32 0, i32 0)
+; LOWER: call void @__llvm_profile_instrument_target(i64 %1, i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [2 x i16] }* @__profd_vp_inline.[[#]] to i8*), i32 0)
   call void %tmp()
   ret void
 }
 
 @bar2 = global void ()* null, align 8
 @_ZTIi = external constant i8*
 
 define i32 @foo2(i32 %arg, i8** nocapture readnone %arg1) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
bb7:                                              ; preds = %bb2
   br label %bb10
 
 bb10:                                             ; preds = %bb7, %bb
   ret i32 0
 
 bb11:                                             ; preds = %bb2
   resume { i8*, i32 } %tmp3
 }
 
-; Test that comdat function's address is recorded.
-; LOWER: @__profd_foo3.[[FOO3_HASH:[0-9]+]] = linkonce_odr{{.*}}@__profc_foo3.[[FOO3_HASH]]
-; Function Attrs: nounwind uwtable
-define linkonce_odr i32 @foo3()  comdat  {
-  ret i32 1
-}
-
 declare i32 @__gxx_personality_v0(...)
 
 ; Function Attrs: nounwind readnone
 declare i32 @llvm.eh.typeid.for(i8*) #0
 
 declare i8* @__cxa_begin_catch(i8*)
 
 declare void @__cxa_end_catch()