[PATCH] D154604: [BOLT] Calculate output values using BOLTLinker

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: bolt/lib/Core/BinaryContext.cpp:192
+  // the linker will never see them.
+  Ctx->setAllowTemporaryLabels(false);
+
----------------
maksfb wrote:
> Background: we've discussed offline that this change causes a noticeable BOLT runtime regression.
> 
> The alternative approach for mapping input to output addresses used for address translation, will be to generate a map in a section that we can read and discard later (i.e. skip writing to a file). The map/section contents will be an ordered list of pairs `<InputAddress, OutputAddress>`. For output address we will emit the temporary symbol to the section. I believe `BOLTLinker` should be able to process it.
> 
> With the approach above, we can continue to emit all internal symbols as temps and avoid the regression.
> The alternative approach for mapping input to output addresses used for address translation, will be to generate a map in a section that we can read and discard later (i.e. skip writing to a file). The map/section contents will be an ordered list of pairs `<InputAddress, OutputAddress>`. For output address we will emit the temporary symbol to the section. I believe `BOLTLinker` should be able to process it.

Interesting idea! I made a PoC implementation and have the following preliminary performance results:
- With `--enable-bat --update-debug-sections`: +4% total runtime
- Without: negligible overhead.

So the overhead is much smaller than for this patch (11%), but it's still non-zero. Would you still agree that the preferred approach (as discussed during our call) is to use this patch but only emit the temp symbols for the targets that need it (i.e., RISC-V)?

One thing I noticed is that the format you proposed for the map section is quite similar to the BAT section BOLT emits with `--enable-bat`. I wonder if it could be worth trying to emit the BAT section //directly// instead of the intermediary map you proposed. That way, we might get rid of having to manually update BB output values. However, I guess it would mean that we'd have to generate the BAT section in some cases even without `--enable-bat` (e.g., to update debug info).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154604