Repository navigation
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Excellent implementation of the 8 MIMG BY2/BY4 image transfer instructions. This is a significant contribution that increases shader instruction coverage from 98.03% to 98.71%.
Strengths:
- Comprehensive testing with both decoder/IR regression tests and Vulkan execution tests
- Proper handling of edge cases (overlapping VDATA/VADDR, unsupported formats/masks)
- Good documentation in TechnicalDebt about limitations and unverified behavior
- Correct use of existing infrastructure (resource tracker, descriptor materializer)
Minor notes:
- The PR is marked as draft with "AMD hardware confirmation is still required" - this is appropriate given the complexity
- Consider whether the format version bump (7→8) needs any migration handling for existing shader caches
This looks ready to merge once AMD hardware validation is complete and CI passes on Linux/Windows builds.
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Excellent implementation of the 8 MIMG BY2/BY4 image transfer instructions. This is a significant contribution to shader instruction coverage (bringing it from 1143/1166 to 1151/1166).
Strengths:
- Comprehensive decoding and translation for all 8 opcodes with proper validation
- Clean lowering into ordinary image reads/writes using existing infrastructure
- Thorough test coverage: decoder regression, IR equivalence checks, and Vulkan execution tests
- Proper handling of texel-major/component-minor register order and address advancement
- Good documentation of limitations in TechnicalDebt.md (register order, unaligned X, out-of-range behavior)
- Correct bump of shader disk cache format version
Technical notes:
- The validation logic for DMASK=0xf, 32-bit addresses/data, and identity-swizzled 2D images is appropriate
- Resource tracking correctly incorporates the
byElementsfield into image resource matching - The approach of deferring load destination writes until all texels are read (including VDATA/VADDR overlap) is correct
This will improve compatibility with games using these texture transfer instructions. Ready to merge once AMD hardware confirmation is obtained as noted in the draft status.
|
Review from the packed-gather measurement work (#1701) — oracle data on BY2/BY4 that may affect your decode validation and lowering math. Measured on gfx1031 (RX 6700 XT) with hand-encoded MIMG words; LLVM rejects these mnemonics on every RDNA1/RDNA2 target, so encodings are opcode-field substitutions on the assembler's 1. The window is block-aligned, not raw x. BY4 reads elements If the lowering reads 2. The whole aligned window must fit inside the width, else all-zero. BY4: x=28 works (window 28..31), x=29/30/31 write zeros; negative x writes zeros; BY2 x=31 writes zeros. This is a bounds check on 3. Data mask rule: exactly 4. y is effectively row-0-only in our probes. Every y≠0 read returned zeros even when height>1 (also at height=2 exactly); we suspect our hand-built descriptors (pitch/slice-pitch words left zero) rather than hardware, so we're not claiming a rule — just flagging that multi-row BY behavior is unmeasured territory. If your lowering maps BY to 5. Formats. Per ISA: BY4 is illegal for formats with more than 1 component, BY2 for more than 2. Consistent with your R32_UINT/RG32_UINT restriction. We only saw data on ≤8-bit single/double-channel formats in our probe set (R8_UNORM fmt=1; RG8 probes returned zeros in the configs we tried). Happy to share the probe scripts / raw output if useful — and thanks for doing the actual lowering. |
Happy to share probe scripts/raw output. |
0f53a18 to
b858fdc
Compare
|
@kali113, sorry: I closed this PR by mistake. I was rebasing your branch onto Nothing is lost:
The hardware (gfx1035, matching @pandather's gfx1031 data above) differs from the model here in a few places. The loads read the aligned window |
Status
Superseded by merged PR #1793. A maintainer accidentally closed this PR while rebasing, preserved my original work as the first commit of #1793, and added hardware measurements and corrections. This branch was reset to main during that incident; restoring it would duplicate the merged continuation.
AMD hardware measurements invalidated this draft's assumptions about wide formats, unaligned coordinates and bounds. The final implementation supports the four BY load variants for measured narrow formats, with aligned fetch windows and all-or-nothing bounds checks. The four store variants decode but still throw. The dashboard's eight newly decoded opcodes must not be read as eight completed runtime implementations.
Validation
The original draft at
0f53a18passed 444/444 Linux tests, including three consecutive BY execution, decoder/IR and disk-cache runs. Those Vulkan tests checked the draft's software model; they did not establish AMD hardware behavior. Its Windows build passed, butimage_bycould not start because libc was missing from its test PATH. Original validation run.The maintainer corrected that DLL path in #1793, replaced the assumed semantics with AMD oracle measurements, and expanded the execution tests for formats, masks, unaligned coordinates, mip selection and out-of-range accesses. #1793's Linux, Windows, conventions and progress checks pass. See that merged PR for the final changes and hardware evidence.
AI-assisted: yes (Codex). No commercial title or AMD hardware was run locally for this original draft.