dynamic dispatch: add test for nested existentials in composite types (#9839)#10116
dynamic dispatch: add test for nested existentials in composite types (#9839)#10116NV-xiaoyongs wants to merge 1 commit intomasterfrom
Conversation
…#9839) Test existential values crossing dynamic dispatch boundaries when nested inside structs and arrays. Covers 6 patterns with 12 scenarios: single container, array container, dual containers, mixed-interface container, triple dispatch (processor + nested IFoo + IBar param), and chained processors. Verified on vk, cuda, cpu, llvm backends with REPORT checks.
NV-xiaoyongs
requested review from
bmillsNV and
Copilot
and removed request for
a team and
Copilot
📝 WalkthroughWalkthroughIntroduces a new Slang test file for nested existential values within composite types across dynamic dispatch patterns. Defines interfaces, concrete structs, containers with existential fields, and multiple dispatch processors. Includes 12 test cases validating existential propagation through dispatch boundaries and buffer operations. 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
Claude finished @NV-xiaoyongs's task in 8m 24s —— View job Review complete
Result: Submitted a review comment. This is a well-structured, test-only PR adding comprehensive regression tests for nested existentials in composite types crossing dynamic dispatch boundaries (issue #9839). I manually verified all 12 expected computation values — all are correct. The test covers 6 dispatch patterns across 4 backends with both runtime ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/language-feature/dynamic-dispatch/nested-existential-dynamic-dispatch.slang (1)
7-13: Minor documentation inconsistency in pattern descriptions.Lines 9-10 mention "through IProcessor dispatch" but the actual implementations use different interfaces:
- Pattern 2 uses
IArrayProcessor(notIProcessor)- Pattern 3 uses
IDualProcessor(notIProcessor)📝 Suggested fix
// Patterns tested: // 1. Single Container (IFoo field) through IProcessor dispatch -// 2. ArrayContainer (IFoo[4]) through IProcessor dispatch -// 3. Two Containers through IProcessor dispatch (multiple nested existentials) +// 2. ArrayContainer (IFoo[4]) through IArrayProcessor dispatch +// 3. Two Containers through IDualProcessor dispatch (multiple nested existentials) // 4. Container with two different interface fields (IFoo + IBar) through dispatch // 5. Chained processors: output of one dispatched call feeds another // 6. IProcessor taking Container + additional IBar parameter (triple dispatch)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language-feature/dynamic-dispatch/nested-existential-dynamic-dispatch.slang` around lines 7 - 13, Update the top comment patterns to accurately name the interfaces used: change pattern 2's "through IProcessor dispatch" to "through IArrayProcessor dispatch" and change pattern 3's "through IProcessor dispatch" to "through IDualProcessor dispatch" so the descriptions match the implementations (refer to IArrayProcessor and IDualProcessor symbols in the test file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/language-feature/dynamic-dispatch/nested-existential-dynamic-dispatch.slang`:
- Around line 7-13: Update the top comment patterns to accurately name the
interfaces used: change pattern 2's "through IProcessor dispatch" to "through
IArrayProcessor dispatch" and change pattern 3's "through IProcessor dispatch"
to "through IDualProcessor dispatch" so the descriptions match the
implementations (refer to IArrayProcessor and IDualProcessor symbols in the test
file).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds a comprehensive test file (tests/language-feature/dynamic-dispatch/nested-existential-dynamic-dispatch.slang) targeting issue #9839. It tests existential values nested inside composite types (structs and arrays) crossing dynamic dispatch boundaries. No compiler source changes—test only.
What the test covers
The test exercises 6 dispatch patterns across 12 scenarios:
- Single Container (struct with
IFoofield) throughIProcessordispatch - ArrayContainer (
IFoo[4]) throughIArrayProcessordispatch (sum and max) - Dual Containers (two
Containerparams) throughIDualProcessordispatch - MixedContainer (
IFoo+IBarfields) throughIMixedProcessordispatch - Triple dispatch —
IWeightedProcessordispatching on processor type, nestedIFoo, and a separateIBarparameter - Chained processors — output of one dispatched call feeding into another
Backends tested: -slang, -vk, -cuda, -cpu.
Verification
I manually traced all 12 expected values through the dispatch logic (with id=0 from numthreads(1,1,1)):
| Test | Processor | Input | Expected | ✓ |
|---|---|---|---|---|
| 1 | ScaleProcessor(3.0) | FooA(5.0), meta=7 | 5×3+7=22.0 | ✓ |
| 2 | OffsetProcessor(10.0) | FooB(4.0), meta=3 | 8+10+3=21.0 | ✓ |
| 3 | ArraySumProcessor | [1,4,3,8] | 16.0 | ✓ |
| 4 | ArrayMaxProcessor | [1,4,3,8] | 8.0 | ✓ |
| 5 | DualScaleProcessor | FooA(3)×FooB(2) | 3×4=12.0 | ✓ |
| 6 | DualAddProcessor | FooB(5)+FooA(7) | 10+7=17.0 | ✓ |
| 7 | MixedScaleProcessor | FooA(6)×BarX(3) | 18.0 | ✓ |
| 8 | MixedSumProcessor | FooB(3)+BarY(10) | 6+5=11.0 | ✓ |
| 9 | WeightedProcessor | FooB(4)×BarX(2) | 8×2=16.0 | ✓ |
| 10 | BiasedProcessor | FooA(3)+BarY(8)+1 | 3+4+1=8.0 | ✓ |
| 11 | Chained Scale→Offset | FooA(5)→FooA(10) | 20.0 | ✓ |
| 12 | MixedScale swapped | FooB(2)×BarY(6) | 4×3=12.0 | ✓ |
All values are correct. Buffer size (12 floats) matches the output indices [0..11]. REPORT-DAG line offsets all reference the correct dispatch site lines.
Key findings
No bugs or correctness issues found. The test is clean.
Positive aspects
- Excellent documentation: Each test block has clear comments explaining the expected computation, making the test self-documenting and easy to maintain.
- Comprehensive dispatch coverage: 6 patterns covering single, array, dual, mixed, triple, and chained dispatch—good breadth for a regression test.
- REPORT checks: The
SIMPLE(filecheck=REPORT)test with-report-dynamic-dispatch-sitesverifies not just runtime correctness but also that the compiler correctly identifies all dispatch sites. This is valuable for catching regressions in the dispatch analysis itself. - Clean structure: Separation of interfaces → composites → processors → helpers → entry point makes the 469-line file easy to follow.
- Multi-backend: Testing across
-slang,-vk,-cuda,-cpuprovides good backend coverage.
Minor observations (non-blocking)
- Issue #9839 also mentions loading existentials from
StructuredBuffer<Container>(buffer-loaded existentials). This test covers the "passed between dynamically dispatched functions" aspect but not the buffer-loading scenario. That could be a follow-up. - The PR description mentions "llvm" backend but the test uses
-slang(which may use LLVM internally). Not an issue, just a note.
Verdict
Looks good. This is a well-structured, comprehensive regression test with correct expected values. No inline comments needed—the code is solid.
1 participant
Fixes #9839
Test existential values crossing dynamic dispatch boundaries when nested inside structs and arrays. Covers 6 patterns with 12 scenarios: single container, array container, dual containers, mixed-interface container, triple dispatch (processor + nested IFoo + IBar param), and chained processors. Verified on vk, cuda, cpu, llvm backends with REPORT checks.