Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BOLT][AArch64] Reduce the number of ADR relaxations #111577

Merged
merged 3 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions bolt/lib/Passes/ADRRelaxationPass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,14 @@ void ADRRelaxationPass::runOnFunction(BinaryFunction &BF) {
continue;
}

// Don't relax adr if it points to the same function and it is not split
// and BF initial size is < 1MB.
// Don't relax ADR if it points to the same function and is in the main
// fragment and BF initial size is < 1MB.
const unsigned OneMB = 0x100000;
if (BF.getSize() < OneMB) {
BinaryFunction *TargetBF = BC.getFunctionForSymbol(Symbol);
if (TargetBF == &BF && !BF.isSplit())
if (TargetBF == &BF && !BB.isSplit())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. Can we add a negative test? (e.g. one where the func is not in the main fragment to show the adrp is still there?)

continue;

// No relaxation needed if ADR references a basic block in the same
// fragment.
if (BinaryBasicBlock *TargetBB = BF.getBasicBlockForLabel(Symbol))
Expand Down
49 changes: 49 additions & 0 deletions bolt/test/AArch64/adr-relaxation.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
## Check that llvm-bolt will not unnecessarily relax ADR instruction.

# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static
# RUN: llvm-bolt %t.exe -o %t.bolt --split-functions --split-strategy=random2
# RUN: llvm-objdump -d --disassemble-symbols=_start %t.bolt | FileCheck %s
# RUN: llvm-objdump -d --disassemble-symbols=foo.cold.0 %t.bolt \
# RUN: | FileCheck --check-prefix=CHECK-FOO %s

## ADR below references its containing function that is split. But ADR is always
## in the main fragment, thus there is no need to relax it.
.text
.globl _start
.type _start, %function
_start:
# CHECK: <_start>:
.cfi_startproc
adr x1, _start
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

# CHECK-NOT: adrp
# CHECK: adr
cmp x1, x11
b.hi .L1
mov x0, #0x0
.L1:
ret x30
.cfi_endproc
.size _start, .-_start


## In foo, ADR is in the split fragment but references the main one. Thus, it
## needs to be relaxed into ADRP + ADD.
.globl foo
.type foo, %function
foo:
.cfi_startproc
cmp x1, x11
b.hi .L2
mov x0, #0x0
.L2:
# CHECK-FOO: <foo.cold.0>:
adr x1, foo
# CHECK-FOO: adrp
# CHECK-FOO-NEXT: add
ret x30
.cfi_endproc
.size foo, .-foo

## Force relocation mode.
.reloc 0, R_AARCH64_NONE
Loading