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

Remove KnownDomain preparation #2590

Merged
merged 6 commits into from Nov 2, 2023
Merged

Conversation

leonschoorl
Copy link
Member

@leonschoorl leonschoorl commented Oct 16, 2023

In preparation for a 1.8 release before merging #2589, this contains the non backwards compatible API changes of #2589.

And some minor cleanup and improvements (Cleanup clash-lib.cabal and Improve blackbox error message)

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@@ -1276,7 +1276,8 @@ unsafeFromReset (Reset r) = r
-- __NB__: You probably want to use 'unsafeFromActiveLow' or
-- 'unsafeFromActiveHigh'.
unsafeToReset
:: Signal dom Bool
:: KnownDomain dom
Copy link
Member

Choose a reason for hiding this comment

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

The KnownDomain constraint is unused. Isn't GHC technically allowed to prune it?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's okay here. As I understand it, this is purely to make the API change now already for 1.8.0 so we can change the implementation later on. So only the user-facing API is what matters here, GHC can prune it without problems. Once the underlying code changes, the constraint will be needed, and it will no longer be pruned.

Huh. Even in general I think it's okay if it is pruned. The constraint is only needed in Haskell, and GHC can do what it wants as long as the code continues to work. We worry about stuff being pruned because we'd like to access it during HDL generation, but HDL generation doesn't need the KnownDomain for this function, it's a wire, nothing more.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, you're right

Copy link
Member

@christiaanb christiaanb left a comment

Choose a reason for hiding this comment

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

LGTM!

@leonschoorl leonschoorl added this to the 1.8 milestone Oct 25, 2023
@leonschoorl leonschoorl force-pushed the remove-KnownDomain-preparation branch 2 times, most recently from b6a566f to 0b26f7c Compare October 25, 2023 15:57
This changes the error when using non-existant arguments in blackboxes 
from:

  Blackbox required at least 32 arguments, but only 8 were passed.

to the new error:

  Blackbox used "~ISSYNC[31]" , but only 8 arguments were passed.
So it'll be easier to keep the temp directory
You can now use ~PERIOD, ~ISSYNC, ~ISINITDEFINED and ~ACTIVEEDGE
on arguments of type Clock,Reset,Enable,ClockN and DiffClock.
You don't need to provide a KnownDomain anymore
In preparation for #2589 "Remove KnownDomain".
This is the only constraint addition in clash-prelude in for the KnownDomain removal,
by adding it here for 1.8 we can do the KnownDomain removal later,
with less backwards compatibility issues.

This commit also had to add a KnownDomain to invertReset,
because it used unsafeToReset, but this will be removed again by #2589.
@leonschoorl leonschoorl merged commit d43460b into master Nov 2, 2023
13 checks passed
@leonschoorl leonschoorl deleted the remove-KnownDomain-preparation branch November 2, 2023 16:27
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.

None yet

4 participants