This is an archive of the discontinued LLVM Phabricator instance.

Differential D82463

[AMDGPU] Spill more than wavesize CSR SGPRs
ClosedPublic

Authored by saiislam on Jun 24 2020, 7:02 AM.

Details

Reviewers
arsenm
sameerds
rampitec
cdevadas
kerbowa
Commits
rG918231639558: [AMDGPU] Spill more than wavesize CSR SGPRs
Summary

In case of more than wavesize CSR SGPR spills, lanes of reserved VGPR were getting
overwritten due to wrap around.

Reserve a VGPR (when NumVGPRSpillLanes = 0, WaveSize, 2*WaveSize, ..) and when one
of the two conditions is true:

  1. One reserved VGPR being tracked by VGPRReservedForSGPRSpill is not yet reserved.
  2. All spill lanes of reserved VGPR(s) are full and another spill lane is required.

Diff Detail

Unit TestsFailed

TimeTest
210 mswindows > LLVM.CodeGen/AMDGPU::Unknown Unit Message ("")

Event Timeline

saiislam created this revision.Jun 24 2020, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2020, 7:02 AM
Herald added subscribers: llvm-commits, kerbowa, hiraditya and 9 others. · View Herald Transcript
arsenm added inline comments.Jun 24 2020, 7:48 AM
llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
312

I think it might be clearer to split out the logic for the special reserved case.

Could you do something like

if (FuncInfo->VGPRReservedForSGPRSpill && NumVGPRSpillLanes <= WaveSize) {
  assert(VGPRResevedForSGPRSpill == SpillVGPRs.back().VGPR);
  LaneVGPR = VGPRResevedForSGPRSpill;
} else (VGPRIndex == 0) {
 ....
} else {
  LaneVGPR = SpillVGPRs.back().VGPR
}
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
11–12

I'm pretty sure this isn't broken if you do not have a stack object, but these should be unrelated so I think something is still wrong here

Harbormaster completed remote builds in B61540: Diff 273016.Jun 24 2020, 8:36 AM
saiislam updated this revision to Diff 273119.Jun 24 2020, 11:50 AM
  1. Split the VGPR allocation logic as suggested
arsenm added inline comments.Jun 24 2020, 1:13 PM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
2

Maybe also try with and without the reserved VGPR flag?

11

You removed the stack object, but my point was without your patch, this already works:

v_writelane_b32 v0, s98, 63
v_writelane_b32 v1, s99, 0
v_writelane_b32 v1, s100, 1

So that means this test should include with and without the stack object. We also need to solve the mystery of why this stack object is needed

Harbormaster completed remote builds in B61602: Diff 273119.Jun 24 2020, 2:07 PM
kerbowa added inline comments.Jun 24 2020, 11:26 PM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
11

Without the stack object FuncInfo->VGPRReservedForSGPRSpill is 0.

kerbowa added inline comments.Jun 24 2020, 11:40 PM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
11

No VGPR is pre-reserved when there are no stack objects.
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AMDGPU/SIISelLowering.cpp#L11283

This patch will fix some current Navi issues as well where it's more common to run out of lanes in the pre-reserved register.

kerbowa added a reviewer: kerbowa.Jun 24 2020, 11:40 PM
saiislam updated this revision to Diff 273353.Jun 25 2020, 7:34 AM

Updated the test case to check for spilling in the second vgpr with and without stack object present, once all lanes of first vgpr has been used.

Harbormaster failed remote builds in B61726: Diff 273353!Jun 25 2020, 9:07 AM
kerbowa added inline comments.Jun 26 2020, 8:58 AM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
31

I think you need "store volatile i32 0, i32 addrspace(5)* %alloca" as well or there is no stack object.

saiislam updated this revision to Diff 273760.Jun 26 2020, 9:25 AM

Corrected the test case

Harbormaster failed remote builds in B61950: Diff 273760!Jun 26 2020, 10:55 AM
kerbowa accepted this revision.Jun 29 2020, 2:08 PM

LGTM

This revision is now accepted and ready to land.Jun 29 2020, 2:08 PM
arsenm accepted this revision.Jun 29 2020, 2:19 PM
arsenm added inline comments.
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
11

This doesn't really make sense though; why would this be a factor in the reservation logic?

saiislam marked an inline comment as done.Jun 30 2020, 7:08 AM
saiislam added inline comments.
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
31

With this patch applied over recent trunk, I am seeing following behavior:

  • without volatile in store instruction, correct behavior (using next vgpr for spilling). stack object instruction gets optimized out
  • with volatile in store instruction, spills are still getting overwritten
  • With -O0, with and without volatile spills are getting overwritten.

So, whenever stack object is present, wrong behavior is getting triggered somehow.

arsenm added inline comments.Jun 30 2020, 9:40 AM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
31

Did you mean without the patch?

saiislam marked an inline comment as done.Jun 30 2020, 9:49 AM
saiislam added inline comments.
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
31

No, I mean with the patch. Something changed in last 3-4 days.
Earlier, this patch used to work correctly for volatile store instruction as well.

kerbowa added inline comments.Jun 30 2020, 5:21 PM
llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll
31

It's because you reversed the logic. It should be 'NumVGPRSpillLanes < WaveSize'.

saiislam updated this revision to Diff 274684.Jun 30 2020, 10:10 PM

Corrected condition on NumVGPRSpillLanes and updated the test case.

kerbowa accepted this revision.Jun 30 2020, 10:12 PM
Harbormaster failed remote builds in B62451: Diff 274684!Jun 30 2020, 11:15 PM
Closed by commit rG918231639558: [AMDGPU] Spill more than wavesize CSR SGPRs (authored by saiislam). · Explain WhyJul 1 2020, 1:02 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
11 lines
test/
CodeGen/
AMDGPU/
43 lines

Diff 273760

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

Show First 20 Lines • Show All 297 Lines • ▼ Show 20 Linesbool SIMachineFunctionInfo::allocateSGPRSpillToVGPR(MachineFunction &MF,
const MCPhysReg *CSRegs = MRI.getCalleeSavedRegs(); const MCPhysReg *CSRegs = MRI.getCalleeSavedRegs();
// Make sure to handle the case where a wide SGPR spill may span between two // Make sure to handle the case where a wide SGPR spill may span between two
// VGPRs. // VGPRs.
for (unsigned I = 0; I < NumLanes; ++I, ++NumVGPRSpillLanes) { for (unsigned I = 0; I < NumLanes; ++I, ++NumVGPRSpillLanes) {
Register LaneVGPR; Register LaneVGPR;
unsigned VGPRIndex = (NumVGPRSpillLanes % WaveSize); unsigned VGPRIndex = (NumVGPRSpillLanes % WaveSize);
if (VGPRIndex == 0 && !FuncInfo->VGPRReservedForSGPRSpill) { // Reserve a VGPR (when NumVGPRSpillLanes = 0, WaveSize, 2*WaveSize, ..) and
// when one of the two conditions is true:
// 1. One reserved VGPR being tracked by VGPRReservedForSGPRSpill is not yet
// reserved.
// 2. All spill lanes of reserved VGPR(s) are full and another spill lane is
// required.
if (FuncInfo->VGPRReservedForSGPRSpill && NumVGPRSpillLanes <= WaveSize) {
arsenmUnsubmitted
Not Done
ReplyInline Actions

I think it might be clearer to split out the logic for the special reserved case.

Could you do something like

if (FuncInfo->VGPRReservedForSGPRSpill && NumVGPRSpillLanes <= WaveSize) {
  assert(VGPRResevedForSGPRSpill == SpillVGPRs.back().VGPR);
  LaneVGPR = VGPRResevedForSGPRSpill;
} else (VGPRIndex == 0) {
 ....
} else {
  LaneVGPR = SpillVGPRs.back().VGPR
}
arsenm: I think it might be clearer to split out the logic for the special reserved case. Could you do…
assert(FuncInfo->VGPRReservedForSGPRSpill == SpillVGPRs.back().VGPR);
LaneVGPR = FuncInfo->VGPRReservedForSGPRSpill;
} else if (VGPRIndex == 0) {
LaneVGPR = TRI->findUnusedRegister(MRI, &AMDGPU::VGPR_32RegClass, MF); LaneVGPR = TRI->findUnusedRegister(MRI, &AMDGPU::VGPR_32RegClass, MF);
if (LaneVGPR == AMDGPU::NoRegister) { if (LaneVGPR == AMDGPU::NoRegister) {
// We have no VGPRs left for spilling SGPRs. Reset because we will not // We have no VGPRs left for spilling SGPRs. Reset because we will not
// partially spill the SGPR to VGPRs. // partially spill the SGPR to VGPRs.
SGPRToVGPRSpills.erase(FI); SGPRToVGPRSpills.erase(FI);
NumVGPRSpillLanes -= I; NumVGPRSpillLanes -= I;
return false; return false;
} }
▲ Show 20 Lines • Show All 266 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/spill_more_than_wavesize_csr_sgprs.ll

  • This file was added.
; RUN: llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx803 -verify-machineinstrs < %s | FileCheck -enable-var-scope %s
arsenmUnsubmitted
Not Done
ReplyInline Actions

Maybe also try with and without the reserved VGPR flag?

arsenm: Maybe also try with and without the reserved VGPR flag?
; CHECK-LABEL: {{^}}spill_more_than_wavesize_csr_sgprs:
; CHECK: v_writelane_b32 v0, s98, 63
; CHECK-NEXT: v_writelane_b32 v1, s99, 0
; CHECK: v_readlane_b32 s99, v1, 0
; CHECK-NEXT: v_readlane_b32 s98, v0, 63
define void @spill_more_than_wavesize_csr_sgprs() {
call void asm sideeffect "",
"~{s35},~{s36},~{s37},~{s38},~{s39},~{s40},~{s41},~{s42}
arsenmUnsubmitted
Not Done
ReplyInline Actions

You removed the stack object, but my point was without your patch, this already works:

v_writelane_b32 v0, s98, 63
v_writelane_b32 v1, s99, 0
v_writelane_b32 v1, s100, 1

So that means this test should include with and without the stack object. We also need to solve the mystery of why this stack object is needed

arsenm: You removed the stack object, but my point was without your patch, this already works…
kerbowaUnsubmitted
Not Done
ReplyInline Actions

Without the stack object FuncInfo->VGPRReservedForSGPRSpill is 0.

kerbowa: Without the stack object FuncInfo->VGPRReservedForSGPRSpill is 0.
kerbowaUnsubmitted
Not Done
ReplyInline Actions

No VGPR is pre-reserved when there are no stack objects.
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AMDGPU/SIISelLowering.cpp#L11283

This patch will fix some current Navi issues as well where it's more common to run out of lanes in the pre-reserved register.

kerbowa: No VGPR is pre-reserved when there are no stack objects. https://github.com/llvm/llvm…
arsenmUnsubmitted
Not Done
ReplyInline Actions

This doesn't really make sense though; why would this be a factor in the reservation logic?

arsenm: This doesn't really make sense though; why would this be a factor in the reservation logic?
,~{s43},~{s44},~{s45},~{s46},~{s47},~{s48},~{s49},~{s50}
arsenmUnsubmitted
Not Done
ReplyInline Actions

I'm pretty sure this isn't broken if you do not have a stack object, but these should be unrelated so I think something is still wrong here

arsenm: I'm pretty sure this isn't broken if you do not have a stack object, but these should be…
,~{s51},~{s52},~{s53},~{s54},~{s55},~{s56},~{s57},~{s58}
,~{s59},~{s60},~{s61},~{s62},~{s63},~{s64},~{s65},~{s66}
,~{s67},~{s68},~{s69},~{s70},~{s71},~{s72},~{s73},~{s74}
,~{s75},~{s76},~{s77},~{s78},~{s79},~{s80},~{s81},~{s82}
,~{s83},~{s84},~{s85},~{s86},~{s87},~{s88},~{s89},~{s90}
,~{s91},~{s92},~{s93},~{s94},~{s95},~{s96},~{s97},~{s98}
,~{s99},~{s100},~{s101},~{s102}"()
ret void
}
; CHECK-LABEL: {{^}}spill_more_than_wavesize_csr_sgprs_with_stack_object:
; CHECK: v_writelane_b32 v0, s98, 63
; CHECK-NEXT: v_writelane_b32 v1, s99, 0
; CHECK: v_readlane_b32 s99, v1, 0
; CHECK-NEXT: v_readlane_b32 s98, v0, 63
define void @spill_more_than_wavesize_csr_sgprs_with_stack_object() {
%alloca = alloca i32, align 4, addrspace(5)
store volatile i32 0, i32 addrspace(5)* %alloca
kerbowaUnsubmitted
Not Done
ReplyInline Actions

I think you need "store volatile i32 0, i32 addrspace(5)* %alloca" as well or there is no stack object.

kerbowa: I think you need "store volatile i32 0, i32 addrspace(5)* %alloca" as well or there is no stack…
saiislamAuthorUnsubmitted
Done
ReplyInline Actions

With this patch applied over recent trunk, I am seeing following behavior:

  • without volatile in store instruction, correct behavior (using next vgpr for spilling). stack object instruction gets optimized out
  • with volatile in store instruction, spills are still getting overwritten
  • With -O0, with and without volatile spills are getting overwritten.

So, whenever stack object is present, wrong behavior is getting triggered somehow.

saiislam: With this patch applied over recent trunk, I am seeing following behavior: - without…
arsenmUnsubmitted
Not Done
ReplyInline Actions

Did you mean without the patch?

arsenm: Did you mean without the patch?
saiislamAuthorUnsubmitted
Done
ReplyInline Actions

No, I mean with the patch. Something changed in last 3-4 days.
Earlier, this patch used to work correctly for volatile store instruction as well.

saiislam: No, I mean with the patch. Something changed in last 3-4 days. Earlier, this patch used to work…
kerbowaUnsubmitted
Not Done
ReplyInline Actions

It's because you reversed the logic. It should be 'NumVGPRSpillLanes < WaveSize'.

kerbowa: It's because you reversed the logic. It should be 'NumVGPRSpillLanes < WaveSize'.
call void asm sideeffect "",
"~{s35},~{s36},~{s37},~{s38},~{s39},~{s40},~{s41},~{s42}
,~{s43},~{s44},~{s45},~{s46},~{s47},~{s48},~{s49},~{s50}
,~{s51},~{s52},~{s53},~{s54},~{s55},~{s56},~{s57},~{s58}
,~{s59},~{s60},~{s61},~{s62},~{s63},~{s64},~{s65},~{s66}
,~{s67},~{s68},~{s69},~{s70},~{s71},~{s72},~{s73},~{s74}
,~{s75},~{s76},~{s77},~{s78},~{s79},~{s80},~{s81},~{s82}
,~{s83},~{s84},~{s85},~{s86},~{s87},~{s88},~{s89},~{s90}
,~{s91},~{s92},~{s93},~{s94},~{s95},~{s96},~{s97},~{s98}
,~{s99},~{s100},~{s101},~{s102}"()
ret void
}