[BOLT][NFC] Unify two symbol table iterations

[corona10]

Simple refactoring which was suggested from #90661 (comment)

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Donghee Na (corona10)

Changes

Simple refactoring which was suggested from #90661 (comment)

---

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

1 Files Affected:

• (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+21-21)

diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 23f79e3c135a78..2e91856c0ebfe6 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -820,7 +820,26 @@ void RewriteInstance::discoverFileObjects() {
       return std::hash<decltype(DataRefImpl::p)>{}(S.getRawDataRefImpl().p);
     }
   };
+
+  // Sort symbols in the file by value. Ignore symbols from non-allocatable
+  // sections. We memoize getAddress(), as it has rather high overhead.
+  struct SymbolInfo {
+    uint64_t Address;
+    SymbolRef Symbol;
+  };
+  auto isSymbolInMemory = [this](const SymbolRef &Sym) {
+    if (cantFail(Sym.getType()) == SymbolRef::ST_File)
+      return false;
+    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Absolute)
+      return true;
+    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Undefined)
+      return false;
+    BinarySection Section(*BC, *cantFail(Sym.getSection()));
+    return Section.isAllocatable();
+  };
+  std::vector<SymbolInfo> SortedSymbols;
   std::unordered_map<SymbolRef, StringRef, SymbolRefHash> SymbolToFileName;
+
   for (const ELFSymbolRef &Symbol : InputFile->symbols()) {
     Expected<StringRef> NameOrError = Symbol.getName();
     if (NameOrError && NameOrError->starts_with("__asan_init")) {
@@ -835,6 +854,8 @@ void RewriteInstance::discoverFileObjects() {
              "support. Cannot optimize.\n";
       exit(1);
     }
+    if (isSymbolInMemory(Symbol))
+      SortedSymbols.push_back({cantFail(Symbol.getAddress()), Symbol});
 
     if (cantFail(Symbol.getFlags()) & SymbolRef::SF_Undefined)
       continue;
@@ -856,27 +877,6 @@ void RewriteInstance::discoverFileObjects() {
       SymbolToFileName[Symbol] = FileSymbolName;
   }
 
-  // Sort symbols in the file by value. Ignore symbols from non-allocatable
-  // sections. We memoize getAddress(), as it has rather high overhead.
-  struct SymbolInfo {
-    uint64_t Address;
-    SymbolRef Symbol;
-  };
-  std::vector<SymbolInfo> SortedSymbols;
-  auto isSymbolInMemory = [this](const SymbolRef &Sym) {
-    if (cantFail(Sym.getType()) == SymbolRef::ST_File)
-      return false;
-    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Absolute)
-      return true;
-    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Undefined)
-      return false;
-    BinarySection Section(*BC, *cantFail(Sym.getSection()));
-    return Section.isAllocatable();
-  };
-  for (const SymbolRef &Symbol : InputFile->symbols())
-    if (isSymbolInMemory(Symbol))
-      SortedSymbols.push_back({cantFail(Symbol.getAddress()), Symbol});
-
   auto CompareSymbols = [this](const SymbolInfo &A, const SymbolInfo &B) {
     if (A.Address != B.Address)
       return A.Address < B.Address;

[corona10]

I checked the build success on my local machine but not familiar to run the BOLT only test, is there any guide exists?

[corona10]

I checked the build success on my local machine but not familiar to run the BOLT only test, is there any guide exists?

Okay ninja check-bolt works.

[aaupov]

Thanks Donghee! Looks good, but please retitle to "[BOLT][NFC] Unify two symbol table iterations".
One way to estimate the effect (in seconds) would be to exit(0) just above auto CompareSymbols and measure processing time before and after this fix. Can you please check with a large binary and add the information to the PR message?

[corona10]

Can you please check with a large binary and add the information to the PR message?

Yeah, I will share the info soon :)

[corona10]

@aaupov
For the CPython build(11083 symbols): 4693µs -> 4194µs (Constantly enhanced) from my environment with handy chrono-based measurement.
But when considering the possibility of noise, I am setting up the clang build.

[corona10]

https://github.com/llvm/llvm-project/blob/main/bolt/docs/OptimizingClang.md

Is this still recommened setup?

[aaupov]

https://github.com/llvm/llvm-project/blob/main/bolt/docs/OptimizingClang.md

Is this still recommened setup?

The guide covers more than what we need in this case. You can just run on BOLT on any Clang binary, for instance, you can try BOLT instrumentation:

llvm-bolt clang-18 -instrument -o clang-18.bolt.inst

[corona10]

Hmm, my system clang-18 and pre-built clang binary from llvm-release page is not able to be processed because of the compile option (in my guess)

BOLT-ERROR: instrumentation runtime libraries require relocation

[aaupov]

Hmm, my system clang-18 and pre-built clang binary from llvm-release page is not able to be processed because of the compile option (in my guess)

BOLT-ERROR: instrumentation runtime libraries require relocation

Sorry, yes, instrumentation requires relocations in the input binary. I've checked that rewriting in -lite=0 mode works:

llvm-bolt clang-18 -lite=0 -o /dev/null -time-rewrite

[corona10]

There was a binary issue from Fedora 39 and I tested with release pre-built binary from GitHub that given more proper information
But anyway, for me, it's noisy level changes.

CPU: AMD Ryzen™ 7 8845HS
Memory: 32gb

AS-IS
---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
0.0789 ( 21.2%) 0.0063 ( 2.1%) 0.0851 ( 12.6%) 0.0854 ( 12.6%) discover file objects

TO-BE
---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
0.0804 ( 20.3%) 0.0046 ( 1.6%) 0.0850 ( 12.3%) 0.0853 ( 12.3%) discover file objects

[aaupov]

Let me test it with a large binary.

[aaupov]

To isolate the effect of the change, I added exit(0) here just after the second loop:

llvm-project/bolt/lib/Rewrite/RewriteInstance.cpp

Lines 876 to 880 in ffc9a30

	for (const SymbolRef &Symbol : InputFile->symbols())
	if (isSymbolInMemory(Symbol))
	SortedSymbols.push_back({cantFail(Symbol.getAddress()), Symbol});
	auto CompareSymbols = [this](const SymbolInfo &A, const SymbolInfo &B) {

For chromium 5735 with 1.2M symbols on Intel ADL i7-12700K the effect is the following (warmup=3, 10 runs):
before:

  Time (mean ± σ):     400.6 ms ±   1.0 ms    [User: 354.3 ms, System: 46.2 ms]
  Range (min … max):   398.8 ms … 402.3 ms    10 runs

after:

  Time (mean ± σ):     415.5 ms ±   0.7 ms    [User: 362.4 ms, System: 52.8 ms]
  Range (min … max):   414.7 ms … 416.6 ms    10 runs

So merging loops has an opposite effect to what I had hoped. Thanks for putting the effort nonetheless!

[corona10]

For chromium 5735 with 1.2M symbols

Next time I will consider this binary for the benchmark.
Yeah it's slowing down sadly :)