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

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added a comment.

@yota9, sorry for the delay. Rafael is on vacation. I took a quick peek at aarch64 lld, and it handles more relaxations than just ADRP/LDR, e.g. TLS-specific sequences. Do you plan to support those in the future?

In general, the pass looks fine. Ideally, we should fix such discrepancies as early in the pipeline as possible, i.e. during disassembly so that --print-disasm output looks correct. But this clearly requires scanning forward, which complicates things. Still, you can check if the next relocation doesn't match the instruction when you see a relaxable ADRP candidate. What do you think?

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.

Please run spellcheck on the summary.



================
Comment at: bolt/lib/Passes/FixRelaxationPass.cpp:59
+      BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun, nullptr,
+      "FixRelaxations", true);
+}
----------------
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.


================
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);
----------------
Can we use `__BOLT_` prefix for the internally-created symbol?


================
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
----------------
Does it make sense to generate a test case with lld similar to x86 GOTPCRELX test in D126747?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138097