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

framecode: Fix missing BasicCFBlock argument #1630

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

iBug
Copy link
Contributor

@iBug iBug commented Aug 26, 2024

Updated for v0.16: The (renamed) BasicCFBlock class still requires 4 arguments in a different order and with none being optional. The new __init__ method as provided by @dataclass looks like this:

def __init__(self,
    dispatch_state: Callable[[SDFGState], str],  # from ControlFlow
    parent: Optional['ControlFlow'],  # from ControlFlow
    last_block: bool,  # from ControlFlow
    state: SDFGState,
)

The current code still supplies 3 arguments and in a wrong order. This PR fixes that.

Original PR description (when I was still running on DaCe v0.15.1) follows.


With optimizer.detect_control_flow == False, this part of code causes an error later on:

  File "/home/ibug/examples/dace/dace/codegen/control_flow.py", line 221, in as_cpp
    expr += elem.as_cpp(codegen, symbols)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ibug/examples/dace/dace/codegen/control_flow.py", line 128, in as_cpp
    sdfg = self.state.parent
           ^^^^^^^^^^^^^^^^^
AttributeError: 'bool' object has no attribute 'parent'

I identified this as cflow.SingleState requiring 4 arguments to its __init__ method with the last one being optional, i.e.:

def __init__(self,
    dispatch_state: Callable[[SDFGState], str],  # from ControlFlow
    parent: Optional['ControlFlow'],  # from ControlFlow
    state: SDFGState,
    last_state: bool = False,
)

The current code incorrectly feeds 3 and did not trigger a TypeError due to the last one having a default value.

This PR adds back the missing parent argument, although I'm not sure if the sdfg object is correct. Local testing shows that None suffices, though.

@iBug iBug marked this pull request as draft August 26, 2024 08:38
@iBug
Copy link
Contributor Author

iBug commented Aug 26, 2024

Looks like I was running against DaCe 0.15, I'll need to take a closer look.

@iBug iBug marked this pull request as ready for review August 26, 2024 08:49
@tbennun
Copy link
Collaborator

tbennun commented Aug 29, 2024

@iBug Thank you for the report! I am not sure what caused this, as I use this mode often (but maybe before the latest release). @phschaad does the PR makes sense?

@tbennun tbennun requested a review from phschaad August 29, 2024 20:20
Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix absolutely makes sense, thank you for catching and fixing this! LGTM

@phschaad phschaad added this pull request to the merge queue Aug 30, 2024
@iBug iBug changed the title framecode: Fix missing SingleState argument framecode: Fix missing BasicCFBlock argument Aug 30, 2024
Merged via the queue into spcl:master with commit 8521f40 Aug 30, 2024
10 checks passed
@iBug iBug deleted the framecode-fix branch August 30, 2024 09:50
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 this pull request may close these issues.

3 participants