Add format strings to assert and assume #3802

uenoku · 2024-01-31T13:15:43Z

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

This supports format strings for assert and assume statements as proposed in chipsalliance/firrtl-spec#166. This change might break existing code if message contains %.

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.

linux-foundation-easycla · 2024-01-31T13:15:50Z

The committers listed above are authorized under a signed CLA.

✅ login: uenoku / name: Hideto Ueno (828aec3, 78c751f, 0936478, 2f053bc)

This supports format strings for assert and assume statements. Internal representaion of `Verification` object is modified in a similar way to `Print`. Cover cannot be used with a format string so conversion raires an exception when we try constructing a cover statement with a format string.

sequencer · 2024-01-31T17:03:47Z

The Panama part lgtm. Although we don’t have enough test for now.

dtzSiFive

LGTM, but may be good to get another approval 👍 .

seldridge

This looks good. I'm requesting @jackkoenig to take a look for some of the data class pieces.

seldridge · 2024-02-01T15:45:14Z

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

+      if (op == Formal.Cover && args.nonEmpty) {
+        throwException("cover message cannot be used as a format string")
+      }


This can be done as a require inside the constructor of the Verification op. (Arguably that motivates splitting this into separate IR structures for assert, assume, and cover, but that's not necessary.)

seldridge · 2024-02-01T15:46:44Z

firrtl/src/main/scala/firrtl/ir/IR.scala

@@ -436,6 +436,7 @@ object Formal extends Enumeration {
  pred: Expression,
  en:   Expression,
  msg:  StringLit,
+  args: Seq[Expression],


If we're doing this the data class way, then this needs an @since(...). @jackkoenig: how do you want to handle this?

We've deprecated everything in package firrtl._ and we should probably make it all private as part of Chisel 7 so I'm fine with just making the breaking change. WIth that, I'm fine with this as is, or we can change it back to a case class which would let us delete the unapply (and companion object) rather than updating it.

WDYT?

I greatly prefer making all this private!

seldridge · 2024-02-01T15:46:58Z

firrtl/src/main/scala/firrtl/ir/IR.scala

+  def unapply(
+    s: Verification
+  ): Some[(Formal.Value, Info, Expression, Expression, Expression, StringLit, Seq[Expression])] = {
+    Some((s.op, s.info, s.clk, s.pred, s.en, s.msg, s.args))


@jackkoenig: is this necessary to update for data class?

If we keep the data class (implying we're trying to maintain source and binary compatibility), we should not update this (but should instead deprecate it). As is, this is making a breaking change, but I support just breaking it, see my other comment.

jackkoenig

I approve but please just change @data class Verification to a case class. To do this, replace @data with case, delete @since, delete copy, and delete the companion object and unapply method.

jackkoenig · 2024-02-16T17:45:28Z

firrtl/src/main/scala/firrtl/ir/IR.scala

+  def unapply(
+    s: Verification
+  ): Some[(Formal.Value, Info, Expression, Expression, Expression, StringLit, Seq[Expression])] = {
+    Some((s.op, s.info, s.clk, s.pred, s.en, s.msg, s.args))


If we keep the data class (implying we're trying to maintain source and binary compatibility), we should not update this (but should instead deprecate it). As is, this is making a breaking change, but I support just breaking it, see my other comment.

jackkoenig · 2024-02-16T17:46:28Z

firrtl/src/main/scala/firrtl/ir/IR.scala

@@ -436,6 +436,7 @@ object Formal extends Enumeration {
  pred: Expression,
  en:   Expression,
  msg:  StringLit,
+  args: Seq[Expression],


We've deprecated everything in package firrtl._ and we should probably make it all private as part of Chisel 7 so I'm fine with just making the breaking change. WIth that, I'm fine with this as is, or we can change it back to a case class which would let us delete the unapply (and companion object) rather than updating it.

WDYT?

uenoku · 2024-02-22T11:57:38Z

@jackkoenig @seldridge Could you merge this? Thanks!

This supports format strings for assert and assume statements. Internal representaion of `Verification` object is modified in a similar way to `Print`. Cover cannot be used with a format string so conversion raires an exception when we try constructing a cover statement with a format string.

uenoku force-pushed the dev/hidetou/assert branch from 9e98628 to d5ea924 Compare January 31, 2024 13:19

uenoku force-pushed the dev/hidetou/assert branch from d5ea924 to 78c751f Compare January 31, 2024 13:22

Add panama changes

828aec3

uenoku force-pushed the dev/hidetou/assert branch from d2cf102 to 828aec3 Compare January 31, 2024 13:53

seldridge added the Backend Code Generation Affects backend code generation, will be included in release notes label Jan 31, 2024

dtzSiFive approved these changes Feb 1, 2024

View reviewed changes

seldridge approved these changes Feb 1, 2024

View reviewed changes

Move op verification to the constructor

2f053bc

uenoku force-pushed the dev/hidetou/assert branch from 98b677d to 2f053bc Compare February 16, 2024 14:25

jackkoenig approved these changes Feb 16, 2024

View reviewed changes

Make Verification case class

0936478

uenoku requested a review from jackkoenig February 22, 2024 11:57

jackkoenig merged commit d565ceb into chipsalliance:main Feb 22, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add format strings to assert and assume #3802

Add format strings to assert and assume #3802

uenoku commented Jan 31, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 31, 2024 •

edited

Loading

sequencer commented Jan 31, 2024

dtzSiFive left a comment

seldridge left a comment

seldridge Feb 1, 2024

seldridge Feb 1, 2024

jackkoenig Feb 16, 2024

seldridge Feb 16, 2024

seldridge Feb 1, 2024

jackkoenig Feb 16, 2024

jackkoenig left a comment

jackkoenig Feb 16, 2024

jackkoenig Feb 16, 2024

uenoku commented Feb 22, 2024

Add format strings to assert and assume #3802

Add format strings to assert and assume #3802

Conversation

uenoku commented Jan 31, 2024 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

linux-foundation-easycla bot commented Jan 31, 2024 • edited Loading

sequencer commented Jan 31, 2024

dtzSiFive left a comment

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku commented Feb 22, 2024

uenoku commented Jan 31, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 31, 2024 •

edited

Loading