Add test coverage for 8/16/64-bit types and vectors in dynamic dispatch#10475
Add test coverage for 8/16/64-bit types and vectors in dynamic dispatch#10475jvepsalainen-nv wants to merge 6 commits intoshader-slang:masterfrom
Conversation
No existing test covers 64-bit scalar types (double, int64_t, uint64_t) or vectors of non-32-bit types as fields in interface implementations dispatched through dynamic dispatch. The AnyValue marshalling code has explicit alignment and packing logic for each bit width, but these paths lacked runtime dispatch coverage. Tests added: - layout-64bit-scalar.slang: double, int64_t, uint64_t as impl fields, verifying 8-byte aligned two-uint32 split-and-reconstruct - layout-64bit-vector.slang: double2, vector<int64_t, 2> as impl fields, testing element-by-element 64-bit vector marshalling - layout-mixed-bitwidths.slang: int8_t, half, float, double, uint8_t together in one impl, exercising all alignment transitions - layout-8bit-vectors.slang: vector<int8_t, 4>, vector<uint8_t, 2>, vector<int8_t, 3> as impl fields, testing bitfield insert packing with different element counts including negative values - layout-16bit-vectors.slang: half4, int16_t2, vector<uint16_t, 3> as impl fields, testing 2-byte aligned packing All tests use a runtime-loaded kind buffer to force genuine dynamic dispatch. Backend coverage varies by type support: cpu and cuda for all; vk with render-feature flags; metal for 8/16-bit; dx12 for 16-bit. Closes shader-slang#10470
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds five new Slang shader tests and updates test expectations: tests that exercise dynamic-dispatch marshalling for 8-bit, 16-bit, 64-bit scalar, 64-bit vector, and mixed-bitwidth fields via interfaces, implementations, factories, and single-thread compute kernels with inline CHECK validations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/language-feature/dynamic-dispatch/layout-64bit-scalar.slang (1)
60-62: Use auint64_ttest value with non-zero upper 32 bits.
99still passes if the unsigned 64-bit path is accidentally truncated to 32 bits, so this branch doesn't really prove the split/reconstruct logic foruint64_t. A power-of-two likeuint64_t(1) << 40would keep thefloatcheck exact while forcing the high word to matter.Also applies to: 78-82
tests/language-feature/dynamic-dispatch/layout-mixed-bitwidths.slang (1)
65-70: Aggregate-only checks can miss edge-byte mispacking.
getSum()plusgetDoubleVal()doesn't pin down the placement ofaande: swapping those 1-byte fields still yields15.0. Please add at least one order-sensitive assertion for the narrow edge fields so this test can catch byte-slot placement bugs, not just gross corruption.tests/language-feature/dynamic-dispatch/layout-8bit-vectors.slang (1)
85-89: ExerciseVec2u8Impl.getFirst()too.The
300sum alone won't catch a lane swap or a broken first-element extraction on the 2-byte packing path. SincegetFirst()already exists, adding aCHECK: 100here would make this case as strong as theVec4i8ImplandVec3i8Implcoverage.tests/language-feature/dynamic-dispatch/layout-16bit-vectors.slang (1)
86-89: Add a first-lane assertion forInt16_2Impl.The
10.0sum still passes if(-10, 20)is unpacked as(20, -10), so this doesn't fully pin down the 16-bit lane layout. Writingi2.getFirst()withCHECK: -10.0would close that gap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebed0433-77e2-473b-9f3d-461d299384a0
📒 Files selected for processing (5)
tests/language-feature/dynamic-dispatch/layout-16bit-vectors.slangtests/language-feature/dynamic-dispatch/layout-64bit-scalar.slangtests/language-feature/dynamic-dispatch/layout-64bit-vector.slangtests/language-feature/dynamic-dispatch/layout-8bit-vectors.slangtests/language-feature/dynamic-dispatch/layout-mixed-bitwidths.slang
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
- Trim output buffer to match actual usage in layout-64bit-scalar - Make DoubleImpl::lowBits() use bitmask consistent with Int64/Uint64 - Add highBits() to IValue interface; use uint64_t with non-zero upper 32 bits to prove AnyValue marshalling preserves the full 64-bit value - Remove stray -slang flag from layout-16bit-vectors dx12 test line - Add getFirst() checks for Vec2u8Impl and Int16_2Impl to pin down lane ordering through dynamic dispatch - Add getByteFields() to IMixed interface for order-sensitive assertion of byte-sized field placement in layout-mixed-bitwidths Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e602af2-1de4-4579-abf6-f3152f53c71d
📒 Files selected for processing (4)
tests/language-feature/dynamic-dispatch/layout-16bit-vectors.slangtests/language-feature/dynamic-dispatch/layout-64bit-scalar.slangtests/language-feature/dynamic-dispatch/layout-8bit-vectors.slangtests/language-feature/dynamic-dispatch/layout-mixed-bitwidths.slang
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
The GLSL emitter is missing bitCast from UInt64/Int64 to Double (tracked in shader-slang#10505), causing these tests to fail when run through the CI Test Slang via glsl step. They pass with direct SPIR-V emission.
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
- Fix dx12 flag order in layout-16bit-vectors to match repo convention - Widen lowBits() mask from 0xFFFF to 0xFFFFFFFF to exercise full lower word of the two-uint32 split in layout-64bit-scalar
There was a problem hiding this comment.
Review: Add test coverage for 8/16/64-bit types and vectors in dynamic dispatch
Verdict: APPROVE ✅
Summary
Clean, test-only PR (zero production code changes) that fills important gaps in AnyValue marshalling coverage for non-32-bit types through dynamic dispatch. All 5 test files address the coverage gaps identified in #10470.
What's tested
| Test file | Types covered | AnyValue path exercised |
|---|---|---|
layout-64bit-scalar.slang |
double, int64_t, uint64_t |
8-byte aligned two-uint32 split/reconstruct |
layout-64bit-vector.slang |
double2, vector<int64_t, 2> |
Element-by-element 64-bit vector marshalling |
layout-mixed-bitwidths.slang |
int8_t + half + float + double + uint8_t |
All alignment transitions (1/2/4/8-byte) in one struct |
layout-8bit-vectors.slang |
vector<int8_t,4>, vector<uint8_t,2>, vector<int8_t,3> |
Bitfield insert packing with varying element counts |
layout-16bit-vectors.slang |
half4, int16_t2, vector<uint16_t,3> |
2-byte aligned packing with even/odd element counts |
Correctness checks performed
- ✅ All CHECK values verified mathematically — sums, sign extensions, bit extractions are correct
- ✅ Output buffer sizes match usage — no over/under-allocation in any test
- ✅ Expected via-GLSL failure entries are correct —
.slang.2suffix correctly maps to the 3rd (0-indexed) Vulkan TEST directive in each 64-bit test; 8-bit/16-bit tests correctly omitted (GLSL has extension support) - ✅ Render-feature flags follow conventions —
int16for 8-bit types on Vulkan (consistent withpack-any-value-8bit.slangandlayout-enum-underlying-types.slang),double,int64for 64-bit,int16,halffor 16-bit - ✅ Float precision safe — all int values stored in float buffers are within exact representability range (< 2^24)
- ✅ Filecheck pattern ordering is sound — sequential CHECK directives match output lines in correct order without false-positive substring collisions
Strengths
kindBufferpattern: UsingStructuredBuffer<int>to select implementations guarantees genuine dynamic dispatch — the compiler cannot specialize at compile time. More robust than theSV_DispatchThreadIDapproach in some older tests.- Re-read verification: Each test re-reads a previously constructed implementation after constructing others, catching potential AnyValue storage corruption.
- Signed/unsigned coverage: Tests include negative values (
int8_t(-1),int16_t(-10),int64_t(-42)) alongside unsigned, exercising sign extension paths. getByteFields()in mixed test: Thefloat(a) * 1000.0 + float(e)encoding pins down individual byte-field placement, not just aggregate correctness.- Backend coverage is well-chosen: DX12 only for 16-bit (sm_6_2), Metal for 8/16-bit, no 8-bit/64-bit on DX12, Vulkan with appropriate render-feature flags. All makes sense.
Minor notes (non-blocking, see inline)
DoubleImpl::lowBits()tests double→int64 conversion rather than raw bit extraction — acceptable sinceasFloat()already validates the double round-trip- Future maintainers should be aware that the
floatoutput buffer limits testable int ranges to < 2^24
Solid addition to the test suite.
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
No existing test covers 64-bit scalar types (double, int64_t, uint64_t) or vectors of non-32-bit types as fields in interface implementations dispatched through dynamic dispatch. The AnyValue marshalling code has explicit alignment and packing logic for each bit width, but these paths lacked runtime dispatch coverage.
Tests added:
as impl fields, testing 2-byte aligned packing
All tests use a runtime-loaded kind buffer to force genuine dynamic dispatch. Backend coverage varies by type support: cpu and cuda for all; vk with render-feature flags; metal for 8/16-bit; dx12 for 16-bit.
Closes #10470