Repository navigation
feat(shader): translate s_swappc_b64 fetch-shader calls at vertex entry - #1217
Conversation
10b7368 to
a0931fd
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for this. Findings:
- CI is red on both jobs:
agc_shader_control_flowfails withs_swappc_b64 with a return address: expected a refusal with 'unsupported SOP1 opcode 33', got 'unclosable s_swappc_b64 call/return pairing ...'. That test pins the old behaviour; update it for the new one and rerun ctest (the Tested section only has a standalone Windows build, no ctest run). - The description doesn't match the diff: #754 is closed, not merged (
mainonly has thes_swappc_b64 null→s_setpc_b64alias,RdnaScalarOpDecoder.cpp:334), ands_call_b64/s_rfe_b64are said to be untouched, butRdnaScalarOpDecoder.cpp:43and:184change their decode to throw. That overlaps #895 (s_call_b64) and is not part of this topic; drop both lines and the matching test checks (ScalarSwappc.cpp:70-71) and TechnicalDebt line 311. docs/dev/TechnicalDebt.md:307duplicates the existing "Font writing lines" entry (also at 312); remove the stray one.- Tests pin IR shape, not values:
CheckLinkModelsNextInstructionAddresscompares two IR dumps, and nothing exercisestranslateExternalFetch(InstructionTranslator.cpp:105) or the inlined attribute loads. Add an execution/translation test that checks the fetched values. - Minor: the newline at the end of
GraphBuilder.hppandScalarSwappc.cppwas dropped.
For reference, on hardware (gfx1035) s_swappc_b64 sdst, ssrc writes the address of the next instruction (PC+4 for the 4-byte SOP1), which matches the pc + 4 model here. The fetch-shader inline itself I could not measure.
934bbc2 to
f2b3780
Compare
|
All four findings handled on
Description drift is the embarrassing class of these — the body now re-derives from the final diff instead of the intent. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the update; the earlier findings are addressed (test updated, s_call_b64/s_rfe_b64 decode changes dropped, duplicate TechnicalDebt line gone, body fixed). Build and ctest on the merge with main are green (431/431, including agc_shader_control_flow, agc_driver_scalar_swappc and scalar_swappc). What's left:
- No test pins values for the new translation path:
translateExternalFetch(InstructionTranslator.cpp:105, called at:505) and the inlined attribute loads are not exercised by anything.CheckLinkModelsNextInstructionAddress(ScalarSwappc.cpp:87) only compares two IR dumps, which pins IR shape. Add an execution or translation test that runs a vertex entry with a fetch-shader call and checks the fetched attribute values (includingdst_seland the 'more than four registers' throw). - Minor:
GraphBuilder.hppandScalarSwappc.cppstill lack the final newline.
Not measured by us: the fetch-shader inline itself (the pc + 4 link semantics of s_swappc_b64 match what we measured on gfx1035).
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
Prerequisite from boykopovar#1217. Preserve current main trap decoding and adapt its regression; include the updated call refusal expectation from f2b3780. Static call regions and the existing vertex fetch plan supply the bounded call/return model. Unsupported paths throw; see TechnicalDebt.md. (cherry picked from commit 49dcd66)
Prerequisite from boykopovar#1217. Preserve current main trap decoding and adapt its regression; include the updated call refusal expectation from f2b3780. Static call regions and the existing vertex fetch plan supply the bounded call/return model. Unsupported paths throw; see TechnicalDebt.md. (cherry picked from commit 49dcd66)
adriadam10
left a comment
There was a problem hiding this comment.
Rechecked the current head (8e63be9, only a merge with main since the last review): build and ctest on the merge are green (451/451, including agc_shader_control_flow, agc_driver_scalar_swappc, scalar_swappc). The two points from the previous review are still open:
- No test pins values for the new translation path:
translateExternalFetch(InstructionTranslator.cpp:105, called at:505) and the inlined attribute loads are not exercised.CheckLinkModelsNextInstructionAddress(ScalarSwappc.cpp:87) compares two IR dumps, which pins IR shape. Add a translation/execution test that runs a vertex entry with a fetch-shader call and checks the fetched attribute values, includingdst_seland the "more than four registers" throw. GraphBuilder.hppandScalarSwappc.cppstill lack the final newline.- New: #1523 touches the same lines of
GraphBuilder.hpp/GraphBuilder.cpp. If one builds on the other, list it in Depends on; otherwise expect a conflict for whichever lands second.
Not measured by us: the fetch-shader inline itself (the pc + 4 link semantics match what we measured on gfx1035).
Prerequisite from boykopovar#1217. Preserve current main trap decoding and adapt its regression; include the updated call refusal expectation from f2b3780. Static call regions and the existing vertex fetch plan supply the bounded call/return model. Unsupported paths throw; see TechnicalDebt.md. (cherry picked from commit 49dcd66)
Prerequisite from boykopovar#1217. Preserve current main trap decoding and adapt its regression; include the updated call refusal expectation from f2b3780. Static call regions and the existing vertex fetch plan supply the bounded call/return model. Unsupported paths throw; see TechnicalDebt.md. (cherry picked from commit 49dcd66)
Decodes SOP1 0x21 s_swappc_b64 (link pair = pc + 4) and resolves its call/return pairing through the existing s_setpc_b64 path: static call regions in GraphBuilder and the vertex-entry fetch-shader call inlined from the parsed fetch-shader plan. s_call_b64 (SOPK 0x16) and s_rfe_b64 (SOP1 0x22) throw at decode. Unmodeled cases throw; see docs/dev/TechnicalDebt.md.
…tched values Co-Authored-By: Claude <noreply@anthropic.com>
8e63be9 to
c3e16af
Compare
|
Thanks. #639 landed and conflicted with this PR, so I rebased it and finished the open points from my review. Your commit is kept, adapted to #639, and ours is on top:
Tested on the merge with |
adriadam10
left a comment
There was a problem hiding this comment.
Requested changes done in the commit on top; CI green on the rebased head.
|
Thank you for the rebase and the #639 adaptation — that is far beyond what I expected when I sent it, and the |
What
Main decodes
s_swappc_b64 nullass_setpc_b64(RdnaScalarOpDecoder.cpp:334) and the merged long-branch and dword-jump-table shapes leave a real destination — a call — throwing. A real destination is the GNM fetch-shader call: at a VS/ES/LS entry GNM swaps the PC to the fetch-shader region and the shader returns throughs_setpc_b64on its link pair. This PR translates that call, so shaders entering through a fetch-shader call recompile instead of throwing.0x21with a real B64 SDST pair decodes asSSwappcB64;scalarDestinationDwordCountaccounts the pair.D.u64 = PC + 4; PC = S0.u64, the link-register reading of the RDNA2 ISA (amdgpu_isa_rdna2.xml, SOP1s_swapPC_b64pseudo-code), matching KytyPS5'sDecodeSop1handling and shadPS4's fetch-shader call treatment (EmitFetch). Unverified on hardware — recorded as such indocs/dev/TechnicalDebt.mdper the Proposal: shader PRs state where instruction semantics come from (hardware or cited source), plus a hardware oracle tool #843 provenance rule.pc + 4) is recognized and the called region is translated inline with the paireds_setpc_b64acting as its return; static in-shader call regions lift through the same return path. Link arithmetic beyond the truepc + 4constant throws.docs/dev/TechnicalDebt.md.s_call_b64ands_rfe_b64are untouched in every form — decode, tests, and docs (final diff greps clean; feat(shader): implement S_CALL_B64 #895 implementss_call_b64for real).Tested
Windows 11, winlibs GCC 15.2 + Ninja.
Scoped ctest (CMake,
-DANYPS5_ENABLE_SPIRV_TOOLS=ON -DBUILD_TESTING=ON, FFmpeg pre-seeded):The
s_swappc_b64 with a return addresscase inControlFlow.cpppinned the old bareunsupported SOP1 opcode 33refusal; it now pins the newunclosable s_swappc_b64 call/return pairingrefusal.Standalone decode+translate closure (no cmake):
All
Requireassertions green: static call-region lifting, fetch-call-at-entry recognition (hasFetchCall,fetchCallProgramCounter), dest accounting, and the throw cases (link escape from the constant-offset call/return model, call region shared with another call, recursion). Fails-on-main proof: the test cannot build onmain—RdnaOpcode::SSwappcB64does not exist there and the call shapes hit the documented real-dest refusal.Checklist
main; no other open PR implements the same functions (feat(shader): implement S_CALL_B64 #895 implementss_call_b64— untouched here; fix(libSceAgc): accept s_swappc_b64 null, s[6:7] as a front half's terminator #1124 is the fused-half validator's terminator acceptance, consumer-side).mdfiles or images added to the repository (TechnicalDebt.mdentry only)