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

[FIRRTL] Add intrinsic for UNR only assume #6867

Merged
merged 5 commits into from Mar 28, 2024

Conversation

uenoku
Copy link
Member

@uenoku uenoku commented Mar 25, 2024

This adds UnclockedAssumeIntrinsicOp which generates a SV assume statement whose predicate is used in a sensitivity list of the enclosing always block.

@uenoku uenoku changed the title [FIRRTL] Add intrinsics for UNR only assume [FIRRTL] Add intrinsic for UNR only assume Mar 25, 2024
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

Generally LGTM!

Consider having this work the same way AssumeOp does re:Fold (?) and InferWidths (so doesn't hit "Default" case and error out? untested).


### circt.assume_edged_predicate

Generate a SV assume statement whose predicate is used in a sensitivity list of the enclosing always block.
Copy link
Contributor

Choose a reason for hiding this comment

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

No clock, that's right. Interesting!

`ifdef USE_UNR_ONLY_CONSTRAINTS
wire _GEN = ~enable | pred;
always @(edge _GEN)
assume_label: assume(_GEN) else $error("Conditional compilation example for UNR-only and formal-only assert");
Copy link
Contributor

Choose a reason for hiding this comment

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

Companion assume's don't have error messages today, but seems reasonable to support on the intrinsic (can use or not when migrating).

Was thinking about implications of not being under a clock in the case when the error has substitution arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah thank you for pointing out. We add %sampled for concurrent assertions but not for immediate assertions but I'm uncertain we need $sampled for this. I'm fine to drop messages for migration.

@uenoku
Copy link
Member Author

uenoku commented Mar 26, 2024

Consider having this work the same way AssumeOp does re:Fold (?) and InferWidths (so doesn't hit "Default" case and error out? untested).

Added, thanks!

@uenoku uenoku force-pushed the dev/hidetou/unr-only-assume-intr branch from 52f5acf to 8598188 Compare March 26, 2024 15:30
@fabianschuiki
Copy link
Contributor

Maybe UnclockedAssumeIntrinsicOp could work? Because the fact that it uses this edge syntax is probably more of an accident than a core feature. always @* assume ... might work just as well 😃

@uenoku
Copy link
Member Author

uenoku commented Mar 27, 2024

UnclockedAssumeIntrinsicOp

Yeah that sounds great!

@uenoku uenoku force-pushed the dev/hidetou/unr-only-assume-intr branch from 1f6f448 to ac45e7c Compare March 28, 2024 07:29
@uenoku uenoku merged commit 1782d5d into main Mar 28, 2024
4 checks passed
@uenoku uenoku deleted the dev/hidetou/unr-only-assume-intr branch March 28, 2024 07:50
uenoku added a commit that referenced this pull request Mar 28, 2024
…ation from AssertOp lowering (#6863)

This PR separates UNROnlyAssume generation from AssertOp lowering into a dedicate op.

This commit adds:
* LowerToHW support for `UnclockedPredicateIntrinsicOp` (added in #6867).
* CreateCompanionAssume pass which explicitly introduces companion assumes to the IR. Normal assume and UnclockedPredicateIntrinsicOp are used based on the guard's content.

The commit is intended not to change semantics of generated verilog. This commit should only introduces cosmetic changes regarding guards(because now they are reused).
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

3 participants