- Notifications
You must be signed in to change notification settings - Fork14.5k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
@llvm/pr-subscribers-backend-aarch64 Author: Ellis Hoag (ellishg) ChangesWe found some code that was hitting this assert because llvm-project/llvm/include/llvm/CodeGen/MachineInstrBundleIterator.h Lines 133 to 135 inae3bba4
Avoid creating those ranges and add a test that hit the assert. Full diff:https://github.com/llvm/llvm-project/pull/148977.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cppindex 5420545cc3cec..a23ed60d0325b 100644--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp@@ -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.push_back(std::make_pair(RangeBegin, RangeEnd)); }; // Find the first point where all unsafe registers are dead. // FIND: <safe instr> <-- end of first potential rangediff --git a/llvm/test/CodeGen/AArch64/machine-outliner-safe-range-in-middle.mir b/llvm/test/CodeGen/AArch64/machine-outliner-safe-range-in-middle.mirindex 23811425101fd..ed4a8fe76c221 100644--- a/llvm/test/CodeGen/AArch64/machine-outliner-safe-range-in-middle.mir+++ b/llvm/test/CodeGen/AArch64/machine-outliner-safe-range-in-middle.mir@@ -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+ ; 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+ |
kyulee-com approved these changesJul 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the fix! It looks good to me.
Uh oh!
There was an error while loading.Please reload this page.
| body:| | ||
| bb.0: | ||
| liveins: $x0 | ||
| ; Begin safe range of 3 instructions |
There was a problem hiding this comment.
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 instructionsisn't safe to outline :-)
There was a problem hiding this comment.
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 testunsafe_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.
There was a problem hiding this comment.
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.
bd6c16c intollvm:main 7 of 9 checks passed
Uh oh!
There was an error while loading.Please reload this page.
LLVM Buildbot has detected a new failure on builder Full details are available at:https://lab.llvm.org/buildbot/#/builders/54/builds/10937 Here is the relevant piece of the build log for the reference |
LLVM Buildbot has detected a new failure on builder Full details are available at:https://lab.llvm.org/buildbot/#/builders/123/builds/23515 Here is the relevant piece of the build log for the reference |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We found some code that was hitting this assert because
getOutlinableRanges()was trying to create a range that crossed a bundle boundary.llvm-project/llvm/include/llvm/CodeGen/MachineInstrBundleIterator.h
Lines 133 to 135 inae3bba4
Avoid creating those ranges and add a test that hit the assert.