[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

933

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

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

933

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

57

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

57

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

936

Add a comment here about NS == 0 condition?

[MaskRay]

improve a comment

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

896

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

932–937

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

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

896

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.

932–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

896

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

932–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

896

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.

932–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) {
   // 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));
#include "llvm/ProfileData/InstrProfData.inc"
   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 linker GC cannot discard data variables while the text section is
+  // retained, data variables can be private. This optimization applies on ELF.
+  // On COFF, when DataReferencedByCode is false, __profd_ is never a comdat
+  // leader, this is applicable as well.
+  if (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

 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]]
+; LOWER: @__profd_foo3.[[FOO3_HASH:[0-9]+]] = private {{.*}} @__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()