Better mangeling of the state struct in the code generator #1413
Conversation
…ses issue spcl#1396) Before the type name of the sdfg struct was just `f'{sdfg.name}_t'` which lead to some issue. This commit introduces the `mangle_dace_state_struct_name()` function which is used to derive the name. I hope that I have found all locations where it is used.
I did not realize that not all code is generated using f-strings but were using `.format`.
Thus it was not possible to evaluate a function inside `{}` only keymatching.
I am pretty sure that I got all.
|
According to the log pytest tests/python_frontend/indirections_test.py::test_indirection_scalar_range_nsdfg |
… (addresses issue spcl#1396)" This reverts all of my changes I hope that now the issue is no longer triggered.
This has become a common issue in the past few days. It fails on the same test configuration every time, if it fails. For now just restarting the failing workflow works, but we have to look into this issue further. |
Now lets activate them again and see whats happens. It is interesting, since this only changes how the name of the state struct is generated, it should not have any influence on it. However as [Philip Schaad](spcl#1413 (comment)) pointed out this seems a common issue.
Thanks for the context. I am tracking this here #1421. I initiated a re-run for the tests. |
|
@BenWeber42 Interestingly rebooting even works on non Windows machines. |
|
I am not sure we should make this change. Several codes depend on the existing naming convention, and the benefit is only for specific names that we should maybe disallow in validation. |
|
Having this function has in my view multiple advantages and I do not find any disadvantages:
Furthermore, while this might not be much of an argument, I learned that: "For everything you do more than once write a function" and we compose this name multiple times. |
I wasn't referring to the function, that part is good. I was referring to the part where we make the code less readable by changing the struct's name. One could add a dace config such as |
|
Oh, you were referring to the generated C++ code. But I really like your idea with the configuration. |
…now be modified by the configuration entry `compiler.codegen_state_struct_suffix`. This change mostly was suggested by [Tal](https://github.com/tbennun).
|
I have implemented your idea, but the default value of the configuration now defaults to By the way would it make sense to move the keys for the code generator to their own hierarchy? |
|
This seems very much like an ad-hoc solution for one instance of a much more general problem. The general problem is that Python identifiers and C++ identifiers (as generated by code-gen) are not the same. A common solution for such a problem is to have a name-mangling system. As far as I know, we currently don't have a proper name mangling system in DaCe (based on I think we should aim towards a general solution to the problem, because DaCe needs this anyway if we want DaCe to be robust. If we start with just an ad-hoc solution, we risk growing a name mangling system that is not suitable to solve the general problem. I think we should take a step back and first consider a general name mangling system for DaCe. Once we have a better view of this, we can think of introducing this step by step. In particular, how should this one bug be fixed, such that the solution facilitates a path towards a more general name mangling system. Regarding the configuration option, my big question is whether our name mangling should be configurable. We could also consider name mangling to be something internal, that shouldn't be configurable. Especially, because it's much easier to later add configuration options than to remove/change them. |
|
@BenWeber42 |
This PR addresses issue #1396, by modifying the way how the name of the state struct is created.