[PATCH] D117612: [BOLT] Update dynamic relocations from section relocations

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 added inline comments.


================
Comment at: bolt/include/bolt/Rewrite/RewriteInstance.h:442
+
+  /// Index of specified symbol in symbol table
+  std::map<MCSymbol *, uint32_t> SymbolIndex;
----------------
maksfb wrote:
> Could you put a more accurate description? I.e. "index in the dynamic symbol table". Also, not all symbols will be in the table, right? Only those referenced by relocations?
Done. Yes, currently only relocation-related symbols will appear there. To be honest I just didn't find the way to get all the symbols in table order, anyway currently it is not needed, I will add a NOTE about this.


================
Comment at: bolt/include/bolt/Rewrite/RewriteInstance.h:443
+  /// Index of specified symbol in symbol table
+  std::map<MCSymbol *, uint32_t> SymbolIndex;
+
----------------
maksfb wrote:
> Perhaps `DenseMap` here as well?
In my mind unordered_map is good choice here, I've forgot to fix it before upload. AFAIK DenseMap keeps the data in one allocation, I don't really think it is needed here, there might be quite a lot of symbols, so I feel like many small allocations is more winning here. Also the fata locality won't really change the overall performance. But I don't really insist, if you'd like I will replace it with  DenseMap


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2107
+    const uint32_t SymbolIdx = getRelocationSymbol(InputFile, Rel);
+    const uint64_t Type = Rel.getType();
 
----------------
maksfb wrote:
> nit: use `RType` defined above. You can make it `const`.
My bad, thanks


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4798-4800
+          auto It = SymbolIndex.find(Symbol);
+          if (It != SymbolIndex.end())
+            SymbolIdx = It->second;
----------------
maksfb wrote:
> I'd rather put the code under `getOutputDynamicSymbolIndex(MCSymbol *)`.
I was thinking about this too, but rewriteinstance lacks of such wrappers so I've decided to add it in place. But I don't mind, so done :) 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117612/new/

https://reviews.llvm.org/D117612