[BOLT] Move ADRRelaxationPass

[yota9]

For non-simple functions we need nop instruction to be presented to
transform ADR to ADRP+ADD sequence, so run this pass before remove nops
pass.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Vladislav Khmelevsky (yota9)

Changes

For non-simple functions we need nop instruction to be presented to
transform ADR to ADRP+ADD sequence, so run this pass before remove nops
pass.

---

Full diff: https://github.com/llvm/llvm-project/pull/101371.diff

1 Files Affected:

• (modified) bolt/lib/Rewrite/BinaryPassManager.cpp (+2-2)

diff --git a/bolt/lib/Rewrite/BinaryPassManager.cpp b/bolt/lib/Rewrite/BinaryPassManager.cpp
index 5dfef0b71cc79..dadb1f4d833b0 100644
--- a/bolt/lib/Rewrite/BinaryPassManager.cpp
+++ b/bolt/lib/Rewrite/BinaryPassManager.cpp
@@ -357,6 +357,8 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
 
     Manager.registerPass(
         std::make_unique<VeneerElimination>(PrintVeneerElimination));
+
+    Manager.registerPass(std::make_unique<ADRRelaxationPass>());
   }
 
   if (BC.isRISCV()) {
@@ -490,8 +492,6 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
   Manager.registerPass(std::make_unique<ReorderData>());
 
   if (BC.isAArch64()) {
-    Manager.registerPass(std::make_unique<ADRRelaxationPass>());
-
     // Tighten branches according to offset differences between branch and
     // targets. No extra instructions after this pass, otherwise we may have
     // relocations out of range and crash during linking.

[peterwaller-arm]

LGTM. I also wonder aloud if RemoveNops could be moved down, I don't see documentation suggesting what it is constrained by or if any other pass depends on it.

The other incidental observation is that passes mix runOnFunctions() { if (!BC.isX86()) return; ... } and having if (arch) Manager.registerPass in BinaryPassManager. Is there a rationale for each? I'm new here but from my perspective it would be easier to follow what's going on if the passes relevant to each architecture were clear from runAllPasses(). I'm not asking for this to be changed in now but do you concur that arch conditions could be hoisted out of the individual passes to make it clearer? Or is the status quo preferred?

[yota9]

Well I think there is no documentation that would tell about pass order in BOLT. When creating new pass you need to consider yourself in which order it would be run. I've decided to run it on latter stage since there is no real difference where to put it and when remove ADR instructions. The pros to run it later is that it would also remove created adr instructions by bolt (if any). But we are not creating them and we don't have to create, any addresses on this stage should use adrp+add. And one of the cons found now is basically that nop instructions sometimes could be removed (also in my mind they really shouldn't due to #101964). (Originally this pass didn't even remove nops so I didn't even think about such problems). Anyway there is no harm on moving this up as I said BOLT should avoid creating ADR instructions by it self and it doesn't really matter when we would replace them, so let's replace from the beginning.

[yota9]

As for inconsistency - basically I agree. Currently I don't think there is some kind of code style here, this is of the minor thing to decide later with meta team

[maksfb]

As for inconsistency - basically I agree. Currently I don't think there is some kind of code style here, this is of the minor thing to decide later with meta team

I second that sentiment. Feel free to hoist arch checks to pass registry.