Repository navigation
[AMDGPU][Scheduler] Restore local spill rejection in occupancy mode - #233979
Conversation
When rematerializing for higher occupancy, the scheduler can retain a schedule that introduces spills even when the saved order is spill-free. The `!TargetOcc` guard disables local spill rejection. Since occupancy is tracked using `max(WavesBefore, WavesAfter)`, global finalization can still consider the target reached even when the trial schedule is worse. The guard was introduced in 7cc7a94 (llvm#180646) alongside a fix for rollback using temporary `DBG_VALUE` placeholders. dc34d16 (llvm#192443) replaced that representation with the shared MIR rematerializer, removing the placeholder-specific slot-index issue. Re-enable the existing `mayCauseSpilling` check in occupancy mode to reject such trials and restore the instruction order saved before rescheduling, while preserving rematerialization. Global finalization still handles rematerialization rollback. Add a MIR regression that spills before the fix and remains spill-free afterward. Related Q2_K spilling discussion: ggml-org/llama.cpp#29910
|
Hello @leeliu103 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-backend-amdgpu Author: leeliu103 ChangesWhen rematerializing for higher occupancy, the scheduler can retain a schedule that introduces spills even when the saved order is spill-free. The The guard was introduced in 7cc7a94 (#180646) alongside a fix for rollback using temporary Re-enable the existing Related Q2_K spilling discussion: Full diff: https://github.com/llvm/llvm-project/pull/233979.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
index 057c72d962dc1..83d5df9860e4a 100644
--- a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
@@ -2245,9 +2245,7 @@ bool ClusteredLowOccStage::shouldRevertScheduling(unsigned WavesAfter) {
}
bool PreRARematStage::shouldRevertScheduling(unsigned WavesAfter) {
- // When trying to increase occupancy (TargetOcc == true) the stage manages
- // region reverts globally (all or none), so we always return false here.
- return !TargetOcc && mayCauseSpilling(WavesAfter);
+ return mayCauseSpilling(WavesAfter);
}
bool ILPInitialScheduleStage::shouldRevertScheduling(unsigned WavesAfter) {
diff --git a/llvm/test/CodeGen/AMDGPU/machine-scheduler-sink-trivial-remats-spills.mir b/llvm/test/CodeGen/AMDGPU/machine-scheduler-sink-trivial-remats-spills.mir
new file mode 100644
index 0000000000000..c7a7d01de0508
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/machine-scheduler-sink-trivial-remats-spills.mir
@@ -0,0 +1,44 @@
+# RUN: llc -mtriple=amdgpu9.00-amd-amdhsa -start-before=machine-scheduler -stop-after=greedy,2 -verify-machineinstrs -o - %s | FileCheck %s
+
+# Check that rematerialization for higher occupancy does not introduce spills.
+
+# CHECK-LABEL: name: remat_occupancy_no_spill
+# CHECK-NOT: SI_SPILL_
+# CHECK: S_ENDPGM
+
+--- |
+ define amdgpu_kernel void @remat_occupancy_no_spill() #0 {
+ ret void
+ }
+
+ attributes #0 = { "amdgpu-waves-per-eu"="9,10" "amdgpu-num-vgpr"="25" }
+...
+---
+name: remat_occupancy_no_spill
+tracksRegLiveness: true
+machineFunctionInfo:
+ isEntryFunction: true
+ stackPtrOffsetReg: '$sgpr32'
+body: |
+ bb.0:
+ successors: %bb.1
+
+ S_NOP 0, implicit-def %live16:vreg_512, implicit-def %live5:vreg_160
+ %loaded:vreg_64 = DS_READ2_B32_gfx9 %live5.sub0, 0, 1, 0, implicit $exec :: (load (s64), align 4, addrspace 3)
+ %address:vgpr_32 = V_ADD_U32_e64 16, %live5.sub0, 0, implicit $exec
+
+ bb.1:
+ successors: %bb.2
+
+ %converted0:vgpr_32 = nofpexcept V_CVT_F32_I32_e32 %loaded.sub0, implicit $exec, implicit $mode
+ DS_WRITE_B32_gfx9 %live5.sub0, %converted0, 8, 0, implicit $exec :: (store (s32), addrspace 3)
+
+ %converted1:vgpr_32 = nofpexcept V_CVT_F32_I32_e32 %loaded.sub1, implicit $exec, implicit $mode
+ DS_WRITE_B32_gfx9 %live5.sub0, %converted1, 12, 0, implicit $exec :: (store (s32), addrspace 3)
+ DS_WRITE_B32_gfx9 %address, %loaded.sub0, 0, 0, implicit $exec :: (store (s32), addrspace 3)
+ DS_WRITE_B32_gfx9 %address, %loaded.sub1, 4, 0, implicit $exec :: (store (s32), addrspace 3)
+
+ bb.2:
+ S_NOP 0, implicit %live16, implicit %live5, implicit %loaded
+ S_ENDPGM 0
+...
|
|
|
| @@ -0,0 +1,44 @@ | |||
| # RUN: llc -mtriple=amdgpu9.00-amd-amdhsa -start-before=machine-scheduler -stop-after=greedy,2 -verify-machineinstrs -o - %s | FileCheck %s | |||
There was a problem hiding this comment.
| # RUN: llc -mtriple=amdgpu9.00-amd-amdhsa -start-before=machine-scheduler -stop-after=greedy,2 -verify-machineinstrs -o - %s | FileCheck %s | |
| # RUN: llc -mtriple=amdgpu9.00-amd-amdhsa -start-before=machine-scheduler -stop-after=greedy,2 -o - %s | FileCheck %s |
| # Check that rematerialization for higher occupancy does not introduce spills. | ||
|
|
||
| # CHECK-LABEL: name: remat_occupancy_no_spill | ||
| # CHECK-NOT: SI_SPILL_ |
There was a problem hiding this comment.
Don't use -NOT checks, check what is emitted
| ret void | ||
| } | ||
|
|
||
| attributes #0 = { "amdgpu-waves-per-eu"="9,10" "amdgpu-num-vgpr"="25" } |
There was a problem hiding this comment.
Don't introduce new uses of amdgpu-num-vgpr, it's in the middle of removal
There was a problem hiding this comment.
I’ve addressed all three comments on the regression test. Could you take another look at the test changes, including the expectation updates in machine-scheduler-sink-trivial-remats.mir and machine-scheduler-rematerialization-scoring.mir?
Use positive output checks and remove -verify-machineinstrs. Adjust the input to preserve the regression without amdgpu-num-vgpr.
|
@leeliu103 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
ROCm 10.x spills far more than 7.2 on gfx906 (llama.cpp discussion #30622: +27% static VGPR spill stores, 47 kernels newly spilling, 104 losing occupancy). ROCm/llvm-project 5eaa4e7e0 (llvm/llvm-project#233979, Oct 23) re-enables local spill rejection in the occupancy-driven rematerialization stage. It applies to the pinned amd-llvm 2792f8ceeb5a (offset 7 lines), which already has the shared MIR rematerializer the fix depends on. No weekly amd-compiler branch includes it yet. - patches/amd-mainline/llvm-project/0001-...: the upstream commit, git am form - build-gfx906-docker.sh: apply new llvm patches to an existing amd-llvm tree (fetch_sources.py only patches on the first fetch) - GFX906.md: why, and when to drop it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BEGn7VjWvykNnNjpPFUwre
When rematerializing for higher occupancy, the scheduler can retain a schedule that introduces spills even when the saved order is spill-free. The
!TargetOccguard disables local spill rejection. Since occupancy is tracked usingmax(WavesBefore, WavesAfter), global finalization can still consider the target reached even when the trial schedule is worse.The guard was introduced in 7cc7a94 (#180646) alongside a fix for rollback using temporary
DBG_VALUEplaceholders. dc34d16 (#192443) replaced that representation with the shared MIR rematerializer, removing the placeholder-specific slot-index issue.Re-enable the existing
mayCauseSpillingcheck in occupancy mode to reject such trials and restore the instruction order saved before rescheduling, while preserving rematerialization. Global finalization still handles rematerialization rollback. Add a MIR regression that spills before the fix and remains spill-free afterward.Related Q2_K spilling discussion:
ggml-org/llama.cpp#29910