Skip to content

RegisterCoalescer: Fix producing malformed IMPLICIT_DEFs #73784

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 2 commits into from
Dec 4, 2023
Merged

RegisterCoalescer: Fix producing malformed IMPLICIT_DEFs #73784

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70b843a
RegisterCoalescer: Fix producing malformed IMPLICIT_DEFs
arsenm Nov 28, 2023
89b6bbc
Fix extra parens
arsenm Dec 2, 2023
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: 9 additions & 2 deletions llvm/lib/CodeGen/RegisterCoalescer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))) {
Copy link
Contributor

@kparzysz kparzysz Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know this isn't a part of your changes, but the excess of parentheses is bugging me.

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this assertion? It seems a bit out of place, since nothing else in this function mentions SUBREG_TO_REG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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;
Expand Down
120 changes: 120 additions & 0 deletions llvm/test/CodeGen/AArch64/coalescer-drop-subreg-to-reg-imm-ops.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

...