Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44e4514
[BOLT][AArch64] Reduce the number of ADR relaxations
maksfb Sep 14, 2024
f317403
fixup! [BOLT][AArch64] Reduce the number of ADR relaxations
maksfb Oct 8, 2024
759f0d6
fixup! fixup! [BOLT][AArch64] Reduce the number of ADR relaxations
maksfb Oct 8, 2024
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
View file
Open in desktop
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

@dcci dcci Oct 8, 2024

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
31 changes: 31 additions & 0 deletions bolt/test/AArch64/adr-relaxation.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
## Check that llvm-bolt will unnecessarily relax ADR instruction.
Copy link
Member

@dcci dcci Oct 8, 2024

Choose a reason for hiding this comment

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

you mean "will not relax" ?

## ADR below references containing function that is split. But ADR is always
## in the main fragment, thus there is no need to relax it.

# 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=randomN \
# RUN: 2>&1 | FileCheck %s
# RUN: llvm-objdump -d --disassemble-symbols=_start %t.bolt | FileCheck %s

# CHECK-NOT: adrp
Copy link
Member

@dcci dcci Oct 8, 2024

Choose a reason for hiding this comment

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

can we dump the whole function (or some context?) that way we can make the test more robust.


.text
.globl _start
.type _start, %function
_start:
.cfi_startproc
adr x1, _start
Copy link
Member

@yota9 yota9 Oct 8, 2024

Choose a reason for hiding this comment

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

Indentation

cmp x1, x11
b.hi .L1

mov x0, #0x0

.L1:
ret x30

.cfi_endproc
.size _start, .-_start

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