Skip to content

Conversation

ZequanWu
Copy link
Contributor

As discussed in #70856 (comment) and #70856 (comment), it's better not to do runtime check for VARIANT_MASK_DBG_CORRELATE bit in __llvm_profile_raw_version when deciding if profile data/name sections should be dropped or not.

As discussed in llvm#70856 (comment) and llvm#70856 (comment), it's better not to do runtime check for VARIANT_MASK_DBG_CORRELATE bit in __llvm_profile_raw_version when deciding if profile data/name sections should be dropped or not.
@llvmbot llvmbot added compiler-rt PGO Profile Guided Optimizations labels Nov 10, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-pgo

Author: Zequan Wu (ZequanWu)

Changes

As discussed in #70856 (comment) and #70856 (comment), it's better not to do runtime check for VARIANT_MASK_DBG_CORRELATE bit in __llvm_profile_raw_version when deciding if profile data/name sections should be dropped or not.


Full diff: https://github.com/llvm/llvm-project/pull/71996.diff

5 Files Affected:

  • (modified) compiler-rt/lib/profile/InstrProfiling.c (-4)
  • (modified) compiler-rt/lib/profile/InstrProfiling.h (-3)
  • (modified) compiler-rt/lib/profile/InstrProfilingBuffer.c (-6)
  • (modified) compiler-rt/lib/profile/InstrProfilingMerge.c (+3-3)
  • (modified) compiler-rt/lib/profile/InstrProfilingWriter.c (+5-8)
diff --git a/compiler-rt/lib/profile/InstrProfiling.c b/compiler-rt/lib/profile/InstrProfiling.c
index 51477c6080559d0..da04d8ebdec95bb 100644
--- a/compiler-rt/lib/profile/InstrProfiling.c
+++ b/compiler-rt/lib/profile/InstrProfiling.c
@@ -89,7 +89,3 @@ COMPILER_RT_VISIBILITY void __llvm_profile_reset_counters(void) {
   }
   lprofSetProfileDumped(0);
 }
-
-COMPILER_RT_VISIBILITY int __llvm_profile_has_correlation() {
-  return (__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE) != 0ULL;
-}
diff --git a/compiler-rt/lib/profile/InstrProfiling.h b/compiler-rt/lib/profile/InstrProfiling.h
index 3c79786e56c48ce..c5b0b34f2d8af03 100644
--- a/compiler-rt/lib/profile/InstrProfiling.h
+++ b/compiler-rt/lib/profile/InstrProfiling.h
@@ -261,9 +261,6 @@ uint64_t __llvm_profile_get_magic(void);
 /*! \brief Get the version of the file format. */
 uint64_t __llvm_profile_get_version(void);
 
-/*! \brief If the binary is compiled with profile correlation. */
-int __llvm_profile_has_correlation();
-
 /*! \brief Get the number of entries in the profile data section. */
 uint64_t __llvm_profile_get_num_data(const __llvm_profile_data *Begin,
                                      const __llvm_profile_data *End);
diff --git a/compiler-rt/lib/profile/InstrProfilingBuffer.c b/compiler-rt/lib/profile/InstrProfilingBuffer.c
index 87546f6c9e4f284..cd1f067bd188e47 100644
--- a/compiler-rt/lib/profile/InstrProfilingBuffer.c
+++ b/compiler-rt/lib/profile/InstrProfilingBuffer.c
@@ -56,8 +56,6 @@ uint64_t __llvm_profile_get_size_for_buffer(void) {
 COMPILER_RT_VISIBILITY
 uint64_t __llvm_profile_get_num_data(const __llvm_profile_data *Begin,
                                      const __llvm_profile_data *End) {
-  if (__llvm_profile_has_correlation())
-    return 0;
   intptr_t BeginI = (intptr_t)Begin, EndI = (intptr_t)End;
   return ((EndI + sizeof(__llvm_profile_data) - 1) - BeginI) /
          sizeof(__llvm_profile_data);
@@ -66,8 +64,6 @@ uint64_t __llvm_profile_get_num_data(const __llvm_profile_data *Begin,
 COMPILER_RT_VISIBILITY
 uint64_t __llvm_profile_get_data_size(const __llvm_profile_data *Begin,
                                       const __llvm_profile_data *End) {
-  if (__llvm_profile_has_correlation())
-    return 0;
   return __llvm_profile_get_num_data(Begin, End) * sizeof(__llvm_profile_data);
 }
 
@@ -98,8 +94,6 @@ uint64_t __llvm_profile_get_num_bitmap_bytes(const char *Begin,
 
 COMPILER_RT_VISIBILITY
 uint64_t __llvm_profile_get_name_size(const char *Begin, const char *End) {
-  if (__llvm_profile_has_correlation())
-    return 0;
   return End - Begin;
 }
 
diff --git a/compiler-rt/lib/profile/InstrProfilingMerge.c b/compiler-rt/lib/profile/InstrProfilingMerge.c
index da43653865663fe..e8ae2189cccdba2 100644
--- a/compiler-rt/lib/profile/InstrProfilingMerge.c
+++ b/compiler-rt/lib/profile/InstrProfilingMerge.c
@@ -139,9 +139,9 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
   if (SrcNameStart < SrcCountersStart || SrcNameStart < SrcBitmapStart)
     return 1;
 
-  // Merge counters by iterating the entire counter section when correlation is
-  // enabled.
-  if (__llvm_profile_has_correlation()) {
+  // Merge counters by iterating the entire counter section when data section is
+  // empty due to correlation.
+  if (Header->NumData == 0) {
     for (SrcCounter = SrcCountersStart,
         DstCounter = __llvm_profile_begin_counters();
          SrcCounter < SrcCountersEnd;) {
diff --git a/compiler-rt/lib/profile/InstrProfilingWriter.c b/compiler-rt/lib/profile/InstrProfilingWriter.c
index ac096bd1e5480fd..4d767d138514857 100644
--- a/compiler-rt/lib/profile/InstrProfilingWriter.c
+++ b/compiler-rt/lib/profile/InstrProfilingWriter.c
@@ -262,8 +262,6 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin,
                    const char *BitmapBegin, const char *BitmapEnd,
                    VPDataReaderType *VPDataReader, const char *NamesBegin,
                    const char *NamesEnd, int SkipNameDataWrite) {
-  int ProfileCorrelation = __llvm_profile_has_correlation();
-
   /* Calculate size of sections. */
   const uint64_t DataSectionSize =
       __llvm_profile_get_data_size(DataBegin, DataEnd);
@@ -302,7 +300,7 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin,
 #endif
 
   /* The data and names sections are omitted in lightweight mode. */
-  if (ProfileCorrelation) {
+  if (NumData == 0 && NamesSize == 0) {
     Header.CountersDelta = 0;
     Header.NamesDelta = 0;
   }
@@ -318,22 +316,21 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin,
 
   /* Write the profile data. */
   ProfDataIOVec IOVecData[] = {
-      {ProfileCorrelation ? NULL : DataBegin, sizeof(uint8_t), DataSectionSize,
-       0},
+      {DataBegin, sizeof(uint8_t), DataSectionSize, 0},
       {NULL, sizeof(uint8_t), PaddingBytesBeforeCounters, 1},
       {CountersBegin, sizeof(uint8_t), CountersSectionSize, 0},
       {NULL, sizeof(uint8_t), PaddingBytesAfterCounters, 1},
       {BitmapBegin, sizeof(uint8_t), NumBitmapBytes, 0},
       {NULL, sizeof(uint8_t), PaddingBytesAfterBitmapBytes, 1},
-      {(SkipNameDataWrite || ProfileCorrelation) ? NULL : NamesBegin,
-       sizeof(uint8_t), NamesSize, 0},
+      {SkipNameDataWrite ? NULL : NamesBegin, sizeof(uint8_t), NamesSize, 0},
       {NULL, sizeof(uint8_t), PaddingBytesAfterNames, 1}};
   if (Writer->Write(Writer, IOVecData, sizeof(IOVecData) / sizeof(*IOVecData)))
     return -1;
 
   /* Value profiling is not yet supported in continuous mode and profile
    * correlation mode. */
-  if (__llvm_profile_is_continuous_mode_enabled() || ProfileCorrelation)
+  if (__llvm_profile_is_continuous_mode_enabled() ||
+      (NumData == 0 && NamesSize == 0))
     return 0;
 
   return writeValueProfData(Writer, VPDataReader, DataBegin, DataEnd);

@ZequanWu ZequanWu merged commit 0358825 into llvm:main Nov 14, 2023
@ZequanWu ZequanWu deleted the remove-has-correlation branch November 14, 2023 19:17
ZequanWu added a commit to ZequanWu/llvm-project that referenced this pull request Nov 15, 2023
…s` (or `.lprfcovdata` and `.lprfcovnames` on COFF) under binary correlation mode.

The reason is that we don't want to rely on the bit in llvm profile version variable to indicate name/data sections are empty or not: llvm#71996. And simply making __llvm_prf_{data}/{names} sections not allocated in ELF does not make `&__stop___llvm_prf_data - &__start___llvm_prf_data == 0`. It still returns the size of the non-allocated sections. So, using different section names eliminate the need to perform runtime check for data/name section size.

With that, there is no need to have VARIANT_MASK_BIN_CORRELATE bit. Clang side codegen changes are not longer necessary.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
As discussed in
llvm#70856 (comment)
and
llvm#70856 (comment),
it's better not to do runtime check for VARIANT_MASK_DBG_CORRELATE bit
in __llvm_profile_raw_version when deciding if profile data/name
sections should be dropped or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants