[BOLT][AArch64] Fix LDR relocation type in ADRP+LDR sequence

[yozhu]

R_AARCH64_ADD_ABS_LO12_NC is for the ADD instruction in the ADRP+ADD sequence. For ADRP+LDR sequence generated in LDR relaxation, relocation type for LDR should be R_AARCH64_LDST64_ABS_LO12_NC if it is 64-bit integer load or R_AARCH64_LDST32_ABS_LO12_NC if 32-bit.

Sorry should have included this in #165787.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: YongKang Zhu (yozhu)

Changes

R_AARCH64_ADD_ABS_LO12_NC is for the ADD instruction in the ADRP+ADD sequence. For ADRP+LDR sequence generated in LDR relaxation, relocation type for LDR should be R_AARCH64_LDST64_ABS_LO12_NC if it is 64-bit integer load or R_AARCH64_LDST32_ABS_LO12_NC if 32-bit.

Sorry should have included this in #165787.

---

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

1 Files Affected:

• (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+2-1)

diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 8a496c566b06b..43c9d9d3f14bd 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -640,7 +640,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     Insts[1].addOperand(MCOperand::createImm(0));
     Insts[1].addOperand(MCOperand::createImm(0));
     setOperandToSymbolRef(Insts[1], /* OpNum */ 2, Target, 0, Ctx,
-                          ELF::R_AARCH64_ADD_ABS_LO12_NC);
+                          isLDRXl(LDRInst) ? ELF::R_AARCH64_LDST64_ABS_LO12_NC
+                                           : ELF::R_AARCH64_LDST32_ABS_LO12_NC);
     return Insts;
   }
 

[paschalis-mpeis]

Thanks for the update. I believe it wouldn't matter which of the 3 RelTypes we pass, as we'd get the LO12 bits expression regardless.
But ofc, it's better to be accurate. You haven't observed any issues, right?

Some thoughts from the previous patch that might have helped:

• Refactoring to reuse logic from materializeAddress could help
• We could also make naming consistent between createAdrpLdr and undoAdrpAddRelaxation?

[yozhu]

Thanks for the update. I believe it wouldn't matter which of the 3 RelTypes we pass, as we'd get the LO12 bits expression regardless. But ofc, it's better to be accurate. You haven't observed any issues, right?

Thanks for the review and suggestions!

Right, I haven't observed any issue, before or after this change. As you said, it is just to make the relocation type name accurately reflect what kind of instruction the relocation is applied on.

Some thoughts from the previous patch that might have helped:

• Refactoring to reuse logic from materializeAddress could help

I had thought about materializeAddress for reuse of some of the logic (especially on creating the ADRP instruction), and later decided to just create a new function because the generated ADRP+LDR sequence is to load content from memory location and conceptually is not to materialize memory address only.

• We could also make naming consistent between createAdrpLdr and undoAdrpAddRelaxation?

I had thought about this when working on the previous patch. undoAdrpAddRelaxation seems to refer to that BOLT will undo the relaxation done by the linker, but LDR (load literal) is not resulted from of any relaxation done by the linker, so I just named it createAdrpLdr.

[paschalis-mpeis]

I had thought about this when working on the previous patch. undoAdrpAddRelaxation seems to refer to that BOLT will undo the relaxation done by the linker, but LDR (load literal) is not resulted from of any relaxation done by the linker, so I just named it createAdrpLdr.

I was not sure of the context when that function was introduced.
That's fair. Thanks again for the patch!

[yozhu]

Thanks again for the review and suggestions!