Repository navigation
[JIT] Actionable diagnostic for dynamic layout field overflow - #1194
mustafayildirim wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate findings affect signed-range validation, diagnostic propagation, and test safety.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds launch-time validation for dynamic layout fields before ABI packing, replacing raw struct.error failures with actionable diagnostics.
Changes:
- Validates signed shape and stride widths.
- Propagates JIT argument and struct field names.
- Adds unit and GPU regression tests.
| File | Summary and review findings |
|---|---|
tests/unit/test_layout_field_overflow.py |
Adds overflow and launch tests. Moderate (1 vote): add CUDA availability guarding. Moderate (1 vote): mark the 10 GB repro large_shape. Nit (1 vote): pair l2_device with rocm_lower. |
python/flydsl/expr/struct.py |
Propagates struct field labels. Moderate (2 votes): propagate top-level parameter/path information through recursive plans to avoid ambiguous diagnostics. |
python/flydsl/compiler/jit_function.py |
Stamps JIT parameter names onto plans. Nit (1 vote): add fast-dispatch coverage asserting the actual parameter name. |
python/flydsl/compiler/jit_argument.py |
Implements ABI-width validation and diagnostics. Moderate (2 votes): accept the full signed lower bound. Nit (3 votes): correct contradictory remediation wording. Nit (1 vote): add DLPack-backed overflow coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Copilot review findings addressed in 30f2e0a (+ test fixes in ab52203/34a13c19):
Verified on gfx942 (MI325X): 7/7 layout-overflow tests, full |
) Launch of a layout-dynamic tensor whose shape or stride exceeds its packed ABI field width used to surface as a bare struct.error deep in the fill closure. Validate the fields before packing and raise a ValueError that names the offending JIT argument, the kind (shape/stride), the dim index, the value, and the signed width limit; suggest keeping the oversized dimension static or reshaping/splitting the tensor. - exact signed bounds (i32/i64, minima included) - struct-typed parameters compose the full field path (argument 'outer.inner') via path_suffix set during the struct ABI recursion + _stamp_plan_param_names at dispatch - tests: overflow messages, signed-min acceptance, struct paths, repeated-launch packing, and an l2_device launch repro on gfx942
34a13c1 to
f6589a5
Compare


What
Launching a kernel whose dynamic layout field exceeds its signed packed-ABI width failed with a raw
struct.error: 'i' format requires -2147483648 <= number <= 2147483647out of_LayoutPlan'spack_into(#1176). Per the maintainer analysis in the issue, the offending field is an individual dynamic dimension (e.g.shape[0]afterview(-1)of a >2^31-element tensor), not the total element count.This PR adds a launch-time diagnostic (no ABI change): every generated
fillvalidates its dynamic fields against the plan's packed widths (shape: signed i32; stride: i32 iffuse_32bit_stride, else i64) beforepack_into, and raises aValueErrornaming the JIT argument, field kind, dim index, value, and limit:The parameter name is stamped onto the plan by
_build_call_stateand propagated through struct fields asStruct.field, so the diagnostic works on the fast dispatch path. Validation is one compare per dynamic field per argument — only on the launch path, never in the pack fast path when values fit.Wider dynamic dims (i64 shapes, per the second option in the issue analysis) are deliberately out of scope: they require coordinated Python-ABI + compiler-type changes and device correctness tests on large tensors.
Testing
All on MI325X (gfx942), in-pod build against LLVM
e2a39f504fe:view(-1)) on upstreammainstruct.error: 'i' format requires ...ValueErrornaming arg,shape[0], value, limittests/unit/test_layout_field_overflow.py(4 L0 fill tests + l2_device launch repro)tests/unit/+tests/language/pytest (gfx942)Fixes #1176