Repository navigation
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Good implementation of VOP3 F32 interpolation instruction decoding. The comprehensive testing (660 combinations) and proper validation of edge cases is excellent.
Strengths:
- Thorough testing covering all opcode/attribute/channel/destination/source combinations
- Proper rejection of malformed, modifier, and truncated instructions
- Good documentation in TechnicalDebt about encoding provenance and limitations
- Correct reuse of existing VINTRP translation semantics
Minor notes:
- The PR mentions potential conflict with #697 (F16 interpolation opcodes) - worth coordinating if both are being worked on simultaneously
- No hardware validation was performed, which is appropriately documented
This looks ready to merge once CI passes and any rebase conflicts are resolved.
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Solid implementation of VOP3 F32 interpolation instruction decoding. This extends the decoder's capabilities while maintaining compatibility with existing VINTRP semantics.
Strengths:
- Clean addition of canonical VOP3 encodings for existing interpolation opcodes
- Proper validation and rejection of non-canonical forms (modifiers, attribute high bit, invalid sources)
- Comprehensive test coverage: 660 combinations plus malformed/modifier/truncation guards
- Good documentation with LLVM source references for encoding provenance
- Correct routing through the existing interpolation translator
Technical notes:
- The approach of treating these as additional encodings rather than new unique opcodes is correct for the progress counter
- Proper handling of both E32 and E64 instruction formats
- Test cases properly verify IR equivalence between VOP3 and VINTRP forms
This will improve compatibility with shaders using VOP3 interpolation instructions. Ready to merge.
|
ok, i think this can be merged. |
|
Closing: no title is shown reaching a VOP3 |
PR title
feat(shader): decode VOP3 F32 interpolation instructions
PR description
What
Add canonical, unmodified VOP3 encodings for the existing interpolation instructions:
V_INTERP_P1_F32(0x200)V_INTERP_P2_F32(0x201)V_INTERP_MOV_F32(0x202)Decode the attribute/channel and vector or parameter selector into the existing interpolation operand representation. Preserve the VOP3 family and two-word instruction length, and route VOP3 interpolation through the existing interpolation translator.
Reject source/output modifiers, the attribute high bit, noncanonical unused-source fields, invalid vector sources, and reserved parameter selectors rather than silently approximating them.
Register
vector_interp_e64with CTest. The shader progress counter is unchanged: this extends encodings of existing opcodes, not the unique opcode inventory.Encoding provenance: LLVM's VOP3Interp_gfx10, opcode assignments, and GFX10 assembler fixtures. Translation semantics are reused from this project's VINTRP implementation. Limitations and lack of hardware confirmation are recorded in
TechnicalDebt.md.Related: #697 adds different, F16 interpolation opcodes. This change does not require that PR, but both touch decoder/dispatch code and may need conflict resolution.
Tested
Windows, isolated C++20 regression harness, MinGW GCC 13.2.0, based on
e79c971f99acd6987b1d6545e9ba57c00b11fe04.VOP3 opcode is not implemented.No title or homebrew, console hardware, Linux execution, or SPIR-V backend validation was tested. Full-project configuration failed on missing SDL2/FFmpeg submodules; the supported GCC 15.2.0 build and full CTest suite still need to run.
The split patch passed
git apply --check --ignore-space-changeagainst the touched files from upstreammainat3c054008b8211405583e60c47eb13ca026e8eb10. That revision has not been compiled or tested with this change.Checklist
main; no other open PR implements the same functions — rebase and reconfirm before submission.TechnicalDebt.md.Patch refresh — 2026-10-08
The patch was regenerated against the user checkout at
4b2413fb5716bb56101e777536fe374e49ba90a2. Updated documentation context are preserved. Patch line endings are now LF.git apply --check --ignore-space-change --whitespace=error-allsucceeds with empty stderr on that checkout; actual application also succeeds on a disposable copy. The focused runtime test results above are from the earlier tested base, not a new test run on this revision.