[PGO] Adjust -vp-counters-per-site.

[hjyamauchi]

Summary

Addressing clang bootstrap under the dynamic linking mode running out of static
allocation of value profile nodes, reported in D81682. The vnodes section size
goes from 1501k to 2171k (+670k bytes).

[dmajor]

Thanks a lot! Glad to see it's fine with a value of 1.5. (We have been setting it to 2 in our build as a temporary workaround)

[davidxl]

how much size increase is in clang instrumented build?

[hjyamauchi]

In D92669#2434247, @davidxl wrote:

how much size increase is in clang instrumented build?

I take it you mean a clang statically-linked build. It goes from 1699k with 1.0 to 2457k with 1.5, which is about a ~760k bytes increase.

[davidxl]

The size increase is significant. It is likely that some very large apps may run into size limit issue at link time with this change.

[hjyamauchi]

Adjust only for dynamic linking mode. Rebase.

[hjyamauchi]

In D92669#2437383, @davidxl wrote:

The size increase is significant. It is likely that some very large apps may run into size limit issue at link time with this change.

Ok. Now adjust the flag value only when the PGO instrumented build and dynamic linking mode.

[davidxl]

llvm/cmake/modules/HandleLLVMOptions.cmake

923

is version 11 when the option is introduced?

[davidxl]

lgtm

[hjyamauchi]

llvm/cmake/modules/HandleLLVMOptions.cmake

923

Version 11 is when the extended value profile buckets was enabled (and presumably this issue started.)

[thakis]

Hm, we tell users not to use Xclang or mllvm flags because they're implementation details. Furthermore, we control the default value of these things. Could we instead change the default value to just work, instead of needing tweaks?

[tambre]

In my case 1.5 is better, but not enough. 2.0 is good and is what I've used so far. However, -vp-counters-per-site can only be specified once so this has broken my builds and I've patched it out for now.
Could we maybe parameterize this or add an option to turn it off?

[hjyamauchi]

I can add "cl::ZeroOrMore" to "-vp-counters-per-site". Would it be good?

[tambre]

In D92669#2452831, @yamauchi wrote:

I can add "cl::ZeroOrMore" to "-vp-counters-per-site". Would it be good?

I pass the option using -DCMAKE_CXX_FLAGS_INIT="-mllvm -vp-counters-per-site=2", which means it gets added before the one with 1.5.

Run Build Command(s):/usr/bin/ninja cmTC_353b0 && [1/2] Building C object CMakeFiles/cmTC_353b0.dir/src.c.o
FAILED: CMakeFiles/cmTC_353b0.dir/src.c.o
/usr/bin/cc -D_GNU_SOURCE  -g -fdebug-prefix-map=/opt/deb/llvm=. -Wformat -Werror=format-security -O3 -Wdate-time -march=skylake -fsplit-machine-functions -mllvm -vp-counters-per-site=2 -fPIC -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wno-comment -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fprofile-generate="/var/tmp/pgo/clang/ir" -Xclang -mllvm -Xclang -vp-counters-per-site=1.5 -flto=thin -DHAVE_DECL__BITSCANFORWARD  -Werror=unguarded-availability-new -MD -MT CMakeFiles/cmTC_353b0.dir/src.c.o -MF CMakeFiles/cmTC_353b0.dir/src.c.o.d -o CMakeFiles/cmTC_353b0.dir/src.c.o -c src.c
clang (LLVM option parsing): for the --vp-counters-per-site option: may only occur zero or one times!

I don't think that'd result in 2 being used with your proposed fix?

Logically -vp-counters-per-site only makes sense to be specified once. I think parameterizing the LLVM CMake code would be better than changing LLVM itself.
Patch: D93281

[hjyamauchi]

D93281 looks good to me.

llvm/cmake/modules/HandleLLVMOptions.cmake

if (LLVM_ENABLE_IR_PGO OR uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "IR")
     append("-fprofile-generate=\"${LLVM_PROFILE_DATA_DIR}\""
       CMAKE_CXX_FLAGS
       CMAKE_C_FLAGS)
     if(NOT LINKER_IS_LLD_LINK)
         append("-fprofile-generate=\"${LLVM_PROFILE_DATA_DIR}\""
           CMAKE_EXE_LINKER_FLAGS
           CMAKE_SHARED_LINKER_FLAGS)
     endif()
+    # Set this to avoid running out of the value profile node section
+    # under clang in dynamic linking mode.
+    if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND
+        CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 11 AND
+        LLVM_LINK_LLVM_DYLIB)
+      append("-Xclang -mllvm -Xclang -vp-counters-per-site=1.5"
+        CMAKE_CXX_FLAGS
+        CMAKE_C_FLAGS)
+    endif()
   elseif(uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "CSIR")
     append("-fcs-profile-generate=\"${LLVM_CSPROFILE_DATA_DIR}\""
       CMAKE_CXX_FLAGS
       CMAKE_C_FLAGS)
     if(NOT LINKER_IS_LLD_LINK)
       append("-fcs-profile-generate=\"${LLVM_CSPROFILE_DATA_DIR}\""
         CMAKE_EXE_LINKER_FLAGS
         CMAKE_SHARED_LINKER_FLAGS)