Deterministic dace toolchain draft#17
Open
tehrengruber wants to merge 55 commits into
Open
Conversation
spcl#2298) Get the number of warnings in tests down by avoiding usage of `state.add_*` functions like `state.add_array(...)`. --------- Co-authored-by: Roman Cattaneo <>
The `ControlFlowReachability` pass gets prohibitively expensive in particular graphs. Updating from `v1/maintenance` to current `main`, we have seen `simplify()` runtimes of 10-15 minutes where previous runtime was in the order of magnitude of tens of seconds. The slowdown turned out to be caused by not caching closures per region. Some of our graphs generate large, nested control flow regions (if statements) from iterative solvers with conditional returns that we map to if/else blocks with a boolean mask. In such a scenario, having four layers of nestedness is easily achieved and then `_region_closure()` gets called again and again for previously calculated closures of regions. Because of the transitive requirement of theses closures, control flow regions nested deep will have to "go up" an re-evaluate the same closures for "upper" regions again and again. This PR suggest a simple cache of closures per region to avoid this duplicate evaluation. Co-authored-by: Roman Cattaneo <>
Reduces SDFG size when serialized, using the following methods: * Non-human-readable JSON dumping by default * Consolidating file names in DebugInfo to be per-SDFG * Reducing the size of the DebugInfo JSON object based on fields * Saving transformation history set to off by default
This PR suggest to write a `CACHEDIR.TAG` file into the program folder. The tag is an attempt to signal (e.g. to backup software) that the containing folder contains no archival value, see http://www.brynosaurus.com/cachedir/. While the convention started with cache directories for things like thumbnails of a webbrowser, I'd argue the same argumentation (no archival value, frequent changes, un-suiteable to be located in `/var/cache` or `/tmp`) apply for build folders. Instead of writing the file by hand, we could also a library like https://pypi.org/project/cachedir-tag/. --------- Co-authored-by: Roman Cattaneo <>
Set is not hashable doesn't work with lru cache decorator, I propose using FrozenSet here.
Some parts of DaCe are currently relying on `six`, a python2 / python3 compatibility library. Given that DaCe is only supporting python 3.10 - 3.14 now, I think we don't need the `six` dependency anymore.
Following up on PR spcl#2312, this PR proposes to save compressed SDFGs in the program folder. As discussed in the last meeting: - no change to the API, i.e. we keep keyword arguments of `sdfg.save()` as they are. - save `program.sdfg` as compressed `program.sdfgz` inside the program folder - make sure that changes are backwards compatible and that the program folder is still found regardless of `program.sdfg` or `program.sdfgz` --------- Co-authored-by: Roman Cattaneo <>
If we unroll a top-level for CFG, then the connectivity might be broken, I added a unit test and the fix to it. Replace dict on loop does not properly update the init statement, which can be exposed by loop unrolling when parent loop parameter is used inside in an inner loop. I fixed it and added a unit test in loop unroll that would expose it.
`Range` subsets have a `reorder()` function that re-orders the dimensions in-place. So far, it only re-ordered the ranges, but not the tile sizes (which are stored in a separate list). This PR makes sure both, ranges and tile sizes, are re-ordered according to the given permutation. The PR adds a simple test case.
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
This is a first high level review of the changes.
| self._dst = dst | ||
| self._data: T = data | ||
|
|
||
| def id(self): |
There was a problem hiding this comment.
There are several things here.
First of all this type of edge is only for edges between states or to be precise control flow regions.
Inside a state, there is a second kind of graph, the dataflow graph.
down in the file you will find the MultiEdge (which is probably irrelevant) and the MultiConnectorEdge (which is the relevant), which are used for the dataflow graph.
In addition these other kind of edges are actually missing the id() function.
Then, you have added the id property to the Node, however, this is only the base class for the the nodes of the dataflow graph.
Thus, calling this function will fail, because the nodes, which are control flow regions do not have that attribute.
You must add them either to AbstractControlFlowRegion or what is probably better to ControlFlowBlock, but in that regard I am not fully sure.
Furthermore, two nodes can have multiple connections between them.
This is more relevant for the dataflow graph, however, it is technically still allowed for the state graph, although less relevant.
Thus, edge.id() is not an unique key, as there could be several edges between any two nodes.
To make them unique, you need to include the .data property, which is either a Memlet or an InterstateEdge.
| @@ -215,7 +218,7 @@ def __getitem__(self, node: NodeT) -> Iterable[NodeT]: | |||
|
|
|||
| def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]: | |||
There was a problem hiding this comment.
Suggested change
| def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]: | |
| def all_edges(self, *nodes: NodeT) -> List[Edge[EdgeT]]: |
| return current | ||
|
|
||
|
|
||
| # TODO: check if the edges need an id. If source and dest, are equal they should be interchangable right? |
There was a problem hiding this comment.
No, see my comment above.
Essentially the code:
a[3] = b[4]
a[6] = b[5]Will lead to an SDFG with two nodes one for a and one for b and two edges between them, each does an assignment.
There are similar situations with Maps.
| "type": typestr, | ||
| "label": labelstr, | ||
| "attributes": dace.serialize.all_properties_to_json(self), | ||
| # TODO(tehrengruber): This id looks very similar to the ID I introduced. |
There was a problem hiding this comment.
This is not stable it is essentially the position in the sorted node array, see here.
This design has some very nasty consequences in the way how transformations have to be implemented.
This is the transformation why your transformations look like:
def apply():
matched_node = self.pattern_node
# Never use `self.pattern_node` from here always use `matched_node` and pass it to all functions!
| ############################## | ||
|
|
||
| # TODO(tehrengruber): This could also be done using the counter. | ||
| def _find_new_name(self, name: str): |
There was a problem hiding this comment.
Yes it would probably better, because not all symbol (names) are stored in self.symbols, for example the symbols used as Map parameters are not inside it.
| """ | ||
| return set() | ||
|
|
||
| # TODO(tehrengruber): should we make this an ordered set? |
There was a problem hiding this comment.
I am actually in favour of that, however, there are some issues.
This function exists because SymPy had it and a lot of other classes have such a method (although it is most likely implemented by used_symbols()).
Thus you will have to change them all.
Furthermore, while here the return type is set[str] this is actually wrong.
Some objects, especially the ones that are very close to SymPy, return a set of dace.symbolic.symbol (which extends (but does not wrap) SymPy's symbol) and sometimes you even get a mixture of them.
So this is a big set of work.
Comment on lines
+90
to
+91
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> Set[SDFGState]: | ||
| closure: Set[SDFGState] = OrderedSet() |
There was a problem hiding this comment.
Suggested change
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> Set[SDFGState]: | |
| closure: Set[SDFGState] = OrderedSet() | |
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> OrderedSet[SDFGState]: | |
| closure: OrderedSet[SDFGState] = OrderedSet() |
| """ | ||
| single_level_reachable: Dict[int, Dict[ControlFlowBlock, | ||
| Set[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) | ||
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) |
There was a problem hiding this comment.
Suggested change
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) | |
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(OrderedSet)) |
| # By definition, data that is referenced by the conditions (branching condition, | ||
| # loop condition, ...) is not single use data, also remove that. | ||
| for cfr in sdfg.all_control_flow_regions(): | ||
| single_use_data.difference_update(cfr.used_symbols(all_symbols=True, with_contents=False)) |
There was a problem hiding this comment.
used_symbols() also returns a set.
| array_name = an.data | ||
| write_subsets = set(e.data.dst_subset for e in st.in_edges(an)) | ||
| write_subsets = OrderedSet(e.data.dst_subset for e in st.in_edges(an)) | ||
| wss = str(write_subsets) |
There was a problem hiding this comment.
I know outside of the scope of this PR, but serializing a set and then use it as keys does not make sense to me.
The initial idea was to reduce the size of the cache folder, by only storing the components that are needed. To this end the code generator was modified such that different versions of build folder could be generated. Currently there are only two versions: - `development`: Which is the old full version, i.e. everything in a single folder. - `production`: This is a reduced folder and only contains the libraries (stub and program library) as well the version. The implementation has two parts. First `generate_program_folder()` was modified such that it only generates the parts that are absolutely needed, such as source files and anything else would not be generated in the first place. Then `compile_and_configure()` was modified such that it would remove the parts that are no longer needed (an example would be the source files which are needed for compilation but not afterwards). The changes are backwards compatible. Thus, caches that were generated _before_ this PR will still work, but they should be phased out. The changes are also done in a way that it should be simple to add new modes later, if their need arises. In Addition: - Also fixes that `sdfg.view()` fails if there are external SDFGs ([727cfb1](spcl@727cfb1)) - `sdfg.generate_code()` no longer generates the source maps as a side effect and writes them to disc. Instead they are generated by the `generate_program_folder()` ([5e1694b](spcl@5e1694b)). - Dumping of the configuration in the build folder ([eb54062](spcl@eb54062)). - Miscellaneous refactoring in the `ReloadableDLL`, that changed its interface.
Old version is now deprecated
Unused imports add a performance overhead at runtime, and risk creating import cycles. To automatically detect and remove them in the future, this PR suggests to add [ruff](https://docs.astral.sh/ruff/) as a `pre-commit` hook to detect unused imports. I've added an exception for `__init__.py` files to allow re-exports without adding an explicit `__all__` list or adding extra annotations. The PR grew quite big. However, non-automatic changes are only in the following seven files - `.pre-commit-config.yaml`: configure `pre-commit` to run the `ruff` linter - `ruff.toml`: configure the `ruff` linter to only search for unused imports (rule `F401`) - `dace/autodiff/library/library.py`: make sure we keep the `ParameterArray` import for backwards compatibility - `dace/frontend/python/replacements/operators.py`: make sure we keep the `dace` import for evaluation of data types - `tests/library/include_test.py`: make sure we keep the necessary import in the middle of the test - `dace/sdfg/analysis/schedule_tree/treenodes.py`: manually remove the now trivial `if TYPE_CHECKING` branch
When we started to enforce consistent formatting on the CI (PR spcl#1957), an important discussion point was for developers to see changes that needed to be applied. For lack of better knowledge, I've added a small script to show the `git diff` output in case of failure. I recently learned that `pre-commit` has a built-in `--show-diff-on-failure` option <img width="3241" height="1202" alt="image" src="https://github.com/user-attachments/assets/843ebae8-1dda-401d-913e-f6dabf150bd0" /> which enables exactly this behavior out of the box ([link to failing workflow run](https://github.com/spcl/dace/actions/runs/24556723300/job/71794961944?pr=2337)). It even comes with a colored output ... 🤩 I suggest, we drop the custom script since this option is much simpler.
- Fix script to query the HIP architecture of the machine - Remove explicit setting of `--offload-arch` in `HIP_HIPCC_FLAGS` - Set `CMAKE_HIP_FLAGS` instead of `EXTRA_HIP_FLAGS`
When we build an SDFG, there's the option to store `DebugInfo` with some SDFG nodes. For example, this `DebugInfo` can be used to store file & line information of parsed code when building an SDFG. When using the SDFG API, the default is to inspect the python stack and extract file & line information from there. These calls to `inspect` can/will be expensive, especially for bigger graphs. This PR proposes to add a configuration option, `compiler.lineinfo`, to drive this behavior from a single place. The defaults are kept as is, i.e. we keep inspecting the stack by default. However, the config option allows a central pace to turn `DebugInfo` off, which could be configured in production scenarios.
Two changes: - Rename `build_folder_version` to `build_folder_mode`. - Write folder mode to file `FOLDER_MODE` instead of `VERSION`. - Always save the `CACHEDIR.TAG` file .
## Description In PR spcl#2321 we introduced a way to turn of automatic stack inspection when adding a node to a state without explicitly stating debug info (such as file name, line info, ...). Since merging this PR, we get a ton of `DeprecationWarning`s from code added in `_get_debug_info()`. In a first attempt (see PR spcl#2344), we tried to work around the issue by setting `._default_line_info` on the state. This method proved to be very verbose and unnecessarily complicates parsing, where we have a legitimate use-case for explicitly passing along an explicit `DebugInfo`. This PR suggests to keep the API of `state.add_*()` functions as before, partially reverting changes from PR spcl#2321. Doing so, we can drop the deprecation warnings because nothing changes.
## Description This PR updates the action `codecov/codecov-action` from `v5` to `v6`. This step is needed to support GHA runners based on node24. <img width="1655" height="428" alt="image" src="https://github.com/user-attachments/assets/0901bf1b-2285-4394-b005-4b64b855233e" /> The only change in the codecov action between `v5` and `v6` is to support GHA runners based on node24. The update should come without any issues. More information can be found [here](https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/).
This fixes 2 bugs in map fission. 1) Loop-dependent views are incorrectly hoisted. For example: The read `__tmp_726_11_r` depends on `A[i]` but it is considered as the subset if not present within the NSDFG. <img width="608" height="512" alt="Pasted image" src="https://github.com/user-attachments/assets/84c525ae-f0d2-4184-a592-0fd0c0141c40" /> After it becomes wrongly hoisted and the SDFG does not compile anymore: <img width="752" height="515" alt="Pasted image (2)" src="https://github.com/user-attachments/assets/02a70bc3-55d6-4067-893b-bc7b29babb76" /> 2) Second, if the step size is not equal to 1, then the array dimensions between the maps are generated according to the map trip count, but the access expression is generated only via the map iterator. I fix this by generating the access expression for intermediate arrays via `(i - begin)/step` such that the generated access expression is always correct , regardless of the map's shape.
Fixes readthedocs failing build, as well as all errors and warnings in documentation, including adding a new Libraries and Environments page.
A simple pass that can detect some reduction patterns: From this: <img width="452" height="571" alt="image" src="https://github.com/user-attachments/assets/8a08db65-92e6-4193-abb8-4dc4a67a1b4b" /> to: <img width="580" height="460" alt="image" src="https://github.com/user-attachments/assets/2c7619d8-ee27-464e-a334-6a021673f6b7" /> or from: <img width="227" height="673" alt="image" src="https://github.com/user-attachments/assets/73bbc634-e3b9-4474-812f-6ea2573042ee" /> to: <img width="262" height="663" alt="image" src="https://github.com/user-attachments/assets/9127ab4d-365a-400f-b5e7-6556de9016b2" />
This PR adds a conversion from the Schedule Tree representation to SDFG. In particular, the following features are implemented: * Keep descriptor repository in schedule tree root and use in reconstruction * Find state boundaries in stree * Generic input/output memlet recovery for stree nodes * MemletSet class and propagation for in/out memlet recovery for stree scopes * Create nested SDFGs only as necessary (i.e., if state boundaries appear in a dataflow block, depending on behavior) Several features are not yet implemented and will be added in a future PR. --------- Co-authored-by: Tal Ben-Nun <tbennun@users.noreply.github.com>
## Description This is a follow-up from spcl#2346. By convention, pytest searches for tests in files ending with `_test.py` (unless otherwise configured). The new min warps by eu test was thus never running on CI: - gpu test run [before 2346 merged](https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/5985887337886893/5392709053677205/-/jobs/14116323037) shows 307 test cases executed - gpu test run [after 2346 merged](https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/5985887337886893/5392709053677205/-/jobs/14139835937) still shows 307 test cases executed With this rename, it will be picked up and the number of gpu tests executed should increase to 308. Co-authored-by: Roman Cattaneo <>
Second installment of documentation entries. - [x] Update getting started tutorial with easier CompiledSDFG syntax (Fixes Issue spcl#1011) - [x] Audit entries for correctness w.r.t. code - [x] Python frontend features / support - [x] Python frontend preprocessing - [x] Optimizing SDFGs with the SDFG API - [x] Schedule Tree representation - [x] Extending DaCe - [x] Working with symbolic expressions - [x] New library nodes - [x] New frontend - [x] DSLs and SDFGConvertible - [x] New Backends - [x] Writing your own instrumentation provider
The variable `connections_to_make` was a class variable and not marked as a dace property and hence shared between all instances. It is now an instance variable and thus private. Also the variable was never cleared, so technically, the connections should have accumulated inside it. Furthermore, the transformation assumed that `can_be_applied()` was called immediately before `apply()`, because the `can_be_applied()` function populates the `connections_to_make`. However, this behaviour is not guaranteed and now `apply()` explicitly calls `can_be_applied()` to make sure that `connections_to_make` is properly populated.
If Loop2Map references arrays only on IfBlock or InterstateEdge, then the symbols necessary for stride/offset/shape are not added properly to the Nested SDFG, causing an invalid SDFG. There is also a new test case now. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dead control flow elimination does not simplify by trying to evaluate trivial if conditions. I wrote a pass that does that: It tries to eval if branches that are trivially true and then lifts the inner CFG upwards: <img width="442" height="699" alt="image" src="https://github.com/user-attachments/assets/913a70e2-bee1-42e3-afbf-abd8be385715" /> Becomes: <img width="455" height="699" alt="image" src="https://github.com/user-attachments/assets/5ad125e3-b4cf-4cf1-b040-d43c4a6bda6c" /> It supports branches of type: ``` if (cond) ``` and (empty else): ``` if(cond) ... else: .... ``` Example: <img width="738" height="635" alt="image" src="https://github.com/user-attachments/assets/43c9bce7-41e7-4894-a4d1-876170400ab1" /> Becomes: <img width="699" height="680" alt="image" src="https://github.com/user-attachments/assets/d08da18c-0825-4afc-8956-cad007b3e6d9" /> --------- Co-authored-by: Philipp Schaad <schaad.phil@gmail.com>
I have added cutensor tensor transpose expansion for cutensor v2 and ported tensor contraction to cutensor v2 too. After this we should also close this old PR: spcl#1303.
## Fix two breaking patterns in Map Fission
This PR fixes two SDFG patterns where `MapFission` produced an invalid
graph
("Leftover nodes in queue" from `scope_dict`).
### Pattern 1 — map nested inside another map
<img width="1111" height="671" alt="Breaking pattern 1"
src="https://github.com/user-attachments/assets/f9bd40ec-f0c7-454e-bb47-07c1ffe754bf"
/>
Before fix 1:
<img width="1448" height="545" alt="Before fix 1"
src="https://github.com/user-attachments/assets/10e36cc1-04e5-4199-8dca-4773a2b098ba"
/>
After fix 1:
<img width="1250" height="752" alt="After fix 1"
src="https://github.com/user-attachments/assets/848e6463-928b-4034-9563-cca6ea171437"
/>
### Pattern 2 — conditional inside the fissioned component
<img width="857" height="624" alt="Breaking pattern 2"
src="https://github.com/user-attachments/assets/177e4ec8-4ecb-4b4e-af26-7813cdcb79d5"
/>
Before fix 2:
<img width="1042" height="692" alt="Before fix 2"
src="https://github.com/user-attachments/assets/adbe14ae-49ae-4d5e-ac96-16fef0aa962c"
/>
After fix 2 (left untouched — refused, not corrupted):
<img width="1032" height="760" alt="After fix 2"
src="https://github.com/user-attachments/assets/c166a42c-fbf0-43ec-9524-1032c9ee628e"
/>
### Follow-up
Supporting if/else fission requires sequentializing and splitting the
branch
conditions and replicating them per fissioned map. I plan a Map Fission
extension (also for Map Fusion, vertical and horizontal, to recombine
branches), tracked separately and out of scope for this bug-fix PR.
## Refuse state fusion that breaks happens-before ordering After `StateFusion` runs, some graphs become incorrect because a happens-before dependency before the fused states is lost. Pattern this fixes: <img width="304" height="496" alt="Pattern" src="https://github.com/user-attachments/assets/1f4cdb9a-4856-4b06-bfa3-3e469783d367" /> Buggy result after fusion: <img width="1108" height="530" alt="Buggy after fusion" src="https://github.com/user-attachments/assets/9b5e8069-5527-46ca-b7d6-70749337acac" /> One possible fix is to insert explicit dependency edges, but that bloats the graph and is not, in my view, a good approach: <img width="734" height="602" alt="Dependency-edge alternative" src="https://github.com/user-attachments/assets/80cfce82-c3f8-4979-904a-54863c388d05" /> **Current approach:** refuse the fusion in this case, leaving the graph correct rather than producing an invalid one.
In [PR#2331](spcl#2331) and [PR#2348](spcl#2348) the possibility of different folder modes/versions was introduced with the goal of reducing the size of the generated folder. The idea was to "only dump what is needed". In `production` `program.sdfgz` (serialized SDFG) was not dumped. This had the effect that it is not possible to construct a `CompiledSDFG` object (and call the SDFG from Python) without having the source SDFG in the first place. This PR solves this issue by again dumping the SDFG. Note in the long run one should switch to `nanobind` instead of `ctype`, which would make the binary an importable package and would also allow to offload the processing of the arguments to `C++`. For more information on this see [issue#2362](spcl#2362).
…ic (spcl#2372) ## Maps must have a positive step A DaCe `Map` is an unordered, ascending iteration domain, and the CPU backend emits an ascending loop only (`for (i = begin; i < end + 1; i += step)`). A negative/descending step silently miscompiles. As we would need to check for `>`. I think Loop2Map already normalizes the loops, but nothing prevents the user from writing `dace.map[10:0:-1]` (validation passes) which I did and noticed this bug :). a ### Changes - **`Map.validate`**: reject a *provably* descending map. - **CPU codegen**: for a symbolic step whose sign can't be decided statically, emit a debug-only `assert((step) > 0)` before the OpenMP pragma. It should not have a performance impact, as we compile with `-O3` by def. No existing test uses a negative-step `dace.map`; positive-step maps are unchanged.
Convert process grids, subarrays, and redistribution arrays to special data descriptors. Removes specialized SDFG fields and streamlines code generation.
Small fix for sporadic test failure caused by low floating point tolerance and unseeded random input.
- [x] Clarify interstate edge assignment type - [x] Make pystr_to_symbolic more efficient by using globals - [x] Create a `Subscript` function in symbolic expressions --------- Co-authored-by: Yakup Koray Budanaz <budanaz.yakup@gmail.com>
Instead of symbolic expressions as sympy strings (which might be ambiguous in symbol names, assumptions, and properties), this PR modifies symbol serialization to emit strings as follows: * Symbols are always prefixed with a dollar sign (`$i`, `$return`). * Symbols with non-default assumptions are saved with a unique function called `symbol`. For example, `symbol($symbol, integer=True)` or `symbol($N, dtype=dace.uint64)`. * All constants follow a Rust-like convention that suffixes the data type at the end. For example: `1i16, 7.2bf16, 8.0f64, 9f32` and will always deserialize to the same type in code generation. * The `SymExpr` class is serialized as a function `SymExpr(expr, overapprox)`. * No implicit simplification done on the expressions. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tbennun <8348955+tbennun@users.noreply.github.com> Co-authored-by: Tal Ben-Nun <tbennun@gmail.com> Co-authored-by: Tal Ben-Nun <tbennun@users.noreply.github.com> Co-authored-by: Yakup Koray Budanaz <budanaz.yakup@gmail.com> Co-authored-by: Yakup Koray Budanaz <ybudanaz@ethz.ch>
## Problem
`ProgramVisitor.make_slice` builds the slice-read memlet with
`Memlet.simple(array, rng, ...)`. `Memlet.simple` stores a passed-in
`Subset` **by reference** (it does not copy), and `rng` is frequently
the cached `Range` object handed back by the `accesses` cache on a
repeated read of the same array slice.
So two sibling reads of the same slice (e.g. `arr[i, k]` in two loop
bodies under a map) produce two distinct edges whose memlets **share one
subset object**. That violates the invariant that each memlet owns its
subset: any later in-place subset rewrite — loop-iterator renaming,
symbol replacement, offsetting — on one edge silently corrupts the
other.
### Reproducer
```python
N = dace.symbol('N')
@dace.program
def two_sibling_slice_reads(out: dace.float64[N, N], arr: dace.float64[N, N]):
for i in dace.map[0:N]:
for k in range(N):
out[i, k] = arr[i, k]
for k in range(N):
out[i, k] += arr[i, k]
```
In the parsed SDFG, the two `arr[i, k]` read edges share a single
`Range` subset object. A pass that rewrites a subset in place on one of
them (the new test originally surfaced this through a loop-iterator
rename) then corrupts the sibling.
## Fix
Deepcopy `rng` for the slice memlet's subset in `make_slice`, mirroring
the `other_subset` deepcopy two lines above. This is a pure
object-identity fix — subset values and the generated SDFG are
unchanged.
## Test
`tests/python_frontend/slice_subset_aliasing_test.py` asserts that no
two memlets in the parsed SDFG share a subset object, plus an end-to-end
value check. Fails before the fix, passes after.
This PR allows SDFGs from versions earlier than 2.0.0a4 (pre-released with this PR) to be loaded successfully.
[PR#2366](spcl#2366) introduced a new global that was used during serialization. However, this fails when multiple threads are active, which is not the default mode of operation (except in GT4Py and the [DaCe tests themselves](spcl#2126)). This PR made the serialization thread safe. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…line (spcl#2401) This PR uses a list for the return type of `depends_on()` in order to enforce deterministic dependency ordering in SDFG optimization pipelines. --------- Co-authored-by: Till Ehrengruber <till.ehrengruber@cscs.ch>
This PR adds a special case for 1D slices that are contiguous in that dimension, but are not packed. --------- Co-authored-by: Yakup Koray Budanaz <budanaz.yakup@gmail.com>
## Description This PR updates `actions/checkout` from `v6` to the latest `v7`. No breakage is expected from reading the change log.
## Summary
`dace.symbolic.simplify` is memoized with `functools.lru_cache`.
`lru_cache`
keys its entries by `hash`/`==`, and in Python booleans *are* integers:
```python
>>> hash(True) == hash(1)
True
>>> True == sympy.Integer(1)
True
```
As a result the cache cannot tell `simplify(True)` apart from
`simplify(sympy.Integer(1))` — they collapse onto a single entry.
Whichever is
evaluated first wins, and the other caller gets the wrong-typed result:
```python
>>> from dace.symbolic import simplify
>>> simplify(True) # caches sympy.true under a key equal to 1
True
>>> simplify(sympy.Integer(1)) # cache hit -> returns the *boolean*
True # expected: 1 (a sympy.Integer)
>>> type(simplify(sympy.Integer(1)))
<class 'sympy.logic.boolalg.BooleanTrue'>
```
The same conflation applies to `False` / `Integer(0)`.
## Impact
The corruption is **process-global and order dependent**: any code path
that
feeds a plain Python `bool` to `simplify` (for example a comparison like
`simplify(s == 1)`, which evaluates to `True`/`False` before reaching
`simplify`) poisons the cache for every later
`simplify(sympy.Integer(1))` /
`simplify(sympy.Integer(0))`.
Downstream, this surfaces as a hard crash in memlet-volume propagation.
For a
single-iteration map the propagated volume is `sympy.Integer(1)`, so
`propagate_subset` (`dace/sdfg/propagation.py`) calls
`simplify(Integer(1))`,
gets back `BooleanTrue`, and the `Memlet.volume` setter rejects it:
```
TypeError: Property volume must be a literal or symbolic expression,
got: <class 'sympy.logic.boolalg.BooleanTrue'>
```
Because it depends on evaluation order across the whole process, the
failure is
intermittent and hard to attribute — an unrelated SDFG breaks long after
the
`bool` was simplified.
## Fix
Construct the cache with `typed=True` so arguments of different types
are cached
under distinct entries, even when they hash and compare equal:
```diff
-@lru_cache(maxsize=2048)
+@lru_cache(maxsize=2048, typed=True)
def simplify(expr: SymbolicType) -> SymbolicType:
return sympy.simplify(expr)
```
With `typed=True`, `simplify(True)` (a `bool`) and
`simplify(sympy.Integer(1))`
(a `sympy.Integer`) occupy separate cache slots, so neither can return
the
other's result.
## Testing
New regression test: `tests/simplify_cache_typed_test.py`. It covers:
- `simplify(True)` then `simplify(Integer(1))` (and the reverse) stay
correctly
typed — both directions, for `True`/`1` and `False`/`0`.
- Normal symbolic expressions still simplify and are still served from
the cache
(`cache_info().hits`).
- End-to-end: after a `bool` is passed to `simplify`, memlet-volume
propagation
over a single-iteration map still succeeds (this reproduced the original
`TypeError` verbatim before the fix).
---------
Co-authored-by: Yakup Koray Budanaz <ybudanaz@ethz.ch>
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
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.
Just a basis for further discussions.