This is an archive of the discontinued LLVM Phabricator instance.

Differential D105800

[AMDGPU] Tidy SReg/SGPR definitions using template class
ClosedPublic

Authored by critson on Jul 12 2021, 1:41 AM.

Details

Reviewers
arsenm
rampitec
foad
Commits
rGc7f2f81f5e2a: [AMDGPU] Tidy SReg/SGPR definitions using template class
Summary

Use a multiclass to consistently define SReg/SGPR/TTMP register classes.
Add missing TTMP registers for 96b, 160b, 192b, 224b.

Diff Detail

Unit TestsFailed

TimeTest
2,750 msx64 debian > libarcher.critical::critical.c
2,640 msx64 debian > libarcher.parallel::parallel-simple2.c
2,700 msx64 debian > libarcher.races::critical-unrelated.c
2,690 msx64 debian > libarcher.races::lock-nested-unrelated.c
2,670 msx64 debian > libarcher.races::lock-unrelated.c
View Full Test Results (18 Failed)

Event Timeline

critson created this revision.Jul 12 2021, 1:41 AM
Herald added subscribers: kerbowa, hiraditya, t-tye and 6 others. · View Herald TranscriptJul 12 2021, 1:41 AM
critson requested review of this revision.Jul 12 2021, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 1:41 AM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
Harbormaster completed remote builds in B113453: Diff 357861.Jul 12 2021, 2:24 AM
foad added a comment.Jul 12 2021, 7:37 AM

Looks like a nice refactoring.

llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

Was SGPR_96 some kind of anomaly among the existing classes?

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
700

Don't you want to divide by two rounding up, so !sra(!add(numRegs, 1), 1)?

727

Should we be defining TTMP classes for all sizes instead of working around it like this?

arsenm added inline comments.Jul 12 2021, 7:52 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

I think the *GPR_* class names are better where applicable

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
733

Typo inherence

critson updated this revision to Diff 358203.Jul 13 2021, 2:23 AM
critson marked 4 inline comments as done.
  • Address reviewer comments
critson added inline comments.Jul 13 2021, 2:25 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

OK, I've put these back. Pretty much everything else in this file uses SReg_*

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
700

Thanks.

727

At the moment if I create the TTMP_96Regs set then SGPR_128 stops working correctly. So I need to investigate further if we want to do that. Also SGPR_1024 will never have an associated TTMP set because there are only 16 trap registers.

Harbormaster completed remote builds in B113691: Diff 358203.Jul 13 2021, 3:03 AM
critson marked an inline comment as done.Jul 13 2021, 7:55 PM
In D105800#2871152, @foad wrote:

Should we be defining TTMP classes for all sizes instead of working around it like this?

I don't think we can at present.

If I define TTMP_96Regs this implicitly causes TTMP_128_with_sub0_sub1_sub2 to be created.
This then creates SReg_128_with_sub0_sub1_sub2.
Now SGPR_128_with_sub0_sub1_sub2 inherits from SReg_128_with_sub0_sub1_sub2 instead of from SGPR_128.
(The superclass of SReg_128_with_sub0_sub1_sub2 is SReg_128, but the superclasses of SGPR_128_with_sub0_sub1_sub2 are SReg_128, SGPR_128, SReg_128_with_sub0_sub1_sub2.)
The net result is that SGPR_128_with_sub0_sub1_sub2 is marked isAllocatable = 0, causing allocation new failures.

The bottom line is that I don't think isAllocatable = 0 currently supports what we are trying to do.
Best guess is it is only appropriate for isolated or leaf register classes.
I can pursue a TableGen change to try and address this, but it would be good to get the immediate issue of allocation failures with SGPR_192 fixed.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
727

Should we be defining TTMP classes for all sizes instead of working around it like this?

critson updated this revision to Diff 359212.Jul 15 2021, 9:45 PM
  • Rework based on TableGen changes in D105967
  • Define all applicable TTMP register classes
critson edited the summary of this revision. (Show Details)Jul 15 2021, 9:45 PM
Harbormaster completed remote builds in B114415: Diff 359212.Jul 15 2021, 10:27 PM
foad accepted this revision.Jul 16 2021, 5:03 AM

Looks good, just nits inline.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
699

Wouldn't it be a bit simpler to use [] as the default, and test it with !empty?

700

Isn't defvar a better way of defining all these variables, instead of making them optional arguments that are never actually used?

This revision is now accepted and ready to land.Jul 16 2021, 5:03 AM
critson marked 2 inline comments as done.Jul 16 2021, 7:12 PM
critson added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
699

It's not possible to initialise a SIRegisterTuples variable with [], so this is the simplest solution in my opinion. Beyond creating a dummy SIRegisterTuples variable to use.

700

Good point. It seems we don't actually use defvar much in AMDGPU.

critson updated this revision to Diff 359517.Jul 16 2021, 7:12 PM
critson marked 2 inline comments as done.
  • Address nits
  • Fix allocation priority of SReg_224
This revision was landed with ongoing or failed builds.Jul 16 2021, 7:27 PM
Closed by commit rGc7f2f81f5e2a: [AMDGPU] Tidy SReg/SGPR definitions using template class (authored by critson). · Explain Why
This revision was automatically updated to reflect the committed changes.
critson added a commit: rGc7f2f81f5e2a: [AMDGPU] Tidy SReg/SGPR definitions using template class.
Harbormaster completed remote builds in B114651: Diff 359517.Jul 16 2021, 7:56 PM

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
161 lines

Diff 358203

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

Show First 20 Lines • Show All 687 Lines • ▼ Show 20 Lines
} }
def SReg_1 : RegisterClass<"AMDGPU", [i1], 32, def SReg_1 : RegisterClass<"AMDGPU", [i1], 32,
(add SReg_1_XEXEC, EXEC, EXEC_LO)> { (add SReg_1_XEXEC, EXEC, EXEC_LO)> {
let CopyCost = 1; let CopyCost = 1;
let isAllocatable = 0; let isAllocatable = 0;
} }
// Requires 2 s_mov_b64 to copy multiclass SRegClass<int numRegs, int priority,
let CopyCost = 2 in { list<ValueType> regTypes,
SIRegisterTuples regList,
// There are no 3-component scalar instructions, but this is needed SIRegisterTuples ttmpList = regList,
foadUnsubmitted
Done
ReplyInline Actions

Wouldn't it be a bit simpler to use [] as the default, and test it with !empty?

foad: Wouldn't it be a bit simpler to use `[]` as the default, and test it with !empty?
critsonAuthorUnsubmitted
Done
ReplyInline Actions

It's not possible to initialise a SIRegisterTuples variable with [], so this is the simplest solution in my opinion. Beyond creating a dummy SIRegisterTuples variable to use.

critson: It's not possible to initialise a SIRegisterTuples variable with [], so this is the simplest…
// for symmetry with VGPRs. int copyCost = !sra(!add(numRegs, 1), 1),
foadUnsubmitted
Done
ReplyInline Actions

Don't you want to divide by two rounding up, so !sra(!add(numRegs, 1), 1)?

foad: Don't you want to divide by two rounding up, so `!sra(!add(numRegs, 1), 1)`?
critsonAuthorUnsubmitted
Done
ReplyInline Actions

Thanks.

critson: Thanks.
foadUnsubmitted
Done
ReplyInline Actions

Isn't defvar a better way of defining all these variables, instead of making them optional arguments that are never actually used?

foad: Isn't defvar a better way of defining all these variables, instead of making them optional…
critsonAuthorUnsubmitted
Done
ReplyInline Actions

Good point. It seems we don't actually use defvar much in AMDGPU.

critson: Good point. It seems we don't actually use defvar much in AMDGPU.
def SGPR_96 : RegisterClass<"AMDGPU", [v3i32, v3f32], 32, bit hasTTMP = !ne(regList, ttmpList),
(add SGPR_96Regs)> { string suffix = !cast<string>(!mul(numRegs, 32)),
let AllocationPriority = 14; string sgprName = !strconcat("SGPR_", suffix),
} string ttmpName = !strconcat("TTMP_", suffix)> {
def SReg_96 : RegisterClass<"AMDGPU", [v3i32, v3f32], 32, def "" # sgprName : RegisterClass<"AMDGPU", regTypes, 32, (add regList)> {
(add SGPR_96)> { let AllocationPriority = priority;
let AllocationPriority = 14; let CopyCost = copyCost;
} }
def SGPR_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64], 32, if hasTTMP then {
(add SGPR_128Regs)> { def "" # ttmpName : RegisterClass<"AMDGPU", regTypes, 32, (add ttmpList)> {
let AllocationPriority = 15; let isAllocatable = 0;
} let CopyCost = copyCost;
def TTMP_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64], 32,
(add TTMP_128Regs)> {
let isAllocatable = 0;
}
def SReg_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64, v2f64], 32,
(add SGPR_128, TTMP_128)> {
let isAllocatable = 0;
}
} // End CopyCost = 2
// There are no 5-component scalar instructions, but this is needed
// for symmetry with VGPRs.
def SGPR_160 : RegisterClass<"AMDGPU", [v5i32, v5f32], 32,
(add SGPR_160Regs)> {
let AllocationPriority = 16;
}
def SReg_160 : RegisterClass<"AMDGPU", [v5i32, v5f32], 32,
(add SGPR_160)> {
// FIXME: Should be isAllocatable = 0, but that causes all TableGen-generated
// subclasses of SGPR_160 to be marked unallocatable too.
// This occurs because SGPR_160 and SReg_160 classes are equivalent in size
// meaning their enumeration order is dependent on alphanumeric ordering of
// their names. The superclass for inherence is the last one in topological
// order (i.e. enumeration order), hence SReg_160 is selected.
// Potential workarounds involve renaming SGPR_160, adding another class
// which is ordered last and hence used for inheritance, or adding more
// registers to SReg_160 to cause it to be moved earlier in the superclass
// list.
let CopyCost = 3;
}
// There are no 6-component scalar instructions, but this is needed
// for symmetry with VGPRs.
def SGPR_192 : RegisterClass<"AMDGPU", [v6i32, v6f32, v3i64, v3f64], 32, (add SGPR_192Regs)> {
let AllocationPriority = 17;
}
def SReg_192 : RegisterClass<"AMDGPU", [v6i32, v6f32, v3i64, v3f64], 32, (add SGPR_192)> {
let isAllocatable = 0;
let CopyCost = 3;
} }
// There are no 7-component scalar instructions, but this is needed
// for symmetry with VGPRs.
def SGPR_224 : RegisterClass<"AMDGPU", [v7i32, v7f32], 32, (add SGPR_224Regs)> {
let AllocationPriority = 18;
} }
def SReg_224 : RegisterClass<"AMDGPU", [v7i32, v7f32], 32, (add SGPR_224)> { def SReg_ # suffix :
let isAllocatable = 0; RegisterClass<"AMDGPU", regTypes, 32,
let CopyCost = 4; !con(!dag(add, [!cast<RegisterClass>(sgprName)], ["sgpr"]),
!if(hasTTMP,
!dag(add, [!cast<RegisterClass>(ttmpName)], ["ttmp"]),
(add)))> {
let AllocationPriority = priority;
let CopyCost = copyCost;
// FIXME: ideally would always be isAllocatable = 0,
foadUnsubmitted
Done
ReplyInline Actions

Should we be defining TTMP classes for all sizes instead of working around it like this?

foad: Should we be defining TTMP classes for all sizes instead of working around it like this?
critsonAuthorUnsubmitted
Done
ReplyInline Actions

At the moment if I create the TTMP_96Regs set then SGPR_128 stops working correctly. So I need to investigate further if we want to do that. Also SGPR_1024 will never have an associated TTMP set because there are only 16 trap registers.

critson: At the moment if I create the TTMP_96Regs set then SGPR_128 stops working correctly. So I need…
critsonAuthorUnsubmitted
Done
ReplyInline Actions

Should we be defining TTMP classes for all sizes instead of working around it like this?

critson: > Should we be defining TTMP classes for all sizes instead of working around it like this?
// but that causes all TableGen-generated subclasses to be marked
// unallocatable too.
// This occurs because with a TTMP_ class, the SGPR_ and SReg_ classes
// are equally sized meaning their enumeration order is dependent on
// alphanumeric ordering of their names.
// The superclass for inheritance is the last one in topological order
arsenmUnsubmitted
Done
ReplyInline Actions

Typo inherence

arsenm: Typo inherence
// (i.e. enumeration order), hence SReg_ is selected over SGPR_.
let isAllocatable = !if(hasTTMP, 0, 1);
} }
def SGPR_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32, (add SGPR_256Regs)> {
let AllocationPriority = 19;
} }
def TTMP_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32, (add TTMP_256Regs)> { defm "" : SRegClass<3, 14, [v3i32, v3f32], SGPR_96Regs>;
let isAllocatable = 0; defm "" : SRegClass<4, 15, [v4i32, v4f32, v2i64], SGPR_128Regs, TTMP_128Regs>;
} defm "" : SRegClass<5, 16, [v5i32, v5f32], SGPR_160Regs>;
defm "" : SRegClass<6, 17, [v6i32, v6f32, v3i64, v3f64], SGPR_192Regs>;
def SReg_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32, defm "" : SRegClass<7, 16, [v7i32, v7f32], SGPR_224Regs>;
(add SGPR_256, TTMP_256)> { defm "" : SRegClass<8, 19, [v8i32, v8f32, v4i64, v4f64], SGPR_256Regs, TTMP_256Regs>;
// Requires 4 s_mov_b64 to copy defm "" : SRegClass<16, 20, [v16i32, v16f32, v8i64, v8f64], SGPR_512Regs, TTMP_512Regs>;
let CopyCost = 4; defm "" : SRegClass<32, 21, [v32i32, v32f32, v16i64, v16f64], SGPR_1024Regs>;
let isAllocatable = 0;
}
def SGPR_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
(add SGPR_512Regs)> {
let AllocationPriority = 20;
}
def TTMP_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
(add TTMP_512Regs)> {
let isAllocatable = 0;
}
def SReg_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
(add SGPR_512, TTMP_512)> {
// Requires 8 s_mov_b64 to copy
let CopyCost = 8;
let isAllocatable = 0;
}
def VRegOrLds_32 : RegisterClass<"AMDGPU", [i32, f32, i16, f16, v2i16, v2f16], 32, def VRegOrLds_32 : RegisterClass<"AMDGPU", [i32, f32, i16, f16, v2i16, v2f16], 32,
(add VGPR_32, LDS_DIRECT_CLASS)> { (add VGPR_32, LDS_DIRECT_CLASS)> {
let isAllocatable = 0; let isAllocatable = 0;
} }
def SGPR_1024 : RegisterClass<"AMDGPU", [v32i32, v32f32, v16i64, v16f64], 32,
(add SGPR_1024Regs)> {
let AllocationPriority = 21;
}
def SReg_1024 : RegisterClass<"AMDGPU", [v32i32, v32f32, v16i64, v16f64], 32,
(add SGPR_1024)> {
let CopyCost = 16;
let isAllocatable = 0;
}
// Register class for all vector registers (VGPRs + Interpolation Registers) // Register class for all vector registers (VGPRs + Interpolation Registers)
class VRegClassBase<int numRegs, list<ValueType> regTypes, dag regList> : class VRegClassBase<int numRegs, list<ValueType> regTypes, dag regList> :
RegisterClass<"AMDGPU", regTypes, 32, regList> { RegisterClass<"AMDGPU", regTypes, 32, regList> {
let Size = !mul(numRegs, 32); let Size = !mul(numRegs, 32);
// Requires n v_mov_b32 to copy // Requires n v_mov_b32 to copy
let CopyCost = numRegs; let CopyCost = numRegs;
let AllocationPriority = numRegs; let AllocationPriority = numRegs;
▲ Show 20 Lines • Show All 325 Lines • Show Last 20 Lines