[AMDGPU] Use a consistent DwarfEH register flavour #84513
Conversation
Previously, we always used the wave64 encodings for EH registers regardless of whether we were compiling for wave32, which seems wrong. We don't seem to use the EH registers, so this commit is mostly just about papering over code that converts from non-EH dwarf registers to LLVM registers while claiming they are EH dwarf registers. That kind of code should be okay on any non-darwin target (since darwin is the only target that uses a different encoding for EH registers).
|
@llvm/pr-subscribers-backend-amdgpu Author: Emma Pilkington (epilk) ChangesPreviously, we always used the wave64 encodings for EH registers regardless of whether we were compiling for wave32, which seems wrong. We don't seem to use the EH registers, so this commit is mostly just about papering over code that converts from non-EH dwarf registers to LLVM registers while claiming they are EH dwarf registers. That kind of code should be okay on any non-darwin target (since darwin is the only target that uses a different encoding for EH registers). For example, this is converting from an EH register to an LLVM register, which can result in "badreg" being printed on downstream tests. Full diff: https://github.com/llvm/llvm-project/pull/84513.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
index a6a01479b5b18a..4700a984770bfb 100644
--- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
+++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
@@ -70,7 +70,7 @@ static MCRegisterInfo *createAMDGPUMCRegisterInfo(const Triple &TT) {
MCRegisterInfo *llvm::createGCNMCRegisterInfo(AMDGPUDwarfFlavour DwarfFlavour) {
MCRegisterInfo *X = new MCRegisterInfo();
- InitAMDGPUMCRegisterInfo(X, AMDGPU::PC_REG, DwarfFlavour);
+ InitAMDGPUMCRegisterInfo(X, AMDGPU::PC_REG, DwarfFlavour, DwarfFlavour);
return X;
}
diff --git a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
index 3664535b325997..5c64c6bcd1968c 100644
--- a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
@@ -318,8 +318,9 @@ struct SGPRSpillBuilder {
} // namespace llvm
SIRegisterInfo::SIRegisterInfo(const GCNSubtarget &ST)
- : AMDGPUGenRegisterInfo(AMDGPU::PC_REG, ST.getAMDGPUDwarfFlavour()), ST(ST),
- SpillSGPRToVGPR(EnableSpillSGPRToVGPR), isWave32(ST.isWave32()) {
+ : AMDGPUGenRegisterInfo(AMDGPU::PC_REG, ST.getAMDGPUDwarfFlavour(),
+ ST.getAMDGPUDwarfFlavour()),
+ ST(ST), SpillSGPRToVGPR(EnableSpillSGPRToVGPR), isWave32(ST.isWave32()) {
assert(getSubRegIndexLaneMask(AMDGPU::sub0).getAsInteger() == 3 &&
getSubRegIndexLaneMask(AMDGPU::sub31).getAsInteger() == (3ULL << 62) &&
diff --git a/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp b/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
index e1acb8677a0462..7f7a3720cf7ceb 100644
--- a/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
+++ b/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
@@ -55,6 +55,7 @@ TEST(AMDGPUDwarfRegMappingTests, TestWave64DwarfRegMapping) {
for (int llvmReg : {16, 17, 32, 95, 1088, 1129, 2560, 2815, 3072, 3327}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
@@ -73,6 +74,7 @@ TEST(AMDGPUDwarfRegMappingTests, TestWave32DwarfRegMapping) {
for (int llvmReg : {16, 1, 32, 95, 1088, 1129, 1536, 1791, 2048, 2303}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
diff --git a/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp b/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
index 620835c5dfc5c2..56da4ce7b43af0 100644
--- a/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
+++ b/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
@@ -29,6 +29,7 @@ TEST(AMDGPU, TestWave64DwarfRegMapping) {
{16, 17, 32, 95, 1088, 1129, 2560, 2815, 3072, 3327}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
@@ -52,6 +53,7 @@ TEST(AMDGPU, TestWave32DwarfRegMapping) {
{16, 1, 32, 95, 1088, 1129, 1536, 1791, 2048, 2303}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
|
|
@llvm/pr-subscribers-mc Author: Emma Pilkington (epilk) ChangesPreviously, we always used the wave64 encodings for EH registers regardless of whether we were compiling for wave32, which seems wrong. We don't seem to use the EH registers, so this commit is mostly just about papering over code that converts from non-EH dwarf registers to LLVM registers while claiming they are EH dwarf registers. That kind of code should be okay on any non-darwin target (since darwin is the only target that uses a different encoding for EH registers). For example, this is converting from an EH register to an LLVM register, which can result in "badreg" being printed on downstream tests. Full diff: https://github.com/llvm/llvm-project/pull/84513.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
index a6a01479b5b18a..4700a984770bfb 100644
--- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
+++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
@@ -70,7 +70,7 @@ static MCRegisterInfo *createAMDGPUMCRegisterInfo(const Triple &TT) {
MCRegisterInfo *llvm::createGCNMCRegisterInfo(AMDGPUDwarfFlavour DwarfFlavour) {
MCRegisterInfo *X = new MCRegisterInfo();
- InitAMDGPUMCRegisterInfo(X, AMDGPU::PC_REG, DwarfFlavour);
+ InitAMDGPUMCRegisterInfo(X, AMDGPU::PC_REG, DwarfFlavour, DwarfFlavour);
return X;
}
diff --git a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
index 3664535b325997..5c64c6bcd1968c 100644
--- a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
@@ -318,8 +318,9 @@ struct SGPRSpillBuilder {
} // namespace llvm
SIRegisterInfo::SIRegisterInfo(const GCNSubtarget &ST)
- : AMDGPUGenRegisterInfo(AMDGPU::PC_REG, ST.getAMDGPUDwarfFlavour()), ST(ST),
- SpillSGPRToVGPR(EnableSpillSGPRToVGPR), isWave32(ST.isWave32()) {
+ : AMDGPUGenRegisterInfo(AMDGPU::PC_REG, ST.getAMDGPUDwarfFlavour(),
+ ST.getAMDGPUDwarfFlavour()),
+ ST(ST), SpillSGPRToVGPR(EnableSpillSGPRToVGPR), isWave32(ST.isWave32()) {
assert(getSubRegIndexLaneMask(AMDGPU::sub0).getAsInteger() == 3 &&
getSubRegIndexLaneMask(AMDGPU::sub31).getAsInteger() == (3ULL << 62) &&
diff --git a/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp b/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
index e1acb8677a0462..7f7a3720cf7ceb 100644
--- a/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
+++ b/llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
@@ -55,6 +55,7 @@ TEST(AMDGPUDwarfRegMappingTests, TestWave64DwarfRegMapping) {
for (int llvmReg : {16, 17, 32, 95, 1088, 1129, 2560, 2815, 3072, 3327}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
@@ -73,6 +74,7 @@ TEST(AMDGPUDwarfRegMappingTests, TestWave32DwarfRegMapping) {
for (int llvmReg : {16, 1, 32, 95, 1088, 1129, 1536, 1791, 2048, 2303}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
diff --git a/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp b/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
index 620835c5dfc5c2..56da4ce7b43af0 100644
--- a/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
+++ b/llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
@@ -29,6 +29,7 @@ TEST(AMDGPU, TestWave64DwarfRegMapping) {
{16, 17, 32, 95, 1088, 1129, 2560, 2815, 3072, 3327}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
@@ -52,6 +53,7 @@ TEST(AMDGPU, TestWave32DwarfRegMapping) {
{16, 1, 32, 95, 1088, 1129, 1536, 1791, 2048, 2303}) {
MCRegister PCReg(*MRI->getLLVMRegNum(llvmReg, false));
EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, false));
+ EXPECT_EQ(llvmReg, MRI->getDwarfRegNum(PCReg, true));
}
}
}
|
There was a problem hiding this comment.
Thank you for tracking this down! I'm still not sure I understand how the IsEH distinction functions generally; did you happen to get an understanding of why the call-site you reference knows it can request with IsEH == true? If not, I still don't think we have to deeply understand this as I don't suspect we will ever want to have a split encoding for IsEH vs other.
It seems like every other targets (just verified this with a quick grep, not certain) is asking for the EH encoding when generating CFI instructions except us, so I guess it seems reasonable to assume that CFI instructions are using EH encodings. Maybe we should switch to asking for |
Previously, we always used the wave64 encodings for EH registers regardless of whether we were compiling for wave32, which seems wrong. We don't seem to use the EH registers, so this commit is mostly just about papering over code that converts from non-EH dwarf registers to LLVM registers while claiming they are EH dwarf registers. That kind of code should be okay on any non-darwin target (since darwin is the only target that uses a different encoding for EH registers).
For example, this is converting from an EH register to an LLVM register, which can result in "badreg" being printed on downstream tests.