From 0cd4c7aabd084a3e281113bebc0a443839887a59 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Tue, 30 Sep 2025 08:56:06 +0900 Subject: [PATCH] CodeGen: Do not store RegisterClass copy costs as a signed value 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. --- .../include/llvm/CodeGen/TargetRegisterInfo.h | 13 +++++--- llvm/include/llvm/MC/MCRegisterInfo.h | 4 +-- .../lib/CodeGen/SelectionDAG/InstrEmitter.cpp | 2 +- .../SelectionDAG/ScheduleDAGSDNodes.cpp | 2 +- llvm/lib/Target/AMDGPU/SIISelLowering.cpp | 2 +- llvm/test/TableGen/RegisterClassCopyCost.td | 31 +++++++++++++++++++ .../TableGen/Common/CodeGenRegisters.cpp | 10 +++++- llvm/utils/TableGen/Common/CodeGenRegisters.h | 2 +- llvm/utils/TableGen/RegisterInfoEmitter.cpp | 3 +- 9 files changed, 56 insertions(+), 13 deletions(-) create mode 100644 llvm/test/TableGen/RegisterClassCopyCost.td 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::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(Bit)->getValue()) << Idx; + + // Saturate negative costs to the maximum + if (CopyCostParsed < 0) + CopyCost = std::numeric_limits::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 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(RC.CopyCost) << ", " << (RC.Allocatable ? "true" : "false") << ", " << (RC.getBaseClassOrder() ? "true" : "false") << " },\n"; }