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 unused argument warnings on writeToBiSignal# #2822

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

leonschoorl
Copy link
Member

Any usage of writeToBiSignal would result in:

Warning: The Haskell implementation of primitive Clash.Signal.BiSignal.writeToBiSignal# isn't using argument #1, but the corresponding primitive blackbox does.
This can lead to incorrect HDL output because GHC can replace these arguments by an undefined value.

Warning: The Haskell implementation of primitive Clash.Signal.BiSignal.writeToBiSignal# isn't using argument #3, but the corresponding primitive blackbox does.
This can lead to incorrect HDL output because GHC can replace these arguments by an undefined value.

Warning: The Haskell implementation of primitive Clash.Signal.BiSignal.writeToBiSignal# isn't using argument #4, but the corresponding primitive blackbox does.
This can lead to incorrect HDL output because GHC can replace these arguments by an undefined value.

This fixes that with some bang patterns on writeToBiSignal#.

I was a little afraid this might result in loops in haskell simulation.
But I've tested the manually test all three Counter* tests in Haskell simulation both inside clashi, and when compiled to executables.

Still TODO:

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

@leonschoorl
Copy link
Member Author

leonschoorl commented Oct 7, 2024

Mmm, my fear wasn't unfounded after all.
It is too strict for the clash-cores SPI test, but only using when ghc-9.0.

But Christiaans suggestion fixes the problem.

@leonschoorl leonschoorl force-pushed the fix-writeToBiSignal-warnings branch from b4dd6b8 to 9e74cc0 Compare October 7, 2024 15:48
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

@christiaanb christiaanb merged commit a532ef0 into master Oct 9, 2024
13 checks passed
@christiaanb christiaanb deleted the fix-writeToBiSignal-warnings branch October 9, 2024 11:08
mergify bot pushed a commit that referenced this pull request Oct 9, 2024
leonschoorl added a commit that referenced this pull request Oct 9, 2024
(cherry picked from commit a532ef0)

Co-authored-by: Leon Schoorl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants