[PATCH] D117612: [BOLT] Update dynamic relocations from section relocations
Vladislav Khmelevsky via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 14 08:00:16 PST 2022
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
More information about the llvm-commits
mailing list