feat(rocjitsu): VGPR/SGPR Register Spilling for CDNA4/RDNA4#8888
feat(rocjitsu): VGPR/SGPR Register Spilling for CDNA4/RDNA4#8888theSK2005 wants to merge 4 commits into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
3169098 to
490d75e
Compare
There was a problem hiding this comment.
Good start. Could you clarify what you planned to accomplish with this PR and what you see as deferred? What should users be able to expect from it? The AccVGPRs is clearly deferred, as is multi-kernel instrumentation. Is there anything else you can think of?
How do you see spilling to dead registers fitting in here?
Additionally, I don't mind keeping non-RDNA4/CDNA4 untested, but DBI should be pretty architecture agnostic. There are only a few places where you had to add ISA-specific stuff, so let's update those for all supported ISAs.
Couple tests that the agent couldn't find and that I think would be good to have:
- A check that a multi-kernel instrumentation fails
- You have separate VGPR and SGPR spilling tests. I'd be interested to one that does both and make sure those play well together.
- I'd also make sure spilling multiple SGPRs plays nicely too.
Please update the dbi design documentation too.
| case ROCJITSU_CODE_ARCH_RDNA3_5: | ||
| case ROCJITSU_CODE_ARCH_RDNA4: | ||
| return 8; | ||
| default: |
There was a problem hiding this comment.
Let's fail on default so that we know to update this when adding a new ISA.
| /// discoverable `.kd` descriptor) and a probe ELF exporting one function | ||
| /// symbol, plus small readback helpers. | ||
| /// | ||
| /// These mirror the in-file helpers in tests/patch/instrumentor_test.cpp (which |
There was a problem hiding this comment.
Since you've already creating a header that can be shared and it copies from tests/patch/instrumentor_test.cpp, please finish the refactor and have it use the new header. This will require cleaning this file up a bit more though. For example, the constants should probably reference gfx950 if they are ISA-specific.
b23a065 to
cd5cc47
Compare
Implements SGPR/VGPR register spilling for CDNA4/RDNA4 to enable non-nop probes to be run.
cd5cc47 to
daf2bf1
Compare
|
🎉 All checks passed! This PR is ready for review. |
…estore states after epilogue, will set EXEC to anchor value at probe start in future commit
| return true; | ||
| } | ||
|
|
||
| bool CodeObjectPatcher::set_private_segment_fixed_size(uint64_t descriptor_file_offset, |
There was a problem hiding this comment.
Do we not already have this? What is DBT doing for this?
| return build_sopc_encoding(arch, sopc_op_cmp_lg_u32(arch), ssrc0, ssrc1); | ||
| } | ||
|
|
||
| // DBI register-spilling primitives (CDNA4 and RDNA4 only). These are the |
There was a problem hiding this comment.
I'm not sure if we should be implement architecture specific filling details here. DBT uses architecture specific files which do this. It might be worth looking at that and maybe sharing the building stuff there. I would expect that there should be a single api with arch specific implementations for these things. instruction_builder.h/.cpp was supposed to be for simple things.
| // Find the allocated section whose address range covers @p kd_vaddr and decode | ||
| // the descriptor out of that section's own bytes (no ELF re-walk). Returns | ||
| // nullopt when no section covers the descriptor or it does not fully fit. | ||
| [[nodiscard]] std::optional<LocatedKernelDescriptor> |
There was a problem hiding this comment.
What does DBT do for this? We should reuse code between DBT/DBI please. I'm sure DBT knows how to locate kernel descriptors.
|
|
||
| namespace rocjitsu { | ||
|
|
||
| /// @brief One kernel descriptor discovered in a code object, with the coordinates |
There was a problem hiding this comment.
I doubt we need all this. DBT gets all this information already, we shouldn't duplicate it and reuse it.
| /// @brief VGPR-count encoding granule for @p arch. Descriptor VGPR count = | ||
| /// (granulated_workitem_vgpr_count + 1) * granule. Matches LLVM's | ||
| /// AMDGPUBaseInfo::getVGPREncodingGranule(). | ||
| inline constexpr uint32_t vgpr_encoding_granule(rj_code_arch_t arch) { |
There was a problem hiding this comment.
This shouldn't like in amdgpu_elf right?
|
Please try to reuse as much as possible between DBT/DBI. We don't want to have two seperate implementations of these things. |
3 participants
Motivation
Register spilling for arbitrary probes requires being able to spill registers that are live at the anchor if the probe would clobber them. SGPRs are spilled into scratch using v_writelane/v_readlane into a bridge VGPR. Relies on SpillManager changes made in #8406.
Test Plan
Test Result
All tests pass
Submission Checklist