Fix symbol-promotion and squeezing behavior in Frontend - #2443
Draft
ThrudPrimrose wants to merge 24 commits into
Draft
Fix symbol-promotion and squeezing behavior in Frontend#2443ThrudPrimrose wants to merge 24 commits into
ThrudPrimrose wants to merge 24 commits into
Conversation
ThrudPrimrose
force-pushed
the
fix-frontend-symbol-promotion-squeezing
branch
from
July 18, 2026 14:26
dc986a0 to
0cd34c9
Compare
ThrudPrimrose
force-pushed
the
fix-frontend-symbol-promotion-squeezing
branch
from
July 18, 2026 14:33
0cd34c9 to
e620b5c
Compare
ThrudPrimrose
marked this pull request as ready for review
July 20, 2026 13:57
ThrudPrimrose
marked this pull request as draft
July 20, 2026 15:12
A subsets.Range entry of size 1 is ambiguous: a rank-reducing index into a larger dimension and a genuine extent-1 dimension are both stored as (0, 0, 1). Range.squeeze() drops both, so a full read of a (NQ, 1, NP) view looks identical to a 2D slice. np.reshape(x, (NQ, 1, NP)) builds exactly that. SpecializeMatMul dispatched on the squeezed sizes and saw a matrix, picking Gemm, whose validate re-read the unsqueezed subset and raised "matrix-matrix product only supported on matrices". npbench's doitgen hits this at every size, on both the simplified and the auto-optimized pipeline. The descriptor is the authority: an entry of size 1 is rank-reducing only where the descriptor's own extent is larger. Under that rule the reshaped operand stays 3D and dispatches to BatchedMatMul, which is what numpy does, while an index into a larger dimension still squeezes to Gemm. BatchedMatMul.validate went through the same shared helper so both nodes agree on operand rank. Regressed in afd0efe, which removed the squeezing from the validate methods but left SpecializeMatMul dispatching on squeezed sizes. That commit also dropped the (NQ, 1, NP) reshape from the in-tree doitgen test, which is why CI stayed green.
numpy distinguishes `a[0]` from `a[0:1]`: an integer index removes its dimension from the result rank, a slice never does, not even at extent 1. `Range` stored both as `(i, i, 1)`, so the distinction was unrecoverable, and consumers needing the rank of an access had to guess by squeezing every extent-1 dimension. That guess is wrong whenever a sliced dimension happens to have extent 1, which is how `np.reshape(x, (NQ, 1, NP)) @ C4` ends up dispatched as a matrix product whose validator then rejects it as not a matrix. `Range` now carries a per-dimension `index_dims` flag, with `rank()`, `rank_dims()` and `is_index_dim()` derived from it, and the frontend records it for mixed subscripts. `squeeze()` deliberately keeps its meaning -- it is `np.squeeze`, dropping every extent-1 dimension -- and now maintains the flags rather than discarding them; `unsqueeze()` inserts slices, matching `np.expand_dims`. Because only a degenerate range can be an index, `is_index_dim` cross-checks the flag against the range instead of trusting it, so widening a dimension through `__setitem__` or a direct `ranges` assignment cannot leave it looking indexed. The flags survive JSON but not the string form, which still renders a degenerate range as `i`. That is deliberate: subset strings are re-parsed as symbolic expressions in places that reject slice syntax (`scalar_to_symbol` embeds them in interstate assignments) and are used as dictionary keys against the original text (`sdfg_to_tree` de-aliasing), so the rendering cannot change. Generated code is byte-identical and existing SDFGs load at the rank they were saved with, since a missing `indexed` entry means slice. Also reverts the MatMul operand change from 638022d, which redefined what `_get_matmul_operands` reports as operand size for every BLAS node. It made a 1x1 contraction look like a matrix while its descriptor stayed a scalar, so cblas_dgemm was handed a `double` where it wanted a `double*` (the CI failure in tests/numpy/einsum_test.py::test_opteinsum, reproducible under DACE_optimizer_autooptimize=1). It also broke Gemv's pure expansion and silently dropped alpha, beta and summation WCR by rerouting products to BatchedMatMul, which honours none of them. doitgen is still unfixed; doitgen_repro.py drives it and is temporary.
`SpecializeMatMul` selects GEMM by matching on the squeezed operand sizes, so `np.reshape(x, (NQ, 1, NP)) @ C4` is routed there as an `(NQ, NP) @ (NP, NP)` product. Since afd0efe, `Gemm.validate`, `ExpandGemmPure` and `_get_codegen_gemm_opts` re-read the raw subset instead, see rank 3, and reject the operand the dispatcher had just accepted with "matrix-matrix product only supported on matrices". npbench's doitgen fails at every size, on both the simplified and the auto-optimized pipeline. The fix is for GEMM to read the same tuple entries the dispatcher matched on. This does not redefine what `_get_matmul_operands` reports -- gemv, ger, dot and the vendor and FPGA expansions continue to read indices 4 and 5 exactly as before -- so it is confined to the GEMM path. Collapsing the unit dimension is exact, not a convenience: it is the row count of an `NQ`-long contiguous batch, so the product really is a single GEMM. Keeping it on GEMM also preserves alpha, beta and a summation WCR on `_c`, none of which BatchedMatMul honours; a unit-batch product with alpha now computes the right answer where it previously raised. afd0efe also weakened the in-tree benchmark, replacing the `(NQ, 1, NP)` reshape with `(NQ, NP)` so it stopped exercising the broken path, which is why CI stayed green. That line is restored, and it fails without this change.
A size computed in the program (`nt = Nt + 1` then `np.empty(nt)`) is a scalar data descriptor, but an array extent has to be a symbol. The previous approach ran a whole-SDFG ScalarToSymbolPromotion mid-parse, which deleted the scalar. Any later read or reassignment of the size then hit a hard KeyError, and the pass could disturb unrelated scalars in the half-built SDFG. Reuse the frontend's own promotion instead: `promote_scalar_to_symbol` mints a `__sym_<name>` symbol assigned from the scalar on an interstate edge and leaves the descriptor in place, exactly as `_promote` already does for subscripts. The shape is rewritten by substituting the symbols in, rather than mutating the SDFG. The promoted symbol is registered in the visitor's globals so nested scopes can resolve it as a free symbol of their scope arrays. The test now executes the programs and checks numerics for the reuse and reassign cases, which were the KeyError crashes, instead of asserting that the descriptor was destroyed.
…trix view
Follow-ups to tracking integer-index dimensions in Range:
- __setitem__ kept eager bookkeeping that unpacked every dimension as a triple,
but a degenerate dimension may legally hold the bare index expression instead
(add_indirection_subgraph assigns one, and dim_to_string has always rendered
it). That raised "cannot unpack non-iterable Symbol object" during frontend
indirection lowering. Revert __setitem__ to its plain form: is_index_dim
already cross-checks the range at read time, and now accepts the bare form.
- map_dim_shuffle permuted ranges and tile_sizes by hand, leaving the flags
misattributed to the wrong dimensions; use Range.reorder, which is exactly
this permutation. Two redundant_array sites rebuilt a Range from bare lists
and dropped the flags; carry them through (popped dimensions return as slices,
matching unsqueeze).
- The GEMM matrix-view rule ("raw subset if 2D, else squeezed") lived in two
places on two different inputs. Collect it into _matrix_subset_size beside
_matrix_operand in matmul.py; validate, expansion and codegen share it.
The index_dims flag added to Range to distinguish an integer index from a unit slice (numpy rank reduction) has no production consumer: rank(), rank_dims() and is_index_dim() are read only by their own test. The GEMM fix this PR needs dispatches on unit-dim squeezability, which is the opposite question -- a reshape to (NQ, 1, NP) is all slices (rank 3) yet must be one 2D GEMM -- so it uses squeeze(), never the flag. The flag is also not maintained through Memlet.from_memlet, bounding_box_union or propagation (all rebuild a Range from bare tuples and reset it), so rank() is unreliable after one propagation pass regardless. Revert subsets.py, memlet_parser.py, map_dim_shuffle.py and redundant_array.py to their pre-feature state and drop the flag's test. The GEMM matrix-view fix (_matrix_subset_size / _matrix_operand in matmul.py) is independent -- it reads only subset.size()/squeeze() -- and stays.
Shape promotion reused one __sym_<name> symbol per size scalar and re-assigned it on every use. Two arrays sized from the same reassigned variable (m = 64; a = np.empty(m); m = 2; b = np.empty(m)) then shared one symbol whose value was overwritten, so a read as length 2 instead of 64 -- a silent miscompile. A shape scalar used again as an index after the reassignment (a[m]) hit the same collision through the subscript-promotion path and read out of bounds. Mint a distinct symbol for each shape (promote_scalar_to_symbol grows a `fresh` flag), so every array records the size it was created with, and give fresh symbols a suffix so they can never land on the bare __sym_<name> the index path reuses. Iterating the size names in sorted order keeps the promotion states deterministic. ExpandGemmPBLAS still read the raw operand size while validate and the other expansions were moved to the matrix view, so a (NQ, 1, NP) operand validate accepts would be mis-sized there; read it through _matrix_operand too.
Shorten the docstrings and inline comments this PR added to DaCe's Sphinx conventions, and replace two hand-rolled helpers with existing utilities: find_new_name for the fresh promotion-symbol suffix, and symbolic.symlist for a shape's free symbols. Read the GEMM C operand through _matrix_operand like A and B rather than indexing the raw operand tuple.
Routing every subscript promotion through promote_scalar_to_symbol published its __sym_ name in ProgramVisitor.globals, which the original _promote never did. The leaked names shadowed name resolution and broke 25 frontend tests with KeyError: '__sym_...'. Only shape promotions need globals visibility, so gate the write on fresh.
A transient sized by a symbol that a control flow region's incoming edge assigns had no legal allocation point. simplify() moves the assignment onto an edge leaving the closest dominating state, and loops and conditionals are excluded as dominators, so the allocation was emitted in that state, ahead of the assignment. The array was then allocated at size zero and the first write corrupted the heap. Bracket every region with an empty state, so the assignment lands on the leading state's incoming edge and the allocation follows it. The trailing state also gives region-scoped deallocations somewhere to go; they were dropped before, leaking the array.
unordered_arglist followed an exit-node connector only one hop, so when the data leaves through several nested scopes the inner memlet names a local transient and the outermost array never enters the argument list. GPU codegen tiles device maps into nested scopes, so the kernel referenced an array and a stride symbol its signature never declared. Follow each path to its last edge, mirroring the read side.
ThrudPrimrose
added a commit
that referenced
this pull request
Jul 21, 2026
Cherry-picked from PR #2443. A size computed in the program (nt = Nt + 1; np.empty(nt)) is a size-1 descriptor, but an extent must be a symbol. The previous version promoted the descriptor itself with ScalarToSymbolPromotion, so two arrays sized from the same reassigned name collapsed onto one value and the second array got the wrong extent. promote_size_scalars_in_shape now rewrites the shape instead of mutating the SDFG, reading each size into its own fresh symbol on an interstate edge and leaving the descriptor in place so it can still be read or reassigned. The subscript-promotion path in ProgramVisitor is factored into promote_scalar_to_symbol and shares it. Adds PR #2443's test file: 5 of its 8 cases fail on the previous implementation.
Review of the promotion and squeezing work turned up four defects. A size used as an extent is admitted when its descriptor holds one element, but the interstate edge assigned the container by name. For an array that assigns its pointer, so np.empty(nt) with nt: int64[1] failed to compile. Read those through nt[0]; a scalar still reads by name. The rule for an operand's matrix view - the raw subset when it is already 2D, the squeezed one otherwise - lived in matmul alone. Transpose squeezed unconditionally, so it rejected an (N, 1) column as not a matrix, and its operand helpers indexed size[1] after squeezing and would raise IndexError in every expansion. Move the rule to blas_helpers.matrix_view, which also returns the dimensions it kept so sizes and strides cannot disagree, and use it from both libraries. The frontend no longer needs to route unit-dimension transposition around the library node. numpy.zeros, ones and full build their transient on their own path and so rejected a computed size that numpy.empty accepted. Promotion opens a state to carry the symbol assignment, so the fill has to be placed after it rather than in the state the replacement was handed. _get_matmul_operands left res_out unbound, crashing instead of reporting a missing _c connector.
…ueezing' into pr2443-work
np.zeros/ones/full built their transient directly, so a size held in a scalar was rejected while np.empty accepted it. Route them through promote_size_scalars_in_shape, which now reports whether it promoted: only then does the fill have to move to the state the promotion opened. The two tests asserting the rejection now assert the result.
RegionBoundaryStates wrapped every control flow region in two empty states. On a three-loop program with a branch that is 8 regions and takes the state count from 5 to 15, none of which was needed. The largest CloudSC SDFG has 778 regions and was paying 1556 empty states. The two sides answer different questions, so they are decided separately: - A leading state is where an allocation goes. It is needed when the region's incoming edge assigns a symbol that a transient's size reads. - A trailing state is where the matching deallocation goes. It is needed when the region ends its scope, whatever sized it: with no state after the last region, the free has nowhere to be emitted and is silently dropped. Bracketing only the sized regions leaked a transient that outlives the region assigning its size. Sizes are collected across the whole SDFG tree rather than per SDFG: a nested transient can be sized by a symbol an enclosing region assigns and passes down through symbol_mapping, and reading only the owning SDFG's arrays leaves that region unbracketed -- the malloc(0) bug again one level down. CloudSC now takes 79 states rather than 1556, in 39 ms. Tests pin all three: the state count of a program needing nothing, a conditional whose incoming edge carries the size, and the free that the first cut dropped.
ThrudPrimrose
added a commit
that referenced
this pull request
Jul 21, 2026
Extended only carried the frontend half of the fix. The rest is brought over from #2443, whose CI is green on all 15 jobs: - promote_scalar_to_symbol emitted `__sym_nt_0 = nt` for a size-1 array, which assigns its pointer. A size-1 array is a valid extent, so the assignment reads the subscript instead. - np.zeros/ones/full built their transient directly, so a size computed in the program was rejected while np.empty accepted it. They route through promote_size_scalars_in_shape, which now reports whether it promoted: only then does the fill move to the state that opened. - matrix_view() shares one rule for reading an operand as a matrix -- the raw subset when already 2D, the squeezed one otherwise. Squeezing unconditionally rejects a genuine (N, 1) column; not squeezing rejects an (NQ, 1, NP) reshape. MatMul had the rule inline, Gemm and Transpose lacked it, and Transpose indexed size[1] in every expansion, so an (N, 1) operand raised IndexError rather than failing validation. - _get_matmul_operands left res_out unbound, raising UnboundLocalError instead of the intended ValueError when _c is missing. - RegionBoundaryStates is wired into codegen after ControlFlowRaising. state.py is deliberately not taken: extended already fixes the same nested-scope kernel-signature bug through the memlet tree root. The PR's assertion on the generated signature is taken and passes against it.
Aligned allocations (#2438) changed the emitted line from `new double` to `new (std::align_val_t(64)) double`, and the free from `delete[] b` to `::operator delete[](b, std::align_val_t(64))`, so the two searches that find those lines stopped matching once this branch merged main. Both assertions are unchanged: the size symbol is still assigned before the allocation, and the array is still freed. Only the patterns used to find the lines are now spelling-agnostic, as the codegen tests on main already do.
…romotion-squeezing
…ol-promotion-squeezing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two DaCe Python-frontend fixes: promoting a size scalar used as an array shape to a symbol (
add_datadesc), so a frontend-materializedNt+1size no longer collides withadd_symbol(FileExistsError); and correctly transposing a unit-dim 2D array ((N,1)/(1,N)) in_transposeby squeezing the unit axis into a stride-safe copy, instead of the library node rejecting it as "not a matrix". Carries only the frontend edits plus two regression tests that expose both bugs.