[SystemZ] Handle address clobbering in splitMove().#92105
Merged
[SystemZ] Handle address clobbering in splitMove().#92105
Conversation
Member
|
@llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesWhen expanding an L128 (which is used to reload i128) it is Fixes #91437 Full diff: https://github.com/llvm/llvm-project/pull/92105.diff 2 Files Affected:
diff --git a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
index 0a29b4f79c7d0..16bbfd44ef8a9 100644
--- a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
@@ -70,49 +70,62 @@ void SystemZInstrInfo::splitMove(MachineBasicBlock::iterator MI,
MachineBasicBlock *MBB = MI->getParent();
MachineFunction &MF = *MBB->getParent();
- // Get two load or store instructions. Use the original instruction for one
- // of them (arbitrarily the second here) and create a clone for the other.
- MachineInstr *EarlierMI = MF.CloneMachineInstr(&*MI);
- MBB->insert(MI, EarlierMI);
+ // Get two load or store instructions. Use the original instruction for
+ // one of them and create a clone for the other.
+ MachineInstr *HighPartMI = MF.CloneMachineInstr(&*MI);
+ MachineInstr *LowPartMI = &*MI;
+ MBB->insert(LowPartMI, HighPartMI);
// Set up the two 64-bit registers and remember super reg and its flags.
- MachineOperand &HighRegOp = EarlierMI->getOperand(0);
- MachineOperand &LowRegOp = MI->getOperand(0);
+ MachineOperand &HighRegOp = HighPartMI->getOperand(0);
+ MachineOperand &LowRegOp = LowPartMI->getOperand(0);
Register Reg128 = LowRegOp.getReg();
unsigned Reg128Killed = getKillRegState(LowRegOp.isKill());
unsigned Reg128Undef = getUndefRegState(LowRegOp.isUndef());
HighRegOp.setReg(RI.getSubReg(HighRegOp.getReg(), SystemZ::subreg_h64));
LowRegOp.setReg(RI.getSubReg(LowRegOp.getReg(), SystemZ::subreg_l64));
- if (MI->mayStore()) {
- // Add implicit uses of the super register in case one of the subregs is
- // undefined. We could track liveness and skip storing an undefined
- // subreg, but this is hopefully rare (discovered with llvm-stress).
- // If Reg128 was killed, set kill flag on MI.
- unsigned Reg128UndefImpl = (Reg128Undef | RegState::Implicit);
- MachineInstrBuilder(MF, EarlierMI).addReg(Reg128, Reg128UndefImpl);
- MachineInstrBuilder(MF, MI).addReg(Reg128, (Reg128UndefImpl | Reg128Killed));
- }
-
// The address in the first (high) instruction is already correct.
// Adjust the offset in the second (low) instruction.
- MachineOperand &HighOffsetOp = EarlierMI->getOperand(2);
- MachineOperand &LowOffsetOp = MI->getOperand(2);
+ MachineOperand &HighOffsetOp = HighPartMI->getOperand(2);
+ MachineOperand &LowOffsetOp = LowPartMI->getOperand(2);
LowOffsetOp.setImm(LowOffsetOp.getImm() + 8);
- // Clear the kill flags on the registers in the first instruction.
- if (EarlierMI->getOperand(0).isReg() && EarlierMI->getOperand(0).isUse())
- EarlierMI->getOperand(0).setIsKill(false);
- EarlierMI->getOperand(1).setIsKill(false);
- EarlierMI->getOperand(3).setIsKill(false);
-
// Set the opcodes.
unsigned HighOpcode = getOpcodeForOffset(NewOpcode, HighOffsetOp.getImm());
unsigned LowOpcode = getOpcodeForOffset(NewOpcode, LowOffsetOp.getImm());
assert(HighOpcode && LowOpcode && "Both offsets should be in range");
+ HighPartMI->setDesc(get(HighOpcode));
+ LowPartMI->setDesc(get(LowOpcode));
+
+ MachineInstr *FirstMI = HighPartMI;
+ if (MI->mayStore()) {
+ FirstMI->getOperand(0).setIsKill(false);
+ // Add implicit uses of the super register in case one of the subregs is
+ // undefined. We could track liveness and skip storing an undefined
+ // subreg, but this is hopefully rare (discovered with llvm-stress).
+ // If Reg128 was killed, set kill flag on MI.
+ unsigned Reg128UndefImpl = (Reg128Undef | RegState::Implicit);
+ MachineInstrBuilder(MF, HighPartMI).addReg(Reg128, Reg128UndefImpl);
+ MachineInstrBuilder(MF, LowPartMI).addReg(Reg128, (Reg128UndefImpl | Reg128Killed));
+ } else {
+ // If HighPartMI clobbers any of the address registers, it needs to come
+ // after LowPartMI.
+ auto overlapsAddressReg = [&](Register Reg) -> bool {
+ return RI.regsOverlap(Reg, MI->getOperand(1).getReg()) ||
+ RI.regsOverlap(Reg, MI->getOperand(3).getReg());
+ };
+ if (overlapsAddressReg(HighRegOp.getReg())) {
+ assert(!overlapsAddressReg(LowRegOp.getReg()) &&
+ "Both loads clobber address!");
+ MBB->splice(HighPartMI, MBB, LowPartMI);
+ FirstMI = LowPartMI;
+ }
+ }
- EarlierMI->setDesc(get(HighOpcode));
- MI->setDesc(get(LowOpcode));
+ // Clear the kill flags on the address registers in the first instruction.
+ FirstMI->getOperand(1).setIsKill(false);
+ FirstMI->getOperand(3).setIsKill(false);
}
// Split ADJDYNALLOC instruction MI.
diff --git a/llvm/test/CodeGen/SystemZ/splitMove_addressReg.mir b/llvm/test/CodeGen/SystemZ/splitMove_addressReg.mir
new file mode 100644
index 0000000000000..64ed2d8f2c00a
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/splitMove_addressReg.mir
@@ -0,0 +1,26 @@
+# RUN: llc -mtriple=s390x-linux-gnu -run-pass=postrapseudos \
+# RUN: %s -o - -verify-machineinstrs | FileCheck %s
+#
+# Test that a L128 reload do not overwrite an address register prematurely
+# after being split into two LGs.
+
+--- |
+ target triple = "s390x-unknown-unknown"
+
+ define void @fun() {
+ ret void
+ }
+
+...
+
+# CHECK: name: fun
+
+---
+name: 'fun'
+body: |
+ bb.0:
+ liveins: $r4d, $r15d
+ $r4q = L128 $r15d, 14920, killed $r4d
+ Return
+
+...
|
You can test this locally with the following command:git-clang-format --diff c4a9a374749deb5f2a932a7d4ef9321be1b2ae5d 50e39cf878a5642c19ca67d4932e14d44a6a0133 -- llvm/lib/Target/SystemZ/SystemZInstrInfo.cppView the diff from clang-format here.diff --git a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
index 16bbfd44ef..4990825d75 100644
--- a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
@@ -107,7 +107,8 @@ void SystemZInstrInfo::splitMove(MachineBasicBlock::iterator MI,
// If Reg128 was killed, set kill flag on MI.
unsigned Reg128UndefImpl = (Reg128Undef | RegState::Implicit);
MachineInstrBuilder(MF, HighPartMI).addReg(Reg128, Reg128UndefImpl);
- MachineInstrBuilder(MF, LowPartMI).addReg(Reg128, (Reg128UndefImpl | Reg128Killed));
+ MachineInstrBuilder(MF, LowPartMI)
+ .addReg(Reg128, (Reg128UndefImpl | Reg128Killed));
} else {
// If HighPartMI clobbers any of the address registers, it needs to come
// after LowPartMI.
|
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
May 15, 2024
When expanding an L128 (which is used to reload i128) it is possible that the quadword destination register clobbers an address register. This patch adds an assertion against the case where both of the expanded parts clobber the address, and in the case where one of the expanded parts do so puts it last. Fixes llvm#91437 (cherry picked from commit d6ee7e8)
tstellar
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
May 16, 2024
When expanding an L128 (which is used to reload i128) it is possible that the quadword destination register clobbers an address register. This patch adds an assertion against the case where both of the expanded parts clobber the address, and in the case where one of the expanded parts do so puts it last. Fixes llvm#91437 (cherry picked from commit d6ee7e8)
Tedlion
pushed a commit
to Tedlion/llvm-project
that referenced
this pull request
Jun 15, 2025
When expanding an L128 (which is used to reload i128) it is possible that the quadword destination register clobbers an address register. This patch adds an assertion against the case where both of the expanded parts clobber the address, and in the case where one of the expanded parts do so puts it last. Fixes llvm#91437 (cherry picked from commit d6ee7e8)
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
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 expanding an L128 (which is used to reload i128) it is
possible that the quadword destination register clobbers an
address register. This patch adds an assertion against the case
where both of the expanded parts clobber the address, and in the
case where one of the expanded parts do so puts it last.
Fixes #91437