[AMDGPU] Add test for GCNRegPressure tracker bug #73786

piotrAMD · 2023-11-29T11:57:21Z

Add a test to document an existing problem in GCNRegPressure tracker.

The upward tracker does not count the registers used (16 of them) in movrel instruction (for example V_INDIRECT_REG_WRITE_MOVREL_B32_V16).

The downward tracker counts the registers but reports a mismatch: %0:L0000000000000C00 isn't found in LIS reported set

Add a test to document an existing problem in GCNRegPressure tracker. The upward tracker does not count the registers used (16 of them) in movrel instruction (for example V_INDIRECT_REG_WRITE_MOVREL_B32_V16). The downward tracker counts the registers but reports a mismatch: %0:L0000000000000C00 isn't found in LIS reported set

llvmbot · 2023-11-29T11:57:52Z

@llvm/pr-subscribers-backend-amdgpu

Author: Piotr Sobczak (piotrAMD)

Changes

Add a test to document an existing problem in GCNRegPressure tracker.

The upward tracker does not count the registers used (16 of them) in movrel instruction (for example V_INDIRECT_REG_WRITE_MOVREL_B32_V16).

The downward tracker counts the registers but reports a mismatch: %0:L0000000000000C00 isn't found in LIS reported set

Full diff: https://github.com/llvm/llvm-project/pull/73786.diff

1 Files Affected:

(modified) llvm/test/CodeGen/AMDGPU/regpressure_printer.mir (+123)

diff --git a/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir b/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
index 0020b13d736026e..b66172ecea67336 100644
--- a/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
+++ b/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
@@ -531,3 +531,126 @@ body:             |
     %1:vgpr_32 = V_MOV_B32_e32 %0, implicit $exec
     S_NOP 0, implicit %1
 ...
+---
+name: movrel
+tracksRegLiveness: true
+body: |
+  ; RPU-LABEL: name: movrel
+  ; RPU: bb.0:
+  ; RPU-NEXT:   Live-in:
+  ; RPU-NEXT:   SGPR  VGPR
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      $sgpr0 = COPY $sgpr1
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      $sgpr2_sgpr3 = S_GETPC_B64
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      $sgpr1 = COPY killed $sgpr3
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      $sgpr0_sgpr1_sgpr2_sgpr3 = S_LOAD_DWORDX4_IMM $sgpr0_sgpr1, 0, 0
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      $sgpr0 = S_BUFFER_LOAD_DWORD_IMM $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      undef %0.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      S_CMP_GT_U32 $sgpr0, 15, implicit-def $scc
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      S_CBRANCH_SCC1 %bb.2, implicit $scc
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      S_BRANCH %bb.1
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   Live-out:
+  ; RPU-NEXT: bb.1:
+  ; RPU-NEXT:   Live-in:
+  ; RPU-NEXT:   SGPR  VGPR
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     1      undef %0.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   0     1      $m0 = S_MOV_B32 killed $sgpr0
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   0     1      %0:vreg_512 = V_INDIRECT_REG_WRITE_MOVREL_B32_V16 %0:vreg_512(tied-def 0), 42, 3, implicit $m0, implicit $exec
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   Live-out: %0:0000000000000C00
+  ; RPU-NEXT: bb.2:
+  ; RPU-NEXT:   Live-in:  %0:0000000000000C00
+  ; RPU-NEXT:   SGPR  VGPR
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   0     1      %1:vgpr_32 = V_CVT_F32_UBYTE0_e64 %0.sub5:vreg_512, 0, 0, implicit $exec
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   0     1      EXP_DONE 0, %1:vgpr_32, undef %2:vgpr_32, undef %3:vgpr_32, undef %4:vgpr_32, -1, 0, 1, implicit $exec
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   0     0      S_ENDPGM 0
+  ; RPU-NEXT:   0     0
+  ; RPU-NEXT:   Live-out:
+  ;
+  ; RPD-LABEL: name: movrel
+  ; RPD: bb.0:
+  ; RPD-NEXT:   Live-in:
+  ; RPD-NEXT:   SGPR  VGPR
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      $sgpr0 = COPY $sgpr1
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      $sgpr2_sgpr3 = S_GETPC_B64
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      $sgpr1 = COPY killed $sgpr3
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      $sgpr0_sgpr1_sgpr2_sgpr3 = S_LOAD_DWORDX4_IMM $sgpr0_sgpr1, 0, 0
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      $sgpr0 = S_BUFFER_LOAD_DWORD_IMM $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     1      undef %0.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     1      S_CMP_GT_U32 $sgpr0, 15, implicit-def $scc
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     1      S_CBRANCH_SCC1 %bb.2, implicit $scc
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     1      S_BRANCH %bb.1
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   Live-out: %0:0000000000000C00
+  ; RPD-NEXT:   mis LIS:
+  ; RPD-NEXT:     %0:L0000000000000C00 isn't found in LIS reported set
+  ; RPD-NEXT: bb.1:
+  ; RPD-NEXT:   Live-in:
+  ; RPD-NEXT:   SGPR  VGPR
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     1      undef %0.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     1      $m0 = S_MOV_B32 killed $sgpr0
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     16     %0:vreg_512 = V_INDIRECT_REG_WRITE_MOVREL_B32_V16 %0:vreg_512(tied-def 0), 42, 3, implicit $m0, implicit $exec
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   Live-out: %0:0000000000000C00
+  ; RPD-NEXT: bb.2:
+  ; RPD-NEXT:   Live-in:  %0:0000000000000C00
+  ; RPD-NEXT:   SGPR  VGPR
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     2      %1:vgpr_32 = V_CVT_F32_UBYTE0_e64 %0.sub5:vreg_512, 0, 0, implicit $exec
+  ; RPD-NEXT:   0     1
+  ; RPD-NEXT:   0     1      EXP_DONE 0, %1:vgpr_32, undef %2:vgpr_32, undef %3:vgpr_32, undef %4:vgpr_32, -1, 0, 1, implicit $exec
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   0     0      S_ENDPGM 0
+  ; RPD-NEXT:   0     0
+  ; RPD-NEXT:   Live-out:
+  bb.0:
+    liveins: $sgpr1
+    $sgpr0 = COPY $sgpr1
+    $sgpr2_sgpr3 = S_GETPC_B64
+    $sgpr1 = COPY killed $sgpr3
+    $sgpr0_sgpr1_sgpr2_sgpr3 = S_LOAD_DWORDX4_IMM killed $sgpr0_sgpr1, 0, 0
+    $sgpr0 = S_BUFFER_LOAD_DWORD_IMM killed $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0
+    undef %47.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+    S_CMP_GT_U32 $sgpr0, 15, implicit-def $scc
+    S_CBRANCH_SCC1 %bb.2, implicit $scc
+    S_BRANCH %bb.1
+
+  bb.1:
+    liveins: $sgpr0
+    undef %47.sub5:vreg_512 = V_MOV_B32_e32 5, implicit $exec
+    $m0 = S_MOV_B32 killed $sgpr0
+    %47:vreg_512 = V_INDIRECT_REG_WRITE_MOVREL_B32_V16 %47:vreg_512, 42, 3, implicit $m0, implicit $exec
+
+  bb.2:
+
+    %49:vgpr_32 = V_CVT_F32_UBYTE0_e64 %47.sub5:vreg_512, 0, 0, implicit $exec
+    EXP_DONE 0, %49:vgpr_32, undef %51:vgpr_32, undef %53:vgpr_32, undef %55:vgpr_32, -1, 0, 1, implicit $exec
+    S_ENDPGM 0
+...

vpykhtin · 2023-11-29T12:15:27Z

llvm/test/CodeGen/AMDGPU/regpressure_printer.mir

+  ; RPU-NEXT:   Live-in:  %0:0000000000000C00
+  ; RPU-NEXT:   SGPR  VGPR
+  ; RPU-NEXT:   0     1
+  ; RPU-NEXT:   0     1      %1:vgpr_32 = V_CVT_F32_UBYTE0_e64 %0.sub5:vreg_512, 0, 0, implicit $exec


We need to agree on how we count such cases.

The problem is that %0 is fully defined by V_INDIRECT_REG_WRITE_MOVREL_B32_V16 but only sub5 of it is used. In general this means that regalloc need to allocate full vreg_512 anyway but the unused lanes can be allocated for other needs though this is not the case here.

This makes tracking more complicated if we start model what regalloc would do. The conservative approach can be to ignore lanes at all after the GCNRewritePartialRegUses pass is enabled because after this pass is guaranteed we have only fully defined or used registers.

I wonder what others think, but for me what RPD reports seems intuitively correct - the RP gets the increment from the movrel just for the instruction (and that should contribute to the max pressure).

Unless I misunderstood the suggestion I do not think this is related to GCNRewritePartialRegUses. The movrel instruction is kind of special because of the indirection. It can't just operate on %0.sub5.

RPD is accounting for the whole vreg_512 "at" the instruction level though. If some of the lanes are reused after the instruction they should be already decremented from the pressure. It looks like RPD approach is more correct here.

Unless I misunderstood the suggestion I do not think this is related to GCNRewritePartialRegUses.

GCNRewritePartialRegUses is irrelevant to this case indeed, I just thought about a conservative way of register pressure accounting when we account for the whole reg always, but we can do better than that as RPD does.

The movrel instruction is kind of special because of the indirection. It can't just operate on %0.sub5.

Sorry I don't really know how it works but I believe
%0:vreg_512 = V_INDIRECT_REG_WRITE_MOVREL_B32_V16 %0:vreg_512(tied-def 0)
models correctly what is does, that is it fully defines %0:vreg_512 on output, right?

@piotrAMD are you going to fix this?

I agree with all of this - what RPD is doing seems correct.

@piotrAMD are you going to fix this?

Yes, happy to work on the fix when I am done with the task I am currently working on.

vpykhtin

LGTM. BTW I'm a bit confused - I don't see live-through sets in the output from the #71096

piotrAMD · 2023-11-30T11:20:17Z

Rebased and added missing live-throughs.

piotrAMD requested a review from vpykhtin November 29, 2023 11:57

llvmbot added the backend:AMDGPU label Nov 29, 2023

piotrAMD requested review from jayfoad, perlfu and dstutt November 29, 2023 11:57

vpykhtin reviewed Nov 29, 2023

View reviewed changes

vpykhtin approved these changes Nov 29, 2023

View reviewed changes

piotrAMD added 2 commits November 30, 2023 12:07

Merge remote-tracking branch 'origin/main'

c294259

Add missing live-throughs

10391e0

piotrAMD merged commit 73d9f5f into llvm:main Nov 30, 2023

This was referenced Dec 4, 2023

[AMDGPU] Fix GCUpwardRPTracker. #74328

Closed

[AMDGPU] Fix GCNUpwardRPTracker: max register pressure on defs. #74422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU] Add test for GCNRegPressure tracker bug #73786

[AMDGPU] Add test for GCNRegPressure tracker bug #73786

Uh oh!

piotrAMD commented Nov 29, 2023

Uh oh!

llvmbot commented Nov 29, 2023

Uh oh!

vpykhtin Nov 29, 2023 •

edited

Loading

Uh oh!

piotrAMD Nov 29, 2023

Uh oh!

vpykhtin Nov 29, 2023

Uh oh!

vpykhtin Nov 29, 2023

Uh oh!

vpykhtin Nov 29, 2023

Uh oh!

jayfoad Nov 29, 2023

Uh oh!

piotrAMD Nov 29, 2023

Uh oh!

vpykhtin left a comment

Uh oh!

piotrAMD commented Nov 30, 2023

Uh oh!

Uh oh!

[AMDGPU] Add test for GCNRegPressure tracker bug #73786

[AMDGPU] Add test for GCNRegPressure tracker bug #73786

Uh oh!

Conversation

piotrAMD commented Nov 29, 2023

Uh oh!

llvmbot commented Nov 29, 2023

Uh oh!

vpykhtin Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piotrAMD Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vpykhtin Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vpykhtin Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vpykhtin Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

jayfoad Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

piotrAMD Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vpykhtin left a comment

Choose a reason for hiding this comment

Uh oh!

piotrAMD commented Nov 30, 2023

Uh oh!

Uh oh!

vpykhtin Nov 29, 2023 •

edited

Loading