RegisterCoalescer: Fix producing malformed IMPLICIT_DEFs #73784
Merged
Conversation
This file contains 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 @llvm/pr-subscribers-llvm-regalloc Author: Matt Arsenault (arsenm) ChangesIf this was coalescing a SUBREG_TO_REG as a copy, the resulting instruction would be an IMPLICIT_DEF with an unexpected 2 immediate operands, which need to be dropped. Until recently the verifier did not catch this error, and an assert would fire if later the broken IMPLICIT_DEF was rematerialized. changes the failure mode to a verifier error. Full diff: https://github.com/llvm/llvm-project/pull/73784.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index a55f3941c98b3bd..148808234a51636 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -1694,12 +1694,19 @@ MachineInstr *RegisterCoalescer::eliminateUndefCopy(MachineInstr *CopyMI) {
// The source interval may also have been on an undef use, in which case the
// copy introduced a live value.
if (((V && V->isPHIDef()) || (!V && !DstLI.liveAt(Idx)))) {
- CopyMI->setDesc(TII->get(TargetOpcode::IMPLICIT_DEF));
for (unsigned i = CopyMI->getNumOperands(); i != 0; --i) {
MachineOperand &MO = CopyMI->getOperand(i-1);
- if (MO.isReg() && MO.isUse())
+ if (MO.isReg()) {
+ if (MO.isUse())
+ CopyMI->removeOperand(i - 1);
+ } else {
+ assert(MO.isImm() &
+ (CopyMI->getOpcode() == TargetOpcode::SUBREG_TO_REG));
CopyMI->removeOperand(i-1);
+ }
}
+
+ CopyMI->setDesc(TII->get(TargetOpcode::IMPLICIT_DEF));
LLVM_DEBUG(dbgs() << "\tReplaced copy of <undef> value with an "
"implicit def\n");
return CopyMI;
diff --git a/llvm/test/CodeGen/AArch64/coalescer-drop-subreg-to-reg-imm-ops.mir b/llvm/test/CodeGen/AArch64/coalescer-drop-subreg-to-reg-imm-ops.mir
new file mode 100644
index 000000000000000..f54c612303c0eb3
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/coalescer-drop-subreg-to-reg-imm-ops.mir
@@ -0,0 +1,120 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
+# RUN: llc -mtriple=arm64-apple-macosx -mcpu=apple-m1 -verify-coalescing -run-pass=register-coalescer -o - %s | FileCheck %s
+
+# Hits assert "Trying to add an operand to a machine instr that is
+# already done!" when rematerializing during greedy. This was because
+# an IMPLICIT_DEF ended up with some immediate operands during
+# coalescing. A SUBREG_TO_REG was not dropping the immediate operands
+# when mutating to IMPLICIT_DEF, and would later fail the assert when
+# creating a new IMPLICIT_DEF copy during rematerialization.
+
+--- |
+ define void @_ZN38SanitizerCommonInterceptors_Scanf_Test8TestBodyEv() {
+ ret void
+ }
+
+ declare void @_ZL9testScanfPKcjz(ptr, i32, ...)
+
+...
+---
+name: _ZN38SanitizerCommonInterceptors_Scanf_Test8TestBodyEv
+alignment: 4
+tracksRegLiveness: true
+frameInfo:
+ maxAlignment: 8
+ adjustsStack: true
+ hasCalls: true
+ maxCallFrameSize: 24
+body: |
+ bb.0:
+ liveins: $x0, $x1, $x2, $x3, $x4, $x5, $x6
+
+ ; CHECK-LABEL: name: _ZN38SanitizerCommonInterceptors_Scanf_Test8TestBodyEv
+ ; CHECK: liveins: $x0, $x1, $x2, $x3, $x4, $x5, $x6
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[DEF:%[0-9]+]]:gpr64sp = IMPLICIT_DEF
+ ; CHECK-NEXT: dead [[DEF1:%[0-9]+]]:gpr32 = IMPLICIT_DEF
+ ; CHECK-NEXT: [[DEF2:%[0-9]+]]:gpr64common = IMPLICIT_DEF
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr64 = COPY $x5
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr64 = COPY $x4
+ ; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr64 = COPY $x3
+ ; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr64 = COPY $x2
+ ; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr64 = COPY $x1
+ ; CHECK-NEXT: [[COPY5:%[0-9]+]]:gpr64 = COPY $x0
+ ; CHECK-NEXT: [[DEF3:%[0-9]+]]:gpr64 = IMPLICIT_DEF
+ ; CHECK-NEXT: [[DEF4:%[0-9]+]]:gpr64 = IMPLICIT_DEF
+ ; CHECK-NEXT: [[DEF5:%[0-9]+]]:gpr64 = IMPLICIT_DEF
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 16, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: BL @_ZL9testScanfPKcjz, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ ; CHECK-NEXT: ADJCALLSTACKUP 16, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: STRXui [[DEF3]], [[DEF]], 0 :: (store (s64) into stack)
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: STRWui undef [[DEF1]], [[DEF2]], 0 :: (store (s32))
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: STRXui [[DEF4]], undef [[DEF]], 0 :: (store (s64) into stack)
+ ; CHECK-NEXT: $x0 = COPY [[COPY5]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: $x0 = COPY [[COPY4]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: $x0 = COPY [[COPY3]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: $x0 = COPY [[COPY2]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: $x0 = COPY [[COPY1]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: $x0 = COPY [[COPY]]
+ ; CHECK-NEXT: ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 24, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: STRXui [[DEF5]], undef [[DEF]], 1 :: (store (s64) into stack + 8)
+ ; CHECK-NEXT: ADJCALLSTACKUP 24, 0, implicit-def dead $sp, implicit $sp
+ ; CHECK-NEXT: RET_ReallyLR
+ %0:gpr64sp = IMPLICIT_DEF
+ %1:gpr32 = IMPLICIT_DEF
+ %2:gpr64common = IMPLICIT_DEF
+ %3:gpr64 = COPY killed $x5
+ %4:gpr64 = COPY killed $x4
+ %5:gpr64 = COPY killed $x3
+ %6:gpr64 = COPY killed $x2
+ %7:gpr64 = COPY killed $x1
+ %8:gpr64 = COPY killed $x0
+ %9:gpr64 = IMPLICIT_DEF
+ %10:gpr64 = IMPLICIT_DEF
+ %11:gpr64 = SUBREG_TO_REG 0, killed undef %1, %subreg.sub_32
+ ADJCALLSTACKDOWN 16, 0, implicit-def dead $sp, implicit $sp
+ BL @_ZL9testScanfPKcjz, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ ADJCALLSTACKUP 16, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ STRXui %9, killed %0, 0 :: (store (s64) into stack)
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ STRWui undef %1, killed %2, 0 :: (store (s32))
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ STRXui killed %10, killed undef %0, 0 :: (store (s64) into stack)
+ $x0 = COPY killed %8
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ $x0 = COPY killed %7
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ $x0 = COPY killed %6
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ $x0 = COPY killed %5
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ $x0 = COPY killed %4
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 8, 0, implicit-def dead $sp, implicit $sp
+ $x0 = COPY killed %3
+ ADJCALLSTACKUP 8, 0, implicit-def dead $sp, implicit $sp
+ ADJCALLSTACKDOWN 24, 0, implicit-def dead $sp, implicit $sp
+ STRXui killed %11, undef %0, 1 :: (store (s64) into stack + 8)
+ ADJCALLSTACKUP 24, 0, implicit-def dead $sp, implicit $sp
+ RET_ReallyLR
+
+...
|
If this was coalescing a SUBREG_TO_REG as a copy, the resulting instruction would be an IMPLICIT_DEF with an unexpected 2 immediate operands, which need to be dropped. Until recently the verifier did not catch this error, and an assert would fire if later the broken IMPLICIT_DEF was rematerialized. PR#73758 is related, it changes the failure mode to a verifier error.
arsenm
force-pushed
the
coalescer-broken-implicit-def-subreg-to-reg
branch
from
8894e56
to
70b843a
Compare
kparzysz
approved these changes
Dec 1, 2023
Comment on lines
1703
to
1704
| assert(MO.isImm() && | ||
| (CopyMI->getOpcode() == TargetOpcode::SUBREG_TO_REG)); |
There was a problem hiding this comment.
Do we need this assertion? It seems a bit out of place, since nothing else in this function mentions SUBREG_TO_REG.
There was a problem hiding this comment.
Everything else suggests this function only handles copies, except for the exception it handles "copy-like" which only includes SUBREG_TO_REG, with the 2 immediate operands
There was a problem hiding this comment.
Right, but the fact that the only copy-like instruction is SUBREG_TO_REG is not at all evident. In my reading of the code this looks like "out of all copy-like instructions, only SUBREG_TO_REG is allowed to have an immediate operand", so the question comes to mind "what is special about SUBREG_TO_REG?".
This is obviously subjective. Feel free to commit as-is. I'm just sharing my perspective on this.
| @@ -1694,12 +1694,19 @@ MachineInstr *RegisterCoalescer::eliminateUndefCopy(MachineInstr *CopyMI) { | |||
| // The source interval may also have been on an undef use, in which case the | |||
| // copy introduced a live value. | |||
| if (((V && V->isPHIDef()) || (!V && !DstLI.liveAt(Idx)))) { | |||
There was a problem hiding this comment.
I know this isn't a part of your changes, but the excess of parentheses is bugging me.
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.
If this was coalescing a SUBREG_TO_REG as a copy, the resulting instruction would be an IMPLICIT_DEF with an unexpected 2 immediate operands, which need to be dropped. Until recently the verifier did not catch this error, and an assert would fire if later the broken IMPLICIT_DEF was rematerialized P
#73758 is related, it changes the failure mode to a verifier error.