[PATCH] D138097: [BOLT][AArch64] Handle adrp+ld64 linker relaxations

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 marked 3 inline comments as done.
yota9 added a comment.

Thank you for review @maksfb and sorry for long response too. 
We are now supporting most of them I think, currently every relaxation is handled in skipRelocationProcessAArch64. As for pure TLS one we don't have to support them since we does not support TLS relocations and don't handle TLS sections in BOLT for now.

>> Ideally, we should fix such discrepancies as early in the pipeline as possible

I'm absolutely agree with this, but currently I don't see how to do it at relocation-handling stage. We need to handle relocations, get instruction opcode & etc, I expect the code to be very messy if we would do that. Although I don't really like the idea of the separate pass too, but probably it is the only way currently :(

>> To speed things up with the current approach, you can detect functions with relaxable relocations while reading relocs, mark functions that need fixes, and only run the pass on them.

It is true, but I don't really want to introduce new BF variables here.. Ideally we might make some bit mask values for such purposes, but for now I think it is ok to use it as-is.



================
Comment at: bolt/lib/Passes/FixRelaxationPass.cpp:59
+      BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun, nullptr,
+      "FixRelaxations", true);
+}
----------------
maksfb wrote:
> yota9 wrote:
> > @rafaelauler To be honest I'm a bit confused, since sometimes I had a problems in the past within parallel execution of other passes. But it seems to be safe here, is it? I should probably make it parallel
> Since you are not modifying `MCContext`, it should be thread-safe. You can run "valgrind --tool=helgrind ..." or thread sanitizer for extra validation.
It seems to be no problem to run this pass in parallel :)


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2558
   if (ForceRelocation) {
-    std::string Name = Relocation::isGOT(RType) ? "Zero" : SymbolName;
+    std::string Name = Relocation::isGOT(RType) ? "Zero4Got" : SymbolName;
     ReferencedSymbol = BC->registerNameAtAddress(Name, 0, 0, 0);
----------------
maksfb wrote:
> Can we use `__BOLT_` prefix for the internally-created symbol?
Sure :)


================
Comment at: bolt/test/AArch64/got-ld64-relaxation.test:1
+// This test checks that ADR+LDR instruction sequence relaxed by the linker
+// to the ADR+ADD sequence is properly reconized and handled by bolt
----------------
maksfb wrote:
> Does it make sense to generate a test case with lld similar to x86 GOTPCRELX test in D126747?
The thing is that the linker will relax it to the adr + nop due to small size of the final binary. Such a case is already handled in skipRelocationProcessAArch64, but for adrp+add case it is better to use pre-built binary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138097