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.

Sign up for GitHub

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

Jump to bottom

[FIRRTL][SFCCompat] Fix tests and handling of fullasyncreset on non-port. #6984

Merged
merged 4 commits into from
May 7, 2024
Merged

[FIRRTL][SFCCompat] Fix tests and handling of fullasyncreset on non-port. #6984

merged 4 commits into from
May 7, 2024

Conversation

dtzSiFive
Copy link
Contributor

Fix test and extend to cover non-port case:

  • Fix async-reset-anno test to fail with its backing change reverted, as-is the passes weren't run and the FileCheck command was wrong for the "did it remove the annotation" check.
  • Extend async-reset-anno.fir and async-reset.fir tests to cover use of the fullasyncreset annotation on non-ports, which presently fails.

Fix handling of non-port fullasyncreset in SFCCompat (including annotation removal)

  • Modify SFCCompat to look for (and remove) fullasyncreset annotations anywhere, not limited to ports.

cc #6912 .

uenoku
uenoku approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM

lib/Dialect/FIRRTL/Transforms/SFCCompat.cpp Outdated Show resolved Hide resolved
@dtzSiFive
[FIRRTL] Fix async-reset-anno test to run passes at all, test non-port.
db98114 
Non-port usage is not handled presently, test fails.
@dtzSiFive
[FIRRTL] async-reset.fir: Check handling of fullasync on non-port.
2c3b166 
Fails for now.
@dtzSiFive
[FIRRTL][SFCCompat] Fix behavior w/fullasyncreset on non-port.
aedb251 
They are not limited to appearing on module ports.
Compute presence of full-async-reset accordingly,
and be sure to remove those annotations as well.
@dtzSiFive
[FIRRTL][SFCCompat] Always walk and remove all fullasync annos.
e8f5c9a
@dtzSiFive dtzSiFive force-pushed the feature/full-async-reset-non-port-fix branch from bef99bf to e8f5c9a Compare May 7, 2024 18:31
@dtzSiFive dtzSiFive merged commit db4f641 into llvm:main May 7, 2024
4 checks passed
@dtzSiFive dtzSiFive deleted the feature/full-async-reset-non-port-fix branch May 7, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uenoku uenoku uenoku approved these changes

@darthscsi darthscsi Awaiting requested review from darthscsi darthscsi is a code owner

@seldridge seldridge Awaiting requested review from seldridge seldridge is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dtzSiFive @uenoku