[RegisterCoalescer] Mark implicit-defs of super-registers as dead in remat #159110
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
…remat Currently, something like: ``` $eax = MOV32ri -11, implicit-def $rax %al = COPY $eax ``` Can be rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def $rax ``` Which marks the full $rax as used, not just $al. With this change, this is rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def dead $rax, implicit-def $al ``` To indicate that only $al is used. This issue is latent right now, but is exposed when llvm#134408 is applied, as it results in the register pressure being incorrectly calculated. I think this change is in line with past fixes in this area, notably: llvm@059cead llvm@69cd121
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-backend-x86 Author: Benjamin Maxwell (MacDue) ChangesCurrently, something like: Can be rematerialized as: Which marks the full $rax as used, not just $al. With this change, this is rematerialized as: To indicate that only $al is used. This issue is latent right now, but is exposed when #134408 is applied, as it results in the register pressure being incorrectly calculated. I think this change is in line with past fixes in this area, notably: Full diff: https://github.com/llvm/llvm-project/pull/159110.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index b8486f6560c5f..d117edb6838bd 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -1475,7 +1475,8 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
// The implicit-def of the super register may have been reduced to
// subregisters depending on the uses.
- bool NewMIDefinesFullReg = false;
+ TinyPtrVector<MachineOperand *> NewMIImpDefDestReg;
+ [[maybe_unused]] unsigned NewMIOpCount = NewMI.getNumOperands();
SmallVector<MCRegister, 4> NewMIImplDefs;
for (unsigned i = NewMI.getDesc().getNumOperands(),
@@ -1486,7 +1487,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
assert(MO.isImplicit());
if (MO.getReg().isPhysical()) {
if (MO.getReg() == DstReg)
- NewMIDefinesFullReg = true;
+ NewMIImpDefDestReg.push_back(&MO);
assert(MO.isImplicit() && MO.getReg().isPhysical() &&
(MO.isDead() ||
@@ -1640,9 +1641,32 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
// been asked for. If so it must implicitly define the whole thing.
assert(DstReg.isPhysical() &&
"Only expect virtual or physical registers in remat");
+
+ // When we're rematerializing into a not-quite-right register we already add
+ // the real definition as an implicit-def, but we should also be marking the
+ // "official" register as dead, since nothing else is going to use it as a
+ // result of this remat. Not doing this can affect pressure tracking.
NewMI.getOperand(0).setIsDead(true);
- if (!NewMIDefinesFullReg) {
+ bool HasDefMatchingCopy = false;
+ if (!NewMIImpDefDestReg.empty()) {
+ // Assert to check MachineOperand*s have not been invalidated.
+ assert(
+ NewMIOpCount == NewMI.getNumOperands() &&
+ "Expected NewMI operands not to be appended/removed at this point");
+ // If NewMI has an implicit-def of a super-register of the CopyDstReg,
+ // we must also mark that as dead since it is not going to used as a
+ // result of this remat.
+ for (MachineOperand *MO : NewMIImpDefDestReg) {
+ if (MO->getReg() != CopyDstReg)
+ MO->setIsDead(true);
+ else
+ HasDefMatchingCopy = true;
+ }
+ }
+
+ // If NewMI does not already have an implicit-def CopyDstReg add one now.
+ if (!HasDefMatchingCopy) {
NewMI.addOperand(MachineOperand::CreateReg(
CopyDstReg, true /*IsDef*/, true /*IsImp*/, false /*IsKill*/));
}
diff --git a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
index b99c5fc8df0cb..44a2aecdc3672 100644
--- a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
+++ b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
@@ -165,5 +165,25 @@ body: |
bb.3:
$rax = COPY %t3
RET 0, $rax
-
...
+---
+name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
+ ; CHECK: dead $esi = MOV32r0 implicit-def dead $eflags, implicit-def $rsi
+ ; CHECK-NEXT: dead $edx = MOV32r0 implicit-def dead $eflags, implicit-def $rdx
+ ; CHECK-NEXT: FAKE_USE implicit killed $rsi, implicit killed $rdx
+ ; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def dead $rax, implicit-def $al
+ ; CHECK-NEXT: FAKE_USE implicit killed $al
+ ; CHECK-NEXT: $eax = MOV32r0 implicit-def dead $eflags
+ ; CHECK-NEXT: RET 0, $eax
+ undef %1.sub_32bit:gr64_with_sub_8bit = MOV32r0 implicit-def dead $eflags, implicit-def %1
+ $rsi = COPY %1
+ $rdx = COPY %1
+ FAKE_USE implicit killed $rsi, implicit killed $rdx
+ %4:gr8 = COPY killed %1.sub_8bit
+ $al = COPY killed %4
+ FAKE_USE implicit killed $al
+ $eax = MOV32r0 implicit-def dead $eflags
+ RET 0, killed $eax
|
|
@llvm/pr-subscribers-llvm-regalloc Author: Benjamin Maxwell (MacDue) ChangesCurrently, something like: Can be rematerialized as: Which marks the full $rax as used, not just $al. With this change, this is rematerialized as: To indicate that only $al is used. This issue is latent right now, but is exposed when #134408 is applied, as it results in the register pressure being incorrectly calculated. I think this change is in line with past fixes in this area, notably: Full diff: https://github.com/llvm/llvm-project/pull/159110.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index b8486f6560c5f0..d117edb6838bd9 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -1475,7 +1475,8 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
// The implicit-def of the super register may have been reduced to
// subregisters depending on the uses.
- bool NewMIDefinesFullReg = false;
+ TinyPtrVector<MachineOperand *> NewMIImpDefDestReg;
+ [[maybe_unused]] unsigned NewMIOpCount = NewMI.getNumOperands();
SmallVector<MCRegister, 4> NewMIImplDefs;
for (unsigned i = NewMI.getDesc().getNumOperands(),
@@ -1486,7 +1487,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
assert(MO.isImplicit());
if (MO.getReg().isPhysical()) {
if (MO.getReg() == DstReg)
- NewMIDefinesFullReg = true;
+ NewMIImpDefDestReg.push_back(&MO);
assert(MO.isImplicit() && MO.getReg().isPhysical() &&
(MO.isDead() ||
@@ -1640,9 +1641,32 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
// been asked for. If so it must implicitly define the whole thing.
assert(DstReg.isPhysical() &&
"Only expect virtual or physical registers in remat");
+
+ // When we're rematerializing into a not-quite-right register we already add
+ // the real definition as an implicit-def, but we should also be marking the
+ // "official" register as dead, since nothing else is going to use it as a
+ // result of this remat. Not doing this can affect pressure tracking.
NewMI.getOperand(0).setIsDead(true);
- if (!NewMIDefinesFullReg) {
+ bool HasDefMatchingCopy = false;
+ if (!NewMIImpDefDestReg.empty()) {
+ // Assert to check MachineOperand*s have not been invalidated.
+ assert(
+ NewMIOpCount == NewMI.getNumOperands() &&
+ "Expected NewMI operands not to be appended/removed at this point");
+ // If NewMI has an implicit-def of a super-register of the CopyDstReg,
+ // we must also mark that as dead since it is not going to used as a
+ // result of this remat.
+ for (MachineOperand *MO : NewMIImpDefDestReg) {
+ if (MO->getReg() != CopyDstReg)
+ MO->setIsDead(true);
+ else
+ HasDefMatchingCopy = true;
+ }
+ }
+
+ // If NewMI does not already have an implicit-def CopyDstReg add one now.
+ if (!HasDefMatchingCopy) {
NewMI.addOperand(MachineOperand::CreateReg(
CopyDstReg, true /*IsDef*/, true /*IsImp*/, false /*IsKill*/));
}
diff --git a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
index b99c5fc8df0cb6..44a2aecdc36722 100644
--- a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
+++ b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir
@@ -165,5 +165,25 @@ body: |
bb.3:
$rax = COPY %t3
RET 0, $rax
-
...
+---
+name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
+ ; CHECK: dead $esi = MOV32r0 implicit-def dead $eflags, implicit-def $rsi
+ ; CHECK-NEXT: dead $edx = MOV32r0 implicit-def dead $eflags, implicit-def $rdx
+ ; CHECK-NEXT: FAKE_USE implicit killed $rsi, implicit killed $rdx
+ ; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def dead $rax, implicit-def $al
+ ; CHECK-NEXT: FAKE_USE implicit killed $al
+ ; CHECK-NEXT: $eax = MOV32r0 implicit-def dead $eflags
+ ; CHECK-NEXT: RET 0, $eax
+ undef %1.sub_32bit:gr64_with_sub_8bit = MOV32r0 implicit-def dead $eflags, implicit-def %1
+ $rsi = COPY %1
+ $rdx = COPY %1
+ FAKE_USE implicit killed $rsi, implicit killed $rdx
+ %4:gr8 = COPY killed %1.sub_8bit
+ $al = COPY killed %4
+ FAKE_USE implicit killed $al
+ $eax = MOV32r0 implicit-def dead $eflags
+ RET 0, killed $eax
|
arsenm
reviewed
Sep 18, 2025
| // the real definition as an implicit-def, but we should also be marking the | ||
| // "official" register as dead, since nothing else is going to use it as a | ||
| // result of this remat. Not doing this can affect pressure tracking. | ||
| NewMI.getOperand(0).setIsDead(true); |
There was a problem hiding this comment.
Is this whole function reinventing MachineInstr::addRegisterDead?
There was a problem hiding this comment.
Not really. MachineInstr::addRegisterDead seems to have a definition of "dead" that conflicts with this code. Here, a super-register dead and a sub-register defined means "everything except the sub-register is dead" (which is required for pressure tracking), but addRegisterDead seems to disagree, as addRegisterDead($rax) will turn:
dead $eax = FOO implicit-def $rax into $eax = FOO implicit-def dead $rax (which implies it thinks "all sub-registers covered by a dead register are dead").
MacDue
force-pushed
the
impdef_remat
branch
from
5857f0c to
af055e9
Compare
arsenm
approved these changes
Sep 25, 2025
sdesmalen-arm
approved these changes
Sep 25, 2025
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
…remat (llvm#159110) Currently, something like: ``` $eax = MOV32ri -11, implicit-def $rax %al = COPY $eax ``` Can be rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def $rax ``` Which marks the full $rax as used, not just $al. With this change, this is rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def dead $rax, implicit-def $al ``` To indicate that only $al is used. Note: This issue is latent right now, but is exposed when llvm#134408 is applied, as it results in the register pressure being incorrectly calculated (unless this patch is applied too). I think this change is in line with past fixes in this area, notably: llvm@059cead llvm@69cd121
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.
Currently, something like:
Can be rematerialized as:
Which marks the full $rax as used, not just $al.
With this change, this is rematerialized as:
To indicate that only $al is used.
Note: This issue is latent right now, but is exposed when #134408 is applied, as it results in the register pressure being incorrectly calculated (unless this patch is applied too).
I think this change is in line with past fixes in this area, notably:
059cead
69cd121