CodeGen: Do not store RegisterClass copy costs as a signed value #161786
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
Tolerate setting negative values in tablegen, and store them as a saturated uint8_t value. This will allow naive uses of the copy cost to directly add it as a cost without considering the degenerate negative case. The degenerate negative cases are only used in InstrEmitter / DAG scheduling, so leave the special case processing there. There are also fixmes about this system already there. This is the expedient fix for an out of tree target regression after #160084. Currently targets can set a negative copy cost to mark copies as "impossible". However essentially all the in-tree uses only uses this for non-allocatable condition registers. We probably should replace the InstrEmitter/DAG scheduler uses with a more direct check for a copyable register but that has test changes.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-tablegen @llvm/pr-subscribers-llvm-regalloc Author: Matt Arsenault (arsenm) ChangesTolerate setting negative values in tablegen, and store them as a This is the expedient fix for an out of tree target regression Full diff: https://github.com/llvm/llvm-project/pull/161786.diff 9 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
index bf133f0332bcb..822245f13edff 100644
--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
@@ -109,10 +109,15 @@ class TargetRegisterClass {
return MC->contains(Reg1.asMCReg(), Reg2.asMCReg());
}
- /// Return the cost of copying a value between two registers in this class.
- /// A negative number means the register class is very expensive
- /// to copy e.g. status flag register classes.
- int getCopyCost() const { return MC->getCopyCost(); }
+ /// Return the cost of copying a value between two registers in this class. If
+ /// this is the maximum value, the register may be impossible to copy.
+ uint8_t getCopyCost() const { return MC->getCopyCost(); }
+
+ /// \return true if register class is very expensive to copy e.g. status flag
+ /// register classes.
+ bool expensiveOrImpossibleToCopy() const {
+ return MC->getCopyCost() == std::numeric_limits<uint8_t>::max();
+ }
/// Return true if this register class may be used to create virtual
/// registers.
diff --git a/llvm/include/llvm/MC/MCRegisterInfo.h b/llvm/include/llvm/MC/MCRegisterInfo.h
index aad3792f88784..e6fc7077a2dc3 100644
--- a/llvm/include/llvm/MC/MCRegisterInfo.h
+++ b/llvm/include/llvm/MC/MCRegisterInfo.h
@@ -45,7 +45,7 @@ class MCRegisterClass {
const uint16_t RegSetSize;
const uint16_t ID;
const uint16_t RegSizeInBits;
- const int8_t CopyCost;
+ const uint8_t CopyCost;
const bool Allocatable;
const bool BaseClass;
@@ -94,7 +94,7 @@ class MCRegisterClass {
/// getCopyCost - Return the cost of copying a value between two registers in
/// this class. A negative number means the register class is very expensive
/// to copy e.g. status flag register classes.
- int getCopyCost() const { return CopyCost; }
+ uint8_t getCopyCost() const { return CopyCost; }
/// isAllocatable - Return true if this register class may be used to create
/// virtual registers.
diff --git a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
index 11bc64c626421..bb10cf687db8d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
@@ -160,7 +160,7 @@ void InstrEmitter::EmitCopyFromReg(SDValue Op, bool IsClone, Register SrcReg,
// If all uses are reading from the src physical register and copying the
// register is either impossible or very expensive, then don't create a copy.
- if (MatchReg && SrcRC->getCopyCost() < 0) {
+ if (MatchReg && SrcRC->expensiveOrImpossibleToCopy()) {
VRBase = SrcReg;
} else {
// Create the reg, emit the copy.
diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
index 31e78558e239b..79022295f0abd 100644
--- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
@@ -136,7 +136,7 @@ static void CheckForPhysRegDependency(SDNode *Def, SDNode *User, unsigned Op,
if (PhysReg) {
const TargetRegisterClass *RC =
TRI->getMinimalPhysRegClass(Reg, Def->getSimpleValueType(ResNo));
- Cost = RC->getCopyCost();
+ Cost = RC->expensiveOrImpossibleToCopy() ? -1 : RC->getCopyCost();
}
}
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index f7265c5fda9dc..79876ff37b97a 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -18879,7 +18879,7 @@ bool SITargetLowering::checkForPhysRegDependency(
PhysReg = AMDGPU::SCC;
const TargetRegisterClass *RC =
TRI->getMinimalPhysRegClass(PhysReg, Def->getSimpleValueType(ResNo));
- Cost = RC->getCopyCost();
+ Cost = RC->expensiveOrImpossibleToCopy() ? -1 : RC->getCopyCost();
return true;
}
return false;
diff --git a/llvm/test/TableGen/RegisterClassCopyCost.td b/llvm/test/TableGen/RegisterClassCopyCost.td
new file mode 100644
index 0000000000000..fc65fdb5fdbef
--- /dev/null
+++ b/llvm/test/TableGen/RegisterClassCopyCost.td
@@ -0,0 +1,31 @@
+// RUN: llvm-tblgen --gen-register-info -I %p/../../include %s 2>&1 | FileCheck %s
+// RUN: not llvm-tblgen --gen-register-info -I %p/../../include -DERROR %s 2>&1 | FileCheck -check-prefix=ERROR %s
+
+// Check that there is no assertion when specifying unsupported
+// CopyCost values on register classes. Check that negative CopyCost
+// values are saturated to 255.
+
+include "llvm/Target/Target.td"
+
+// CHECK: extern const MCRegisterClass MyTargetMCRegisterClasses[] = {
+// CHECK-NEXT: { GPR32, GPR32Bits, 0, 2, sizeof(GPR32Bits), MyTarget::GPR32RegClassID, 32, 1, true, false },
+// CHECK-NEXT: { SPECIAL_CLASS, SPECIAL_CLASSBits, 6, 1, sizeof(SPECIAL_CLASSBits), MyTarget::SPECIAL_CLASSRegClassID, 32, 255, true, false },
+// CHECK-NEXT: };
+
+def MyTargetISA : InstrInfo;
+def MyTarget : Target { let InstructionSet = MyTargetISA; }
+
+def R0 : Register<"r0"> { let Namespace = "MyTarget"; }
+def R1 : Register<"r1"> { let Namespace = "MyTarget"; }
+def SPECIAL : Register<"special"> { let Namespace = "MyTarget"; }
+
+// ERROR: :[[@LINE+1]]:5: error: 'CopyCost' must be an 8-bit value
+def GPR32 : RegisterClass<"MyTarget", [i32], 32, (add R0, R1)> {
+#ifdef ERROR
+ let CopyCost = 256;
+#endif
+}
+
+def SPECIAL_CLASS : RegisterClass<"MyTarget", [i32], 32, (add SPECIAL)> {
+ let CopyCost = -1;
+}
diff --git a/llvm/utils/TableGen/Common/CodeGenRegisters.cpp b/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
index e873b3eaa4b7e..294f3af5a0055 100644
--- a/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
@@ -744,7 +744,7 @@ CodeGenRegisterClass::CodeGenRegisterClass(CodeGenRegBank &RegBank,
RSI.insertRegSizeForMode(DefaultMode, RI);
}
- CopyCost = R->getValueAsInt("CopyCost");
+ int CopyCostParsed = R->getValueAsInt("CopyCost");
Allocatable = R->getValueAsBit("isAllocatable");
AltOrderSelect = R->getValueAsString("AltOrderSelect");
int AllocationPriority = R->getValueAsInt("AllocationPriority");
@@ -757,6 +757,14 @@ CodeGenRegisterClass::CodeGenRegisterClass(CodeGenRegBank &RegBank,
const BitsInit *TSF = R->getValueAsBitsInit("TSFlags");
for (auto [Idx, Bit] : enumerate(TSF->getBits()))
TSFlags |= uint8_t(cast<BitInit>(Bit)->getValue()) << Idx;
+
+ // Saturate negative costs to the maximum
+ if (CopyCostParsed < 0)
+ CopyCost = std::numeric_limits<uint8_t>::max();
+ else if (!isUInt<8>(CopyCostParsed))
+ PrintFatalError(R->getLoc(), "'CopyCost' must be an 8-bit value");
+
+ CopyCost = CopyCostParsed;
}
// Create an inferred register class that was missing from the .td files.
diff --git a/llvm/utils/TableGen/Common/CodeGenRegisters.h b/llvm/utils/TableGen/Common/CodeGenRegisters.h
index 81aa663b8f11e..89dac125e3d15 100644
--- a/llvm/utils/TableGen/Common/CodeGenRegisters.h
+++ b/llvm/utils/TableGen/Common/CodeGenRegisters.h
@@ -359,7 +359,7 @@ class CodeGenRegisterClass {
StringRef Namespace;
SmallVector<ValueTypeByHwMode, 4> VTs;
RegSizeInfoByHwMode RSI;
- int CopyCost;
+ uint8_t CopyCost;
bool Allocatable;
StringRef AltOrderSelect;
uint8_t AllocationPriority;
diff --git a/llvm/utils/TableGen/RegisterInfoEmitter.cpp b/llvm/utils/TableGen/RegisterInfoEmitter.cpp
index c4b1c5f5a6d9d..a67a5a944b1e1 100644
--- a/llvm/utils/TableGen/RegisterInfoEmitter.cpp
+++ b/llvm/utils/TableGen/RegisterInfoEmitter.cpp
@@ -1083,14 +1083,13 @@ void RegisterInfoEmitter::runMCDesc(raw_ostream &OS) {
std::string RCName = Order.empty() ? "nullptr" : RC.getName();
std::string RCBitsName = Order.empty() ? "nullptr" : RC.getName() + "Bits";
std::string RCBitsSize = Order.empty() ? "0" : "sizeof(" + RCBitsName + ")";
- assert(isInt<8>(RC.CopyCost) && "Copy cost too large.");
uint32_t RegSize = 0;
if (RC.RSI.isSimple())
RegSize = RC.RSI.getSimple().RegSize;
OS << " { " << RCName << ", " << RCBitsName << ", "
<< RegClassStrings.get(RC.getName()) << ", " << RC.getOrder().size()
<< ", " << RCBitsSize << ", " << RC.getQualifiedIdName() << ", "
- << RegSize << ", " << RC.CopyCost << ", "
+ << RegSize << ", " << static_cast<unsigned>(RC.CopyCost) << ", "
<< (RC.Allocatable ? "true" : "false") << ", "
<< (RC.getBaseClassOrder() ? "true" : "false") << " },\n";
}
|
llvmbot
added
backend:AMDGPU
tablegen
llvm:mc
|
I've verified that this patch solves at least one of the regressions we saw with #160084 |
|
The patch looks reasonable to me. As mentioned in the description, we should turn the cost values referenced in the DAGScheduler and the other place to use unsigned as well. |
cdevadas
approved these changes
Oct 3, 2025
arsenm
deleted the
users/arsenm/tablegen-validate-registerclass-copycost
branch
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.
Tolerate setting negative values in tablegen, and store them as a
saturated uint8_t value. This will allow naive uses of the copy cost
to directly add it as a cost without considering the degenerate negative
case. The degenerate negative cases are only used in InstrEmitter / DAG
scheduling, so leave the special case processing there. There are also
fixmes about this system already there.
This is the expedient fix for an out of tree target regression
after #160084. Currently targets can set a negative copy cost to mark
copies as "impossible". However essentially all the in-tree uses only
uses this for non-allocatable condition registers. We probably should
replace the InstrEmitter/DAG scheduler uses with a more direct check
for a copyable register but that has test changes.