[CodeGen][NVPTX][WebAssembly] Add copyReg interface to TargetInstrInfo that takes Register instead of MCRegister. #128456
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
…o that takes Register instead of MCRegister. NVPTX, SPIRV, and WebAssembly all need to expand copies of virtual registers because they don't go through register allocation. The copyPhysReg interface currently used takes two MCRegister arguments that get implicitly converted from Registers in LowerCopy. It should be illegal to convert a virtual register Register to an MCRegister. This patch adds a new interface copyReg that takes two Registers. The default implementation falls back to copyPhysReg with an explicit conversion. NVPTX, SPIRV, and WebAssembly are modified to implement the new interface. I chose this approach to be least disruptive to out of tree targets. Alternatively, we could change copyPhysReg to take Register operands, though we would probably want to rename it to remove "Phys".
topperc
requested review from
arsenm,
jholewinski,
sunfishcode,
efriedma-quic and
VyacheslavLevytskyy
|
@llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-backend-webassembly Author: Craig Topper (topperc) ChangesNVPTX, SPIRV, and WebAssembly all need to expand copies of virtual registers because they don't go through register allocation. The copyPhysReg interface currently used takes two MCRegister arguments that get implicitly converted from Registers in LowerCopy. It should be illegal to convert a virtual register Register to an MCRegister. This patch adds a new interface copyReg that takes two Registers. The default implementation falls back to copyPhysReg with an explicit conversion. NVPTX, SPIRV, and WebAssembly are modified to implement the new interface. I chose this approach to be least disruptive to out of tree targets. Alternatively, we could change copyPhysReg to take Register operands, though we would probably want to rename it to remove "Phys". Full diff: https://github.com/llvm/llvm-project/pull/128456.diff 8 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index c8eba71c9bb0a..f7e502ee04448 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1040,6 +1040,16 @@ class TargetInstrInfo : public MCInstrInfo {
bool RenamableSrc = false) const {
llvm_unreachable("Target didn't implement TargetInstrInfo::copyPhysReg!");
}
+ // Similar to copycopyPhysReg, but for targets that don't do register
+ // allocation and need to copy virtual registers like NVPTX, SPIR-V, and
+ // WebAssembly.
+ virtual void copyReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
+ const DebugLoc &DL, Register DestReg, Register SrcReg,
+ bool KillSrc, bool RenamableDest = false,
+ bool RenamableSrc = false) const {
+ copyPhysReg(MBB, MI, DL, DestReg.asMCReg(), SrcReg.asMCReg(), KillSrc,
+ RenamableDest, RenamableSrc);
+ }
/// Allow targets to tell MachineVerifier whether a specific register
/// MachineOperand can be used as part of PC-relative addressing.
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 7a905b65f26e5..64d9318ce16a1 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -822,10 +822,10 @@ void TargetInstrInfo::lowerCopy(MachineInstr *MI,
return;
}
- copyPhysReg(*MI->getParent(), MI, MI->getDebugLoc(), DstMO.getReg(),
- SrcMO.getReg(), SrcMO.isKill(),
- DstMO.getReg().isPhysical() ? DstMO.isRenamable() : false,
- SrcMO.getReg().isPhysical() ? SrcMO.isRenamable() : false);
+ copyReg(*MI->getParent(), MI, MI->getDebugLoc(), DstMO.getReg(),
+ SrcMO.getReg(), SrcMO.isKill(),
+ DstMO.getReg().isPhysical() ? DstMO.isRenamable() : false,
+ SrcMO.getReg().isPhysical() ? SrcMO.isRenamable() : false);
if (MI->getNumOperands() > 2)
transferImplicitOperands(MI, TRI);
diff --git a/llvm/lib/Target/NVPTX/NVPTXInstrInfo.cpp b/llvm/lib/Target/NVPTX/NVPTXInstrInfo.cpp
index b4dbe6a0930ca..4101ebd96d2a4 100644
--- a/llvm/lib/Target/NVPTX/NVPTXInstrInfo.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXInstrInfo.cpp
@@ -26,11 +26,10 @@ void NVPTXInstrInfo::anchor() {}
NVPTXInstrInfo::NVPTXInstrInfo() : RegInfo() {}
-void NVPTXInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
- MachineBasicBlock::iterator I,
- const DebugLoc &DL, MCRegister DestReg,
- MCRegister SrcReg, bool KillSrc,
- bool RenamableDest, bool RenamableSrc) const {
+void NVPTXInstrInfo::copyReg(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator I, const DebugLoc &DL,
+ Register DestReg, Register SrcReg, bool KillSrc,
+ bool RenamableDest, bool RenamableSrc) const {
const MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
const TargetRegisterClass *DestRC = MRI.getRegClass(DestReg);
const TargetRegisterClass *SrcRC = MRI.getRegClass(SrcReg);
diff --git a/llvm/lib/Target/NVPTX/NVPTXInstrInfo.h b/llvm/lib/Target/NVPTX/NVPTXInstrInfo.h
index 06b111c69fb74..dd5662b5eb54a 100644
--- a/llvm/lib/Target/NVPTX/NVPTXInstrInfo.h
+++ b/llvm/lib/Target/NVPTX/NVPTXInstrInfo.h
@@ -50,10 +50,10 @@ class NVPTXInstrInfo : public NVPTXGenInstrInfo {
* MachineInstr::MIFlag Flags = MachineInstr::NoFlags) const;
*/
- void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
- const DebugLoc &DL, MCRegister DestReg, MCRegister SrcReg,
- bool KillSrc, bool RenamableDest = false,
- bool RenamableSrc = false) const override;
+ void copyReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
+ const DebugLoc &DL, Register DestReg, Register SrcReg,
+ bool KillSrc, bool RenamableDest = false,
+ bool RenamableSrc = false) const override;
// Branch analysis.
bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
index 49b6b3bbb6cef..313c0903381a1 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
@@ -256,11 +256,10 @@ unsigned SPIRVInstrInfo::insertBranch(MachineBasicBlock &MBB,
return 1;
}
-void SPIRVInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
- MachineBasicBlock::iterator I,
- const DebugLoc &DL, MCRegister DestReg,
- MCRegister SrcReg, bool KillSrc,
- bool RenamableDest, bool RenamableSrc) const {
+void SPIRVInstrInfo::copyReg(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator I, const DebugLoc &DL,
+ Register DestReg, Register SrcReg, bool KillSrc,
+ bool RenamableDest, bool RenamableSrc) const {
// Actually we don't need this COPY instruction. However if we do nothing with
// it, post RA pseudo instrs expansion just removes it and we get the code
// with undef registers. Therefore, we need to replace all uses of dst with
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
index 4e5059b4b8891..06c5e9741120c 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
@@ -50,10 +50,10 @@ class SPIRVInstrInfo : public SPIRVGenInstrInfo {
MachineBasicBlock *FBB, ArrayRef<MachineOperand> Cond,
const DebugLoc &DL,
int *BytesAdded = nullptr) const override;
- void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
- const DebugLoc &DL, MCRegister DestReg, MCRegister SrcReg,
- bool KillSrc, bool RenamableDest = false,
- bool RenamableSrc = false) const override;
+ void copyReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
+ const DebugLoc &DL, Register DestReg, Register SrcReg,
+ bool KillSrc, bool RenamableDest = false,
+ bool RenamableSrc = false) const override;
bool expandPostRAPseudo(MachineInstr &MI) const override;
};
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp
index 75011ab3c8721..ed060b9c63bb3 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp
@@ -54,17 +54,17 @@ bool WebAssemblyInstrInfo::isReallyTriviallyReMaterializable(
}
}
-void WebAssemblyInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
- MachineBasicBlock::iterator I,
- const DebugLoc &DL, MCRegister DestReg,
- MCRegister SrcReg, bool KillSrc,
- bool RenamableDest,
- bool RenamableSrc) const {
+void WebAssemblyInstrInfo::copyReg(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator I,
+ const DebugLoc &DL, Register DestReg,
+ Register SrcReg, bool KillSrc,
+ bool RenamableDest,
+ bool RenamableSrc) const {
// This method is called by post-RA expansion, which expects only pregs to
// exist. However we need to handle both here.
auto &MRI = MBB.getParent()->getRegInfo();
const TargetRegisterClass *RC =
- Register::isVirtualRegister(DestReg)
+ DestReg.isVirtual()
? MRI.getRegClass(DestReg)
: MRI.getTargetRegisterInfo()->getMinimalPhysRegClass(DestReg);
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h b/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h
index 474f04628066b..b4141cb86b4b7 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h
@@ -39,10 +39,10 @@ class WebAssemblyInstrInfo final : public WebAssemblyGenInstrInfo {
bool isReallyTriviallyReMaterializable(const MachineInstr &MI) const override;
- void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
- const DebugLoc &DL, MCRegister DestReg, MCRegister SrcReg,
- bool KillSrc, bool RenamableDest = false,
- bool RenamableSrc = false) const override;
+ void copyReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
+ const DebugLoc &DL, Register DestReg, Register SrcReg,
+ bool KillSrc, bool RenamableDest = false,
+ bool RenamableSrc = false) const override;
MachineInstr *commuteInstructionImpl(MachineInstr &MI, bool NewMI,
unsigned OpIdx1,
unsigned OpIdx2) const override;
|
arsenm
reviewed
Feb 24, 2025
So you would prefer to change copyPhysReg to take Register instead of MCRegister? |
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Feb 24, 2025
…egister. NVPTX, SPIRV, and WebAssembly pass virtual registers to this function since they don't perform register allocation. We need to use Register to avoid a virtual register being converted to MCRegister by the caller. This is an alternative to llvm#128456.
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.
NVPTX, SPIRV, and WebAssembly all need to expand copies of virtual registers because they don't go through register allocation.
The copyPhysReg interface currently used takes two MCRegister arguments that get implicitly converted from Registers in LowerCopy. It should be illegal to convert a virtual register Register to an MCRegister.
This patch adds a new interface copyReg that takes two Registers. The default implementation falls back to copyPhysReg with an explicit conversion. NVPTX, SPIRV, and WebAssembly are modified to implement the new interface.
I chose this approach to be least disruptive to out of tree targets. Alternatively, we could change copyPhysReg to take Register operands, though we would probably want to rename it to remove "Phys".