Enable user-defined map parameter types #1640

ThrudPrimrose · 2024-09-05T15:33:28Z

Adding param_types variable I can set the loop params to what I want.

I also remove the "auto" type to prefer the inferred type.

Using the inferred type instead of auto fixes the discrepancy of C++/CUDA inferred auto type and the inferred type between DaCe, where the discrepancy causes unnecessary implicit type conversion, resulting with a loss of performance.

…use auto in the for loop generated from the map

tbennun · 2024-09-05T16:07:42Z

dace/codegen/targets/cpu.py

+                if var in map_param_types.keys():
+                    var_type = map_param_types[var]
+                else:
+                    var_type = dtypes.result_type_of(infer_expr_type(r[0], sdfg.symbols), infer_expr_type(r[1], sdfg.symbols))


I think it's better to use symbols_defined_at

dace/codegen/targets/cpu.py

dace/sdfg/nodes.py

tbennun · 2024-09-05T16:08:36Z

dace/sdfg/nodes.py

@@ -793,7 +793,10 @@ def new_symbols(self, sdfg, state, symbols) -> Dict[str, dtypes.typeclass]:
        result = {}
        # Add map params
        for p, rng in zip(self._map.params, self._map.range):
-            result[p] = dtypes.result_type_of(infer_expr_type(rng[0], symbols), infer_expr_type(rng[1], symbols))
+            if p in self.map.param_types.keys():
+                result[p] = dtypes.typeclass(self.map.param_types[p])


why isn't the value type typeclass?

tbennun · 2024-09-05T16:08:56Z

dace/sdfg/nodes.py

@@ -877,6 +880,7 @@ class Map(object):
    # List of (editable) properties
    label = Property(dtype=str, desc="Label of the map")
    params = ListProperty(element_type=str, desc="Mapped parameters")
+    param_types = DictProperty(key_type=str, value_type=str, desc="Types of mapped parameters")


shouldn't param types have a typeclass value?

I had saved the typestrings set by the transformation as a Dict(str,str), such that if a user wants to give the type info as a part of a transformation then the user can just write the str, but in a sense, having the type be a typeclass has information value in the "param_types" parameter, I will update.

tbennun · 2024-09-05T16:09:03Z

dace/sdfg/nodes.py

+    gpu_force_syncthreads = Property(dtype=bool, desc="Force a call to the __syncthreads for the map", default=False)
+


Must have sneaked in from the other PR, will delete it

Co-authored-by: Tal Ben-Nun <[email protected]>

ThrudPrimrose · 2024-09-06T08:12:10Z

I also need to check why the param type can be none sometimes (some tests failing)

ThrudPrimrose · 2024-09-06T10:08:32Z

There is one issue that I am currently trying to solve.
I can know change the map parameters' types, and can set them to be for example int, but I am having problems implementing the calls to the nested SDFGs seeing the correct types.

(I need to find a way for the method "new_symbols" of an interstate edge not to infer the type of the parameters if the parameter was introduced by a map parameter set by the user)

…rams

tbennun

Good addition, thank you! Two things are missing:

My comment on symbols_defined_at (which should be computed as seldomly as possible)
Missing unit test with types (maybe even an additional test that would fail with auto, as you now changed the behavior of codegen even in the default case)

Also, you do not have to keep merging with master, the merge queue will do that for you. Every merge puts an unnecessary burden on the CI queue.

tbennun · 2024-09-15T06:52:30Z

dace/codegen/targets/cpu.py

+                    var_type = dtypes.result_type_of(infer_expr_type(r[0], state_dfg.symbols_defined_at(node)), 
+                                                     infer_expr_type(r[1], state_dfg.symbols_defined_at(node)))


Please compute this once, preferably outside the loop of node.map.range. symbols_defined_at can be an expensive function. Thanks!

tbennun · 2024-09-15T06:53:04Z

dace/sdfg/state.py

@@ -1601,6 +1601,7 @@ def add_nested_sdfg(
        schedule=dtypes.ScheduleType.Default,
        location=None,
        debuginfo=None,
+        symbol_type_mapping: Dict[str, dtypes.typeclass] = None


Suggested change

symbol_type_mapping: Dict[str, dtypes.typeclass] = None

symbol_type_mapping: Dict[str, dtypes.typeclass] = None,

ThrudPrimrose · 2024-09-16T12:31:30Z

Good addition, thank you! Two things are missing:
1. My comment on symbols_defined_at (which should be computed as seldomly as possible)

2. Missing unit test with types (maybe even an additional test that would fail with `auto`, as you now changed the behavior of codegen even in the default case)
Also, you do not have to keep merging with the master; the merge queue will do that for you. Every merge puts an unnecessary burden on the CI queue.

I will update the changes according to your comments. Thanks for pointing out the merge queue.

Since auto was only used in a loop, the type of the loop variable depends on the type used in the loop condition. The auto case will never produce code that does not compile unless we compile with -Wconversion -Werror.
However, I can create test cases where the SDFG will produce incorrect results. I will do that (and try to find couple of good test cases).

I have looked at the tests. No hard-coded SDFGs are used in the tests. I should create any SDFG I use through the API or a front-end, right?

tbennun · 2024-09-16T15:59:17Z

@ThrudPrimrose First of all, many tests use the SDFG API (e.g., add_edge_pair_test.py, you should use that.

Second, for a wrong test case, a simple for loop with a mismatch in start/end types will run forever if not written correctly. See the following example (which one can write in dace): https://godbolt.org/z/YW5xsP51x

ThrudPrimrose · 2024-09-18T11:07:53Z

Since there is no auto type, it is hard to create one that fails due to auto.
When I tried with auto, having auto I = 0; I <N with N being an unsigned long long variable results with I being an int and causing issues.

I can easily create a test that mimics this behaviour by forcing the type of the variable. Then I can run it with subprocess and check for segfaults, should I do that or these tests should be enough?

tbennun · 2024-10-10T14:52:32Z

What I meant was to have a test that would fail if we generated auto. Obviously all the tests will now pass. :)

Enable user-defined map parameter types, infer if not set and do not …

e32d1da

…use auto in the for loop generated from the map

ThrudPrimrose changed the title ~~Enable user-defined map parameter types, infer if not set and do not …~~ Enable user-defined map parameter types Sep 5, 2024

tbennun reviewed Sep 5, 2024

View reviewed changes

ThrudPrimrose and others added 2 commits September 6, 2024 10:09

Update dace/sdfg/nodes.py

0cf56c8

Co-authored-by: Tal Ben-Nun <[email protected]>

Update dace/codegen/targets/cpu.py

b25b95d

Co-authored-by: Tal Ben-Nun <[email protected]>

ThrudPrimrose added 4 commits September 6, 2024 14:06

Update according to the comments

54e29cc

Merge branch 'map_params' of https://github.com/spcl/dace into map_pa…

aa21812

…rams

Remove the rogue gpu force syncthreads parameter (again)

a4079ac

Merge branch 'master' into map_params

88d5a07

ThrudPrimrose marked this pull request as ready for review September 9, 2024 08:53

ThrudPrimrose marked this pull request as draft September 9, 2024 09:39

ThrudPrimrose marked this pull request as ready for review September 11, 2024 12:00

ThrudPrimrose added 2 commits September 11, 2024 14:00

Merge branch 'master' into map_params

054028c

Merge branch 'master' into map_params

3f6a081

tbennun requested changes Sep 15, 2024

View reviewed changes

ThrudPrimrose added 3 commits September 18, 2024 12:41

Add unit tests

696f5bc

Reduce number of calls to symbols_defined_at

98655d6

Add sub variants

8be0fba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable user-defined map parameter types #1640

Enable user-defined map parameter types #1640

ThrudPrimrose commented Sep 5, 2024 •

edited

Loading

tbennun Sep 5, 2024

tbennun Sep 5, 2024

tbennun Sep 5, 2024

ThrudPrimrose Sep 6, 2024

tbennun Sep 5, 2024

ThrudPrimrose Sep 6, 2024

ThrudPrimrose commented Sep 6, 2024

ThrudPrimrose commented Sep 6, 2024

tbennun left a comment

tbennun Sep 15, 2024

tbennun Sep 15, 2024

ThrudPrimrose commented Sep 16, 2024 •

edited

Loading

tbennun commented Sep 16, 2024

ThrudPrimrose commented Sep 18, 2024

tbennun commented Oct 10, 2024

		gpu_force_syncthreads = Property(dtype=bool, desc="Force a call to the __syncthreads for the map", default=False)

		var_type = dtypes.result_type_of(infer_expr_type(r[0], state_dfg.symbols_defined_at(node)),
		infer_expr_type(r[1], state_dfg.symbols_defined_at(node)))

	symbol_type_mapping: Dict[str, dtypes.typeclass] = None
	symbol_type_mapping: Dict[str, dtypes.typeclass] = None,

Enable user-defined map parameter types #1640

Are you sure you want to change the base?

Enable user-defined map parameter types #1640

Conversation

ThrudPrimrose commented Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThrudPrimrose commented Sep 6, 2024

ThrudPrimrose commented Sep 6, 2024

tbennun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThrudPrimrose commented Sep 16, 2024 • edited Loading

tbennun commented Sep 16, 2024

ThrudPrimrose commented Sep 18, 2024

tbennun commented Oct 10, 2024

ThrudPrimrose commented Sep 5, 2024 •

edited

Loading

ThrudPrimrose commented Sep 16, 2024 •

edited

Loading