Repository navigation
fix(shader): print memory instructions' pc and offset in the IR dump - #9907
Conversation
The APS5_DUMP_IR dump showed every scalar load's immediate offset as 0. An SMEM load keeps its immediate in the instruction's memory metadata: MemoryFlags indexes Resources().memoryInfo, whose offset holds the immediate plus 4 per component, while the soffset argument is the constant 0 when the instruction has none. Buffer, address and shared memory instructions keep their offsets there too. ProgramToString printed the arguments, the type and the use count, but not the flags or the metadata, so a dump could not tell 0x54 from 0x6c. ProgramToString now appends " [pc=0x... off=0x...]" to every instruction that carries memory metadata, and " [pc=0x... mem=?]" when its index is past memoryInfo, since dumps run before validation. The opcode set is one predicate, carriesMemoryInfo, which ValidateProgram now also uses for its memory-info index check; the validator accepts and rejects the same programs with the same messages. boykopovar#9633 (comment) Related: boykopovar#9749.
|
Reviewed the diff and test case. The new |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 2cbaa96390ccba25e16a17efdf4e5b49ffdea59e. The formatter adds metadata only to memory-bearing instructions and preserves other lines; tests cover SMEM, vector buffer offsets, split DS offsets, invalid metadata and non-memory text. The refactored predicate is also used by validation. Current-main whitespace and conventions pass, and the branch merges cleanly; only the AGC CMake registration file changed on both sides. Author-reported Windows/Linux tests, validation-layer logs and the full dump exercise are useful; product jobs remain skipped by workflow policy. I did not rerun C++ tests locally. One description correction: because carriesMemoryInfo includes image operations, malformed image instructions with an out-of-range metadata index now receive the generic bounds error before the former image-kind lookup (which previously indexed without a bounds check). That is a safer invalid-input diagnostic, but is a narrow validation behavior change, so please qualify the claim that validation messages are unchanged and consider adding an image-op missing-metadata case.
What
APS5_DUMP_IRprinted every scalar load's immediate offset as 0. On #9633 that hid the difference between the two words of a pointer chase (0x54 against 0x6c), and the dump couldn't show what decided the refusal (oneandonlydean's comment).Why:
MemoryFlagsindexesResources().memoryInfo, whoseoffsetholds the immediate plus 4 per component (sLoad,ScalarBufferInstructions.cpp).soffsetargument is the constant 0 when the instruction has none.ProgramToStringprinted each instruction's arguments, type and use count, but not its flags or metadata.The change:
ProgramToStringnow appends[pc=0x… off=0x…]to every instruction that carries memory metadata. It prints values, not the metadata index.[pc=0x… mem=?]when the index is pastmemoryInfo, since dumps run before validation.carriesMemoryInfo.ValidateProgramnow uses it for its memory-info index check, which each of its five memory blocks used to repeat. The validator accepts and rejects the same programs with the same messages.off2: translation splits the DS pair forms into two shared ops, each with its own offset. So the second offset shows as a second line, and no IR opcode usessecondaryOffset.For
s_load_dword s4, s[0:1], 0x54ands_buffer_load_dwordx4 s[12:15], s[8:11], 0x60, before:After:
Users of the dump text:
git grepover the tree,tools/included, finds only:APS5_DUMP_IR's own printer;ENVIRONMENT.md's entry for the variable;ShaderMemory.cpp's frontend-pair check (reused == fresh) already compares every instruction's flags and the programs'memoryInfo, so the printed values can't newly differ there.Overlap:
git merge-treeis clean against #8939, #9457, #9633, #9749 and #9790, and against #9198, #6612, #7410 and #8881, which touchIrProgram.cpporIrOpcode.hpp. Of the 47 PRs the overlap bot couldn't compare incore/libs/prx/libSceAgcDriver/CMakeLists.txt,git merge-treewith this head shows a conflict at this PR'sir_dump_memoryregistration only for #6044 (TaymKal), which converts the file's registrations toadd_library_test. The order is proposed in its thread, as for our other PRs there (comment): this PR first; if #6044 lands first,ir_dump_memoryis registered withadd_library_testat this PR's next merge ofmain.Tested
ir_dump_memory(core/libs/tests/IrDumpMemory.cpp):s_load_dword … 0x54,s_buffer_load_dwordx4 … 0x60andds_read2_b32 … offset0:3 offset1:9. Each line must end with its pc and offset: 0x54; 0x60 to 0x6c; 0xc and 0x24. A non-memory line must be unchanged.memoryInfocleared, the dump showsmem=?andValidateProgramthrows its usualLoadAddressU32 has an invalid memory-info index.IrProgram.cppatmain, it fails 10/10 (expected "… true (U32; uses=1)" to end with " [pc=0x00012de0 off=0x54]"). Green: passes 10/10.agc_shader_frontend_pair,agc_shader_frontend_pair_off,agc_shader_memory,scalar_call,scalar_swappcandagc_validate_programpass.ir_dump_memoryfails withmain'sIrProgram.cppin all 3 runs and passes 10 times with the change.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationandANYPS5_NO_SHADER_CACHE=1on llvmpipe (LLVM 20.1.2), GitHub's Linux runner. The run includes two real-device tests,agc_driver_image_table_vouched_lookupsandagc_driver_msaa_dcc_clear_4.VUID-VkDeviceCreateInfo-pNext-pNext, so the change adds noVUID-message. It changes no generated SPIR-V.APS5_DUMP_IR=allonagc_shader_memory_tests: 18,655 instructions carry[pc=… off=…], and none showsmem=?.FormatVersion: unchanged. The dump isn't serialized.check_conventions.py: passes, with no comment lines added.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)APS5_DUMP_IRnow shows memory offsets)