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

proc_dff: fix early return bug #4714

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

georgerennie
Copy link
Collaborator

@georgerennie georgerennie commented Nov 6, 2024

Fixes #4712 which is a bug introduced in #4569, where multiple rules in processes were not being considered if an asynchronous one was encountered as the code erroneously returned from the function instead of continuing the processing loop.

* early return caused proc_dff to stop considering rules after seeing
  one async rule - this is because continue should have been used to
  continue to procecssing the next rule instead of returning from the
  function
Copy link
Collaborator

@widlarizer widlarizer left a comment

Choose a reason for hiding this comment

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

Test reproduces unfixed behavior, fix looks good

@gadfort
Copy link
Contributor

gadfort commented Nov 20, 2024

@widlarizer is this good to merge?

@widlarizer
Copy link
Collaborator

Yep, it is. Just to clarify, at least from me, an approval + green CI means that the PR author is free to merge it. I don't like to merge PRs I'm revieweing since the authors might still have something to add or fix. @georgerennie you can go ahead and merge these whenever

@georgerennie
Copy link
Collaborator Author

georgerennie commented Nov 20, 2024

@georgerennie you can go ahead and merge these whenever

I don't have write access to Yosys so I need someone else to merge these. All of the ones you have approved I am happy to be merged - I don't have anything to add

@povik povik merged commit 7ebe451 into YosysHQ:main Nov 20, 2024
26 checks passed
@widlarizer
Copy link
Collaborator

Oh, I didn't realize only maintainers get to merge even maintainer-approved PRs. My bad, just used to a diff setup.

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.

handling of register arrays broken after bdb5d45591d7501825349bedcb2952d0b80a1112
4 participants