Skip to content

[MachineOutliner] Avoid ranges that cross bundle boundary #148977

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
Jul 16, 2025
Merged

[MachineOutliner] Avoid ranges that cross bundle boundary #148977

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ade8cb
[MachineOutliner] Avoid ranges that cross bundle boundary
ellishg Jul 15, 2025
543cde8
be fancy
ellishg Jul 15, 2025
fcac631
Fix test and add extra check
ellishg Jul 16, 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
11 changes: 7 additions & 4 deletions llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9585,10 +9585,13 @@ AArch64InstrInfo::getOutlinableRanges(MachineBasicBlock &MBB,
};
auto SaveRangeIfNonEmpty = [&RangeLen, &Ranges, &RangeBegin, &RangeEnd]() {
// At least one unsafe register is not dead. We do not want to outline at
// this point. If it is long enough to outline from, save the range
// [RangeBegin, RangeEnd).
if (RangeLen > 1)
Ranges.push_back(std::make_pair(RangeBegin, RangeEnd));
// this point. If it is long enough to outline from and does not cross a
// bundle boundary, save the range [RangeBegin, RangeEnd).
if (RangeLen <= 1)
return;
if (RangeBegin->isBundledWithPred())
return;
Ranges.emplace_back(RangeBegin, RangeEnd);
};
// Find the first point where all unsafe registers are dead.
// FIND: <safe instr> <-- end of first potential range
Expand Down
49 changes: 49 additions & 0 deletions llvm/test/CodeGen/AArch64/machine-outliner-safe-range-in-middle.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,52 @@ body: |
$x9 = ADDXri $x9, 16, 0
$x16 = ADDXri killed $x16, 16, 0
RET undef $x9
...
---
name: unsafe_range_bundle
tracksRegLiveness: true
machineFunctionInfo:
hasRedZone: false
body: |
bb.0:
liveins: $x0
; Begin safe range of 3 instructions
Copy link
Collaborator

@statham-arm statham-arm Jul 16, 2025

Choose a reason for hiding this comment

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

I'm confused by the "safe range" comments. This "Begin safe range of 3 instructions" comment seems to go with an "End safe range" comment on line 74 (in this version of the patch) four instructions later, not three. Then there's another "End safe range" further down (line 86) without any "Begin" comment. Finally, it's confusing that the CHECK comments are after the "Begin safe range"; putting them the other way round would surely have been clearer?

It looks to me as if the real point of this test is that the four ADDXri to x0,x1,x2,x3 are duplicated in this function (lines 69-72 and 81-85), but in the second copy two of them are inside a bundle, and therefore they mustn't be outlined. If I've understood that correctly, perhaps describing them as something like an "outlining candidate" instead of a safe range might be less confusing – especially since the whole point of this example is that this range of instructions isn't safe to outline :-)

Copy link
Contributor Author

@ellishg ellishg Jul 16, 2025

Choose a reason for hiding this comment

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

These comments were copied from the test unsafe_range_in_middle above, and I didn't check if they were correct or not. I simply found some tests that had separate ranges and inserted a bundle that triggered the crash.

Copy link
Contributor Author

@ellishg ellishg Jul 16, 2025

Choose a reason for hiding this comment

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

I ended up updating the comments to reflect the actual ranges and simplified my test a bit. I also found that we need some extra checks, too.

; CHECK-LABEL: name: unsafe_range_bundle
; CHECK: liveins: $x0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $x0, implicit-def $x1, implicit-def $x2, implicit-def $x3, implicit-def $x16, implicit $x0, implicit $sp
; CHECK-NEXT: $x9 = ADDXri $x16, 16, 0
; CHECK-NEXT: BUNDLE {
; CHECK-NEXT: $x16 = ADDXri killed $x16, 16, 0
; CHECK-NEXT: $x0 = ADDXri $x0, 0, 0
; CHECK-NEXT: $x1 = ADDXri $x0, 1, 0
; CHECK-NEXT: }
; CHECK-NEXT: $x2 = ADDXri $x0, 2, 0
; CHECK-NEXT: $x3 = ADDXri $x0, 3, 0
; CHECK-NEXT: $x16 = ADDXri $x0, 16, 0
; CHECK-NEXT: $x9 = ADDXri $x9, 16, 0
; CHECK-NEXT: $x16 = ADDXri killed $x16, 16, 0
; CHECK-NEXT: RET undef $x9
$x0 = ADDXri $x0, 0, 0
$x1 = ADDXri $x0, 1, 0
$x2 = ADDXri $x0, 2, 0
$x3 = ADDXri $x0, 3, 0

; End safe range
$x16 = ADDXri $x0, 16, 0
$x9 = ADDXri $x16, 16, 0
; Bundle crosses a safe range
BUNDLE {
$x16 = ADDXri killed $x16, 16, 0

$x0 = ADDXri $x0, 0, 0
$x1 = ADDXri $x0, 1, 0
}
$x2 = ADDXri $x0, 2, 0
$x3 = ADDXri $x0, 3, 0
; End safe range
$x16 = ADDXri $x0, 16, 0
$x9 = ADDXri $x9, 16, 0
$x16 = ADDXri killed $x16, 16, 0
RET undef $x9

Loading