Add ifElseFatal intrinsics; use it for chisel3.assert emission #3825

uenoku · 2024-02-16T14:12:30Z

Replaced printf-encoded assertion with ifElseFatal intrinsics[0]. Currently the intrinsic is a private object of core since we don't want to expose it other than chisel3.assert.
Removed outdated comments regarding verification statements.
Added a new case class PrintableParam for lazily constructing a string parameter for format string.

[0] https://github.com/llvm/circt/blob/main/docs/Dialects/FIRRTL/FIRRTLIntrinsics.md#circtchisel_ifelsefatal

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Backend code generation

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Support circt_chisel_ifelsefatal intrinsics and use it for chisel3.assert emission.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

uenoku · 2024-02-29T14:37:18Z

src/test/scala/chiselTests/naming/NamePluginSpec.scala

@@ -75,14 +75,12 @@ class NamePluginSpec extends ChiselFlatSpec with Utils {
      val foo, bar = IO(Input(UInt(8.W)))

      {
-        val x1 = chisel3.assert(1.U === 1.U)


Deleted because a label doesn't make sense for chisel3.assert

Does that change match the firrtl spec (not sure whether asserts having labels ever was in the spec)

The problem is that chisel3.assert never use plain FIRRTL assert. IfElseFatal style assertion doesn't have a label and we have ignored labels. I think it's fair to add a new API for emitting a plain FIRRTL assert and use that API here though.

You're saying it never had a label, but if it didn't, how did this test pass previously?

chisel3.assert emitted an assert op along with print op. The assert op is actually a dummy op deleted by a parser and this test passed because the dummy assert had a label.

core/src/main/scala/chisel3/VerificationStatement.scala

core/src/main/scala/chisel3/experimental/VerificationIntrinsics.scala

src/main/scala/chisel3/stage/ChiselAnnotations.scala

mwachs5 · 2024-02-29T14:49:30Z

src/test/scala/chiselTests/naming/NamePluginSpec.scala

@@ -75,14 +75,12 @@ class NamePluginSpec extends ChiselFlatSpec with Utils {
      val foo, bar = IO(Input(UInt(8.W)))

      {
-        val x1 = chisel3.assert(1.U === 1.U)


Does that change match the firrtl spec (not sure whether asserts having labels ever was in the spec)

core/src/main/scala/chisel3/VerificationStatement.scala

This replaces printf-encoded assertion with ifElseFatal intrinsics. The intrinsic is a private object of core since we don't want to expose it other than chisel3.assert.

core/src/main/scala/chisel3/VerificationStatement.scala

core/src/main/scala/chisel3/experimental/VerificationIntrinsics.scala

core/src/main/scala/chisel3/internal/firrtl/Converter.scala

core/src/main/scala/chisel3/Printable.scala

jackkoenig

A few last nits but looks good

core/src/main/scala/chisel3/Printable.scala

core/src/main/scala/chisel3/BlackBox.scala

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

uenoku · 2024-03-06T14:59:24Z

@jackkoenig Thank you for reviews! Could you merge this?

dtzSiFive

LGTM!

This is a follow-up to chipsalliance#3825. MFC has attached a laebl "chisel3_builtin" to every ifElseFatal assertion. Usually a label on ifElseFatal is just ignored but the label is emitted when compiled with `-emit-chisel-asserts-as-sva`. So labels on assertions converted from ifElseFatal disappear due to chipsalliance#3825. This PR restores labels to preserve the old behavior.

This is a follow-up to #3825. MFC has attached a laebl "chisel3_builtin" to every ifElseFatal assertion. Usually a label on ifElseFatal is just ignored but the label is emitted when compiled with `-emit-chisel-asserts-as-sva`. So labels on assertions converted from ifElseFatal disappear due to #3825. This PR restores labels to preserve the old behavior.

#3825)" This reverts commit 386a7f3.

Revert #3912 and #3825. This reverts commit 35d3b71. This reverts commit 386a7f3.

Restore chipsalliance#3912 and chipsalliance#3825. Use intrinsic expression instead of intrinsic module. This reverts commit 347d82b.

Restore #3912 and #3825. Use intrinsic expression instead of intrinsic module. This reverts commit 347d82b.

jackkoenig added the Feature New feature, will be included in release notes label Feb 16, 2024

uenoku marked this pull request as draft February 16, 2024 19:04

uenoku force-pushed the dev/hidetou/verif-int branch from 6e08b11 to 736d23e Compare February 28, 2024 17:45

uenoku changed the title ~~Support circt_chisel_ifelsefatal intrinsics~~ Add ifElseFatal intrinsics; use it for chisel3.assert emission Feb 28, 2024

uenoku force-pushed the dev/hidetou/verif-int branch 3 times, most recently from 1c7698f to afb4e3f Compare February 29, 2024 14:36

uenoku commented Feb 29, 2024

View reviewed changes

uenoku marked this pull request as ready for review February 29, 2024 14:37

mwachs5 reviewed Feb 29, 2024

View reviewed changes

fabianschuiki reviewed Feb 29, 2024

View reviewed changes

core/src/main/scala/chisel3/VerificationStatement.scala Outdated Show resolved Hide resolved

uenoku force-pushed the dev/hidetou/verif-int branch 2 times, most recently from e45c7af to ddc39b0 Compare March 1, 2024 02:45

Add ifElseFatal intrinsics; use it for chisel3.assert emission

d69c470

This replaces printf-encoded assertion with ifElseFatal intrinsics. The intrinsic is a private object of core since we don't want to expose it other than chisel3.assert.

uenoku force-pushed the dev/hidetou/verif-int branch from ddc39b0 to d69c470 Compare March 1, 2024 03:31

jackkoenig added this to the 7.0 milestone Mar 1, 2024

jackkoenig added Backend Code Generation Affects backend code generation, will be included in release notes and removed Feature New feature, will be included in release notes labels Mar 1, 2024

jackkoenig reviewed Mar 1, 2024

View reviewed changes

core/src/main/scala/chisel3/VerificationStatement.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Mar 1, 2024

View reviewed changes

core/src/main/scala/chisel3/experimental/VerificationIntrinsics.scala Show resolved Hide resolved

jackkoenig reviewed Mar 1, 2024

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/Converter.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Mar 1, 2024

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/Converter.scala Outdated Show resolved Hide resolved

uenoku added 2 commits March 4, 2024 03:36

Address comments

30e6e6d

Rename extract_args and implement it as an abstract method

501b0d1

uenoku requested a review from jackkoenig March 4, 2024 15:03

Use Seq instead of List.

f882fb8

uenoku commented Mar 5, 2024

View reviewed changes

core/src/main/scala/chisel3/Printable.scala Outdated Show resolved Hide resolved

jackkoenig approved these changes Mar 6, 2024

View reviewed changes

uenoku and others added 2 commits March 6, 2024 23:13

Apply suggestions from code review

e266681

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Address comments

7414d38

dtzSiFive approved these changes Mar 6, 2024

View reviewed changes

dtzSiFive merged commit 386a7f3 into chipsalliance:main Mar 6, 2024
14 checks passed

uenoku mentioned this pull request Mar 7, 2024

Add a label "chisel3_builtin" to ifElseFatal intrinsic #3912

Merged

14 tasks

dtzSiFive added a commit that referenced this pull request Mar 7, 2024

Revert "Add ifElseFatal intrinsics; use it for chisel3.assert emission (

ac6e22b

#3825)" This reverts commit 386a7f3.

dtzSiFive added a commit that referenced this pull request Mar 7, 2024

Revert move to intrinsic for assert due to performance impact (#3916)

347d82b

Revert #3912 and #3825. This reverts commit 35d3b71. This reverts commit 386a7f3.

dtzSiFive mentioned this pull request Apr 17, 2024

Add ifElseFatal intrinsics; use it for chisel3.assert emission #4001

Merged

14 tasks

dtzSiFive added a commit that referenced this pull request Apr 24, 2024

Add ifElseFatal intrinsics; use it for chisel3.assert emission (#4001)

22e9d02

Restore #3912 and #3825. Use intrinsic expression instead of intrinsic module. This reverts commit 347d82b.

a0u mentioned this pull request Jul 30, 2024

[cd] Bump CIRCT from firtool-1.75.0 to firtool-1.79.0 (copy #1929) ucb-bar/chipyard#1930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ifElseFatal intrinsics; use it for chisel3.assert emission #3825

Add ifElseFatal intrinsics; use it for chisel3.assert emission #3825

uenoku commented Feb 16, 2024 •

edited

Loading

uenoku Feb 29, 2024

mwachs5 Feb 29, 2024

uenoku Feb 29, 2024

jackkoenig Mar 1, 2024

uenoku Mar 4, 2024

mwachs5 Feb 29, 2024

jackkoenig left a comment

uenoku commented Mar 6, 2024

dtzSiFive left a comment

Add ifElseFatal intrinsics; use it for chisel3.assert emission #3825

Add ifElseFatal intrinsics; use it for chisel3.assert emission #3825

Conversation

uenoku commented Feb 16, 2024 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

uenoku Feb 29, 2024

Choose a reason for hiding this comment

mwachs5 Feb 29, 2024

Choose a reason for hiding this comment

uenoku Feb 29, 2024

Choose a reason for hiding this comment

jackkoenig Mar 1, 2024

Choose a reason for hiding this comment

uenoku Mar 4, 2024

Choose a reason for hiding this comment

mwachs5 Feb 29, 2024

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

uenoku commented Mar 6, 2024

dtzSiFive left a comment

Choose a reason for hiding this comment

uenoku commented Feb 16, 2024 •

edited

Loading