Skip to content

[LOH] Don't emit AdrpAddStr when register could be clobbered #142849

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
Jun 11, 2025
Merged

[LOH] Don't emit AdrpAddStr when register could be clobbered #142849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b18989a
[LOH] Don't emit str LOH when register could be clobbered
ellishg Jun 4, 2025
e05fb4d
Merge branch 'main' into loh-fix-adrp-add-str
ellishg Jun 11, 2025
f90de1e
format
ellishg Jun 11, 2025
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
37 changes: 22 additions & 15 deletions llvm/lib/Target/AArch64/AArch64CollectLOH.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,17 @@ static bool supportLoadFromLiteral(const MachineInstr &MI) {
}
}

/// Returns \p true if there are no non-debug instructions between \p First and
/// \p Second
static bool areInstructionsConsecutive(const MachineInstr *First,
const MachineInstr *Second) {
auto It = First->getIterator();
auto EndIt = First->getParent()->instr_end();
if (It == EndIt)
return false;
return next_nodbg(It, EndIt) == Second->getIterator();
}

/// Number of GPR registers tracked by mapRegToGPRIndex()
static const unsigned N_GPR_REGS = 31;
/// Map register number to index from 0-30.
Expand Down Expand Up @@ -415,7 +426,7 @@ static void handleADRP(const MachineInstr &MI, AArch64FunctionInfo &AFI,
++NumADRPToLDR;
}
break;
case MCLOH_AdrpAddLdr: {
case MCLOH_AdrpAddLdr:
// There is a possibility that the linker may try to rewrite:
// adrp x0, @sym@PAGE
// add x1, x0, @sym@PAGEOFF
Expand All @@ -432,28 +443,24 @@ static void handleADRP(const MachineInstr &MI, AArch64FunctionInfo &AFI,
// FIXME: Implement proper liveness tracking for all registers. For now,
// don't emit the LOH if there are any instructions between the add and
// the ldr.
MachineInstr *AddMI = const_cast<MachineInstr *>(Info.MI1);
const MachineInstr *LdrMI = Info.MI0;
auto AddIt = MachineBasicBlock::iterator(AddMI);
auto EndIt = AddMI->getParent()->end();
if (AddMI->getIterator() == EndIt || LdrMI != &*next_nodbg(AddIt, EndIt))
if (!areInstructionsConsecutive(Info.MI1, Info.MI0))
break;

LLVM_DEBUG(dbgs() << "Adding MCLOH_AdrpAddLdr:\n"
<< '\t' << MI << '\t' << *Info.MI1 << '\t'
<< *Info.MI0);
AFI.addLOHDirective(MCLOH_AdrpAddLdr, {&MI, Info.MI1, Info.MI0});
++NumADDToLDR;
break;
}
case MCLOH_AdrpAddStr:
if (Info.MI1 != nullptr) {
LLVM_DEBUG(dbgs() << "Adding MCLOH_AdrpAddStr:\n"
<< '\t' << MI << '\t' << *Info.MI1 << '\t'
<< *Info.MI0);
AFI.addLOHDirective(MCLOH_AdrpAddStr, {&MI, Info.MI1, Info.MI0});
++NumADDToSTR;
}
if (!Info.MI1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This check already exists. I'm curious why this check is not needed for AdrpAddLdr. Can we conservatively integrate a null-check for both MI0 and MI1 inside areInstructionsConsecutive(), return false if either one is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also confused about this. The reason we need it for AdrpAddStr and not AdrpAddLdr is because AdrpAddLdr starts out as a AdrpLdr and then is converted to a AdrpAddLdr by setting MI1. There is no AdrpStr, so MI1 is set to null until it finds the add instruction.

llvm-project/llvm/lib/Target/AArch64/AArch64CollectLOH.cpp

Lines 297 to 308 in 2c0a226

// Start new LOHInfo if applicable.
if (isCandidateLoad(MI)) {
Info.Type = MCLOH_AdrpLdr;
Info.IsCandidate = true;
Info.MI0 = &MI;
// Note that even this is AdrpLdr now, we can switch to a Ldr variant
// later.
} else if (isCandidateStore(MI, MO)) {
Info.Type = MCLOH_AdrpAddStr;
Info.IsCandidate = true;
Info.MI0 = &MI;
Info.MI1 = nullptr;

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the point! The code change looks good to me.

break;
if (!areInstructionsConsecutive(Info.MI1, Info.MI0))
break;
LLVM_DEBUG(dbgs() << "Adding MCLOH_AdrpAddStr:\n"
<< '\t' << MI << '\t' << *Info.MI1 << '\t'
<< *Info.MI0);
AFI.addLOHDirective(MCLOH_AdrpAddStr, {&MI, Info.MI1, Info.MI0});
++NumADDToSTR;
break;
case MCLOH_AdrpLdrGotLdr:
LLVM_DEBUG(dbgs() << "Adding MCLOH_AdrpLdrGotLdr:\n"
Expand Down
37 changes: 26 additions & 11 deletions llvm/test/CodeGen/AArch64/loh-adrp-add-ldr-clobber.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,34 @@
# RUN: llc -o /dev/null %s -mtriple=aarch64-apple-ios -run-pass=aarch64-collect-loh -debug-only=aarch64-collect-loh 2>&1 | FileCheck %s
# RUN: llc -o /dev/null %s -mtriple=aarch64-apple-ios -run-pass=aarch64-collect-loh -debug-only=aarch64-collect-loh 2>&1 | FileCheck %s --implicit-check-not=MCLOH_
# REQUIRES: asserts

# Check that we don't emit LOHs when there is a clobbering def of x8.
--- |
@sym2 = local_unnamed_addr global [10000000 x i32] zeroinitializer, align 8
@sym = local_unnamed_addr global i32 zeroinitializer, align 8

define i32 @main() {
ret i32 0
}
define i32 @adrp_add_ldr() { ret i32 0 }
define i32 @adrp_add_str() { ret i32 0 }
...

---
name: adrp_add_ldr
alignment: 4
tracksRegLiveness: true
liveins:
- { reg: '$x21', virtual-reg: '' }
body: |
bb.0:
liveins: $x21
renamable $x8 = ADRP target-flags(aarch64-page) @sym
renamable $x9 = ADDXri killed renamable $x8, target-flags(aarch64-pageoff, aarch64-nc) @sym, 0
renamable $x8 = ADDXri killed renamable $x21, 1, 0
$x9 = LDRXui $x9, 0

RET undef $lr
...

---
name: main
name: adrp_add_str
alignment: 4
tracksRegLiveness: true
liveins:
Expand All @@ -19,13 +37,10 @@ liveins:
body: |
bb.0:
liveins: $x21, $x22
; Check we don't emit an loh here because there's a clobbering def of x8 before the ldr.
; CHECK-LABEL: main
; CHECK-NOT: MCLOH_AdrpAddLdr
renamable $x8 = ADRP target-flags(aarch64-page) @sym
renamable $x9 = ADDXri killed renamable $x8, target-flags(aarch64-pageoff, aarch64-nc) @sym, 0
renamable $x8 = ADDXri killed renamable $x22, 1, 0
$x9 = LDRXui $x9, 0
RET undef $lr
renamable $x8 = ADDXri killed renamable $x21, 1, 0
STRXui $x22, $x9, 0

RET undef $lr
...
Loading