RTL codegen "line" error #1403
Merged
RTL codegen "line" error #1403
Conversation
This file contains 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
Should be fixed properly.
…ions which target mode they're inteded for.
tbennun
requested changes
Oct 18, 2023
dace/codegen/targets/rtl.py
Outdated
Comment on lines
105
to
108
| elif isinstance(arr, data.Stream): | ||
| # TODO Streams are currently unsupported, as the proper behaviour has to be implemented to avoid deadlocking. | ||
| line: str = "// Unsupported read from ({}) variable '{}' from stream '{}'".format( | ||
| dst_node.in_connectors[edge.dst_conn].ctype, edge.dst_conn, edge.src_conn) |
There was a problem hiding this comment.
Is it not better to raise a verbose error or at least a warning in this case? It will likely produce erroneous code.
There was a problem hiding this comment.
An error no, as the RTL backend is used by the Xilinx backend, which may hit this case without using the faulty code. I've further elaborated this in the comment. I've added the information as a warning as well.
Comment on lines
2
to
5
| """ | ||
| Two sequential RTL tasklets connected through a memlet. | ||
| """ | ||
| # Copyright 2019-2023 ETH Zurich and the DaCe authors. All rights reserved. | ||
| # | ||
| # Two sequential RTL tasklets connected through a memlet. | ||
| # | ||
| # It is intended for running simulation xilinx targets. |
There was a problem hiding this comment.
The previous docstring version is the standard way of defining a file.
There was a problem hiding this comment.
Ah, right, I see! I've reverted them "back" to this format! :)
tbennun
approved these changes
Oct 19, 2023
Closed
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.
The issue in #1383 uncovered flaws in the RTL samples:
The specific error arose when an RTL tasklet tried to access a stream during simulation mode, which is currently not supported in simulation mode, as the produced Verilator code does not mimick actual behaviour, resulting in a deadlock. Current "fix" is to emit this shortcoming as a comment in the resulting code. It should not always fail (hence the emission to a comment), since the RTL code generator is partially used by the Xilinx code generator, leading to said faulty code to be omitted anyways; at least for the samples/tests that reach this point.
Some of the samples did correctly use
with dace.config.set_temporary(), forcing them to run as they're inteded to be run. This has been changed, so all RTL samples run in their respective modes. Furthermore, some of the samples did not have this requirement mentioned in their comments, which has also been fixed.