[MachineOutliner] Preserve instruction bundles #106402
Merged
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
When the machine outliner copies instructions from a source function into an outlined function, it was doing it using `CloneMachineInstr`, which is documented as not preserving the interior of any instruction bundle. So outlining code that includes an instruction bundle would fail, because in the outlined version, the bundle would be empty, so instructions would go missing in the move. This occurs when any bundled instruction appears in the outlined code, so there was no need to construct an unusual test case: I've just copied a function from the existing `stp-opt-with-renaming.mir`, which happens to contain an SVE instruction bundle. Including two identical copies of that function makes the outliner merge them, and then we check that it didn't destroy the interior of the bundle in the process.
|
@llvm/pr-subscribers-backend-aarch64 Author: Simon Tatham (statham-arm) ChangesWhen the machine outliner copies instructions from a source function into an outlined function, it was doing it using This occurs when any bundled instruction appears in the outlined code, so there was no need to construct an unusual test case: I've just copied a function from the existing Full diff: https://github.com/llvm/llvm-project/pull/106402.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineOutliner.cpp b/llvm/lib/CodeGen/MachineOutliner.cpp
index 42f410c277179b..9678534cffce09 100644
--- a/llvm/lib/CodeGen/MachineOutliner.cpp
+++ b/llvm/lib/CodeGen/MachineOutliner.cpp
@@ -763,10 +763,9 @@ MachineFunction *MachineOutliner::createOutlinedFunction(
BuildMI(MBB, MBB.end(), DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(MF.addFrameInst(CFI));
} else {
- MachineInstr *NewMI = MF.CloneMachineInstr(&MI);
- NewMI->dropMemRefs(MF);
- NewMI->setDebugLoc(DL);
- MBB.insert(MBB.end(), NewMI);
+ MachineInstr &NewMI = MF.cloneMachineInstrBundle(MBB, MBB.end(), MI);
+ NewMI.dropMemRefs(MF);
+ NewMI.setDebugLoc(DL);
}
}
diff --git a/llvm/test/CodeGen/AArch64/machine-outliner-bundle.mir b/llvm/test/CodeGen/AArch64/machine-outliner-bundle.mir
new file mode 100644
index 00000000000000..1dd5b0811bdfb6
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/machine-outliner-bundle.mir
@@ -0,0 +1,54 @@
+# RUN: llc -mtriple=aarch64 -run-pass=machine-outliner \
+# RUN: -verify-machineinstrs %s -o - | FileCheck %s
+
+# CHECK: name: OUTLINED_FUNCTION_0
+# CHECK-NOT: name:
+# CHECK: BUNDLE implicit-def $z3, implicit-def $q3, implicit-def $d3, implicit-def $s3, implicit-def $h3, implicit-def $b3, implicit $z19, implicit $p0, implicit $z16 {
+# CHECK: $z3 = MOVPRFX_ZZ $z19
+# CHECK: $z3 = FMUL_ZPmZ_S renamable $p0, killed $z3, renamable $z16
+# CHECK: }
+
+---
+name: bundled
+alignment: 4
+tracksRegLiveness: true
+frameInfo:
+ maxAlignment: 1
+ maxCallFrameSize: 0
+machineFunctionInfo:
+ hasRedZone: false
+body: |
+ bb.0.entry:
+ liveins: $z3, $z19, $p0, $z16
+ renamable $q0 = LDRQui $sp, 1 :: (load 16)
+ STRSui renamable $s0, $sp, 9, implicit killed $q0 :: (store (s32))
+ BUNDLE implicit-def $z3, implicit-def $q3, implicit-def $d3, implicit-def $s3, implicit-def $h3, implicit-def $b3, implicit $z19, implicit $p0, implicit $z16 {
+ $z3 = MOVPRFX_ZZ $z19
+ $z3 = FMUL_ZPmZ_S renamable $p0, killed $z3, renamable $z16
+ }
+ renamable $q0 = LDRQui $sp, 0 :: (load 16, align 32)
+ STRSui renamable $s0, $sp, 10, implicit killed $q0 :: (store (s32))
+ RET undef $lr
+...
+---
+name: bundled_clone
+alignment: 4
+tracksRegLiveness: true
+frameInfo:
+ maxAlignment: 1
+ maxCallFrameSize: 0
+machineFunctionInfo:
+ hasRedZone: false
+body: |
+ bb.0.entry:
+ liveins: $z3, $z19, $p0, $z16
+ renamable $q0 = LDRQui $sp, 1 :: (load 16)
+ STRSui renamable $s0, $sp, 9, implicit killed $q0 :: (store (s32))
+ BUNDLE implicit-def $z3, implicit-def $q3, implicit-def $d3, implicit-def $s3, implicit-def $h3, implicit-def $b3, implicit $z19, implicit $p0, implicit $z16 {
+ $z3 = MOVPRFX_ZZ $z19
+ $z3 = FMUL_ZPmZ_S renamable $p0, killed $z3, renamable $z16
+ }
+ renamable $q0 = LDRQui $sp, 0 :: (load 16, align 32)
+ STRSui renamable $s0, $sp, 10, implicit killed $q0 :: (store (s32))
+ RET undef $lr
+...
|
|
This seems to fix the immediate problem we noticed, but while searching for the API function that would fix it, I wondered if |
|
Investigating further, it looks as if the difference is that @ornata or @yroux , Oliver suggests you might know better than me about that, so please correct me if I've got that wrong? |
ostannard
approved these changes
Sep 3, 2024
There was a problem hiding this comment.
Even if we don't outline constant pool references at the moment, I think TargetInstrInfo::duplicate is the safer choice, in case either we do start outlining them, or ARMBaseInstrInfo::duplicate need in the future to handle something we do outline.
LGTM assuming you make that change.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/5404 Here is the relevant piece of the build log for the reference |
|
The next buildbot run was happy again, so that looks like a transient glitch. |
Sign up for free
to 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.
When the machine outliner copies instructions from a source function into an outlined function, it was doing it using
CloneMachineInstr, which is documented as not preserving the interior of any instruction bundle. So outlining code that includes an instruction bundle would fail, because in the outlined version, the bundle would be empty, so instructions would go missing in the move.This occurs when any bundled instruction appears in the outlined code, so there was no need to construct an unusual test case: I've just copied a function from the existing
stp-opt-with-renaming.mir, which happens to contain an SVE instruction bundle. Including two identical copies of that function makes the outliner merge them, and then we check that it didn't destroy the interior of the bundle in the process.