Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scalar to symbol promotion fails if scalar is read in another state #1727

Closed
romanc opened this issue Nov 4, 2024 · 6 comments · Fixed by #1766
Closed

Scalar to symbol promotion fails if scalar is read in another state #1727

romanc opened this issue Nov 4, 2024 · 6 comments · Fixed by #1766
Milestone

Comments

@romanc
Copy link
Contributor

romanc commented Nov 4, 2024

Describe the bug

In the attached (see below) generated SDFG, I have the following pattern (see screenshot below)

  • State 1: Evaluate (boolean) condition inside a Tasklet, write the condition to a transient scalar.
  • State 2: Read generated transient scalar into another transient boolean that is manually promoted to a symbol.
  • State 3 (onwards): The symbol is now used for if / else branching.

This triggers scalar to symbol promotion on the evaluation scalar maks_* as well as on the intermediate boolean if_expression_*. It looks like mask_* is processed twice (once per state) and scalar to symbol promotion then fails when mask_* is processed in the second state (again).

To Reproduce

Steps to reproduce the behavior:

  1. Load the attached (see below) SDFG (original-generated.sdfg)
  2. Run simplify() on that SDFG
  3. See error networkx.exception.NetworkXError: nbunch is not a node or a sequence of nodes

Expected behavior

I expect scalar to symbol pass to fail gracefully in case it can't promote a scalar.

Screenshots

image

Desktop (please complete the following information):

  • OS: Linux (OpenSUSE)
  • Dace version: rev 945b5ce

Additional context

Zip file with SDFGs scalar-to-symbol.zip contains:

  • original generated SDFG
  • SDFG right before scalar to symbol pass
  • SDFG after scalar to symbol pass failed

Quickfix in main...romanc:romanc/scalar-to-symbol seems to work in my simple test case. However, not sure about this approach. I might just be treating symptoms without understanding the problem.

Last part of the stack trace, tracing the problem back to

image

I search the open issues and found #1129 and #1665 which might be related.

@phschaad
Copy link
Collaborator

The two issues #1129 and #1665 appear to be unrelated to the issue at hand here after investigation. However, I can say that the quickfix you pointed to actually seems quite reasonable. I am making a PR out of it to see if it passes tests.

@phschaad
Copy link
Collaborator

Scratch that, the quickfix does not get close to passing the tests. We need to investigate this further.

@romanc
Copy link
Contributor Author

romanc commented Nov 14, 2024 via email

@romanc
Copy link
Contributor Author

romanc commented Nov 15, 2024

As mentioned to @tbennun through other channels, I wasn't able to reproduce this issue with the plain SDFG API. I can confirm the issue still persists with the latest (commit c83f601) version of the 1.0 branch though.

I did some digging and when I am looking at the scalars that are to be promoted, I see the following in the debugger

image

Is it expected that maks_* is a symbol reference while if_condition_* is just a string?

@tbennun
Copy link
Collaborator

tbennun commented Nov 15, 2024

What is a SymbolRef? I don't think that's a supported object, as the function expects a set of strings. Who is creating that?

@romanc
Copy link
Contributor Author

romanc commented Nov 15, 2024

What is a SymbolRef? I don't think that's a supported object, as the function expects a set of strings. Who is creating that?

Just for (later) reference: SymbolRefs are a concept of gt4py (and in there the eve framework). It's basically a string with the constraint that it must reference a SymbolName, which - in turn - is just a wrapper around plain strings that enforces a certain regex.

SymbolRefs and SymbolNames should work just as normal strings because they are actually subclassing the str class, see https://github.com/GridTools/gt4py/blob/ea8d9dbfefa16ac71dee5afa48367d7018861721/src/gt4py/eve/type_definitions.py#L87-L104 for reference (if you are interested).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants