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

Fix problems with the specification from #24 #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jvanstraten
Copy link
Contributor

@jvanstraten jvanstraten commented Mar 10, 2020

The following things have come up that are either broken or aren't very nice about the current specification.

  • Asynchronous signals cannot be reversed (Non-stream signal reversibility #23). Proposed solutions:
    • Make reversal part of fields instead of Stream nodes. (This is problematic because at best it introduces implicit streams again. It also means tools cannot reasonably place registers/clock domain crossings automatically.)
    • Remove the signals from the specification, but add a separate type system (based on Tydi, bit supporting reversal) for user-defined signals.
    • Keep the current specification for signals that tools may insert registers into, but add a separate type system (based on Tydi) for user-defined signals that supports reversal that the tools don't do anything with.
  • Order of asynchronous signals and streams is not maintained.
  • The user-friendly representation notes should be removed. This and any requirements on field names beside uniqueness are entirely in user control. Code generators are considered to be users in this context; this means that generators may impose additional constraints.
  • Add a flag to Stream nodes and to physical streams that specifies whether empty sequences are supported or not.
  • Maybe add a note that Tuple(N, T) may be used as a shorthand for Group(0: T, 1: T, ..., N-1: T).
  • Resolve implications of "asynchronous", because these signals are actually meant to be synchronous with a clock but not any stream handshake.

@jvanstraten jvanstraten added WIP: do not merge Work in progress, do not merge until tag is removed 🐬 specification Item related to the specification labels Mar 10, 2020
@jvanstraten jvanstraten self-assigned this Mar 10, 2020
@jvanstraten
Copy link
Contributor Author

I clarified the original intent of the user-defined signals somewhat (propagation of constant values). I'm still very much in favor for keeping the specification as it is here, and just adding a separate mechanism to the streamlet generation tooling for signals that the tools should not touch.

I'm not sure how to fix the order of the user-defined signals w.r.t. the streams.

@ccromjongh
Copy link
Member

Hey @jvanstraten, we want to update the documentation, and it seems this is work that needs some finishing touches but is mostly done, apart from notions about asynchronous signals. What needs to be done to integrate the updates? Are these changes still correct in 2024?

@jvanstraten
Copy link
Contributor Author

I have absolutely no idea anymore. It's been so long since I did anything in-depth with Tydi that you or whoever else in ABS probably knows better at this point. But knowing past me, I doubt anything in there will be complete nonsense, and anything that is in there just needs a coherency check to make sure no documentation inconsistencies were introduced; otherwise I think I would've put "WIP" in the commit messages as well. Feel free to commandeer this branch and pull request and do with it as you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐬 specification Item related to the specification WIP: do not merge Work in progress, do not merge until tag is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants