WIP: Update Chisel from 6.5.0 to 7.0.0 #11

tymcauley · 2024-07-16T15:57:16Z

UnknownWidth became a case object in this Chisel PR: chipsalliance/chisel#4242

The connectFromBits method was removed in this Chisel PR: chipsalliance/chisel#4168

The connectFromBits API was replaced with the _fromUInt API in this Chisel PR: chipsalliance/chisel#4782

Wrapping unary negation (unary_-%) was deprecated in this PR: chipsalliance/chisel#4829

Updating to the latest Chisel/Scala compiler version set off a cascade of updates (scalafmt, scalatest, etc). Hopefully that's alright to include in this PR.

Status

I'll keep this in a draft state until Chisel 7 is released, since this is currently pinned to a SNAPSHOT version of Chisel.

tymcauley · 2024-08-20T15:23:39Z

cc @jackkoenig, I brought this up a few weeks ago (regarding the transition from Chisel 6 to 7), but wanted to make sure it was still on your radar.

src/main/scala/fixedpoint/FixedPoint.scala

tymcauley · 2025-03-11T20:29:23Z

Current status, I'm getting two errors due to firtool not being happy with the generated FIRRTL. You can recreate with sbt test or:

$ sbt
...
> testOnly FixedPointSpec

These are the errors I'm seeing:

[info] - Floor operation works *** FAILED ***
[info]   circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /path/to/org.chipsalliance.llvm-firtool/1.109.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+529-5f2e70c3-SNAPSHOT) was published against firtool version 1.109.0.
[info] ------------------------------------------------------------------------------
[info] ExitCode:
[info] 1
[info] STDOUT:
[info]
[info] STDERR:
[info] <stdin>:48:33: error: initializer too wide for declared width
[info]     node _out_floored_T_1 = shr(SInt<8>(0hde), 2) @[src/test/scala/FixedPointSpec.scala 176:9]
[info]                                 ^
[info]
[info] ------------------------------------------------------------------------------
[info]   ...
[info] - Ceil operation works *** FAILED ***
[info]   circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /path/to/org.chipsalliance.llvm-firtool/1.109.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+529-5f2e70c3-SNAPSHOT) was published against firtool version 1.109.0.
[info] ------------------------------------------------------------------------------
[info] ExitCode:
[info] 1
[info] STDOUT:
[info]
[info] STDERR:
[info] <stdin>:141:211: error: initializer too wide for declared width
[info]         intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed: Wrong value: in=FixedPoint( -> %d); out=FixedPoint( -> %d); expected=FixedPoint( -> %d)\n", label = "chisel3_builtin">, clock, _T_2, _T_3, SInt<8>(0hde), out_2, SInt<8>(0he0)) @[src/test/scala/FixedPointSpec.scala 167:13]
[info]                                                                                                                                                                                                                   ^
[info]
[info] ------------------------------------------------------------------------------

Any clues on this @jackkoenig?

Also @seldridge could you please check that I migrated to ChiselSim in a sane way? That's contained in 6fb4668

jackkoenig · 2025-03-11T20:54:53Z

It looks like Chisel just isn't properly validating SInt literals:

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.16"
//> using dep "org.chipsalliance::chisel:7.0.0-M2+533-a7effb7d-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin:7.0.0-M2+533-a7effb7d-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
// _root_ disambiguates from package chisel3.util.circt if user imports chisel3.util._
import _root_.circt.stage.ChiselStage

class Foo extends Module {
  val out = IO(Output(SInt(8.W)))

  out := 0xde.S(8.W)
}

object Main extends App {
  println(
    ChiselStage.emitCHIRRTL(new Foo)
  )
  println(
    ChiselStage.emitSystemVerilog(
      gen = new Foo,
      firtoolOpts = Array("-disable-all-randomization", "-strip-debug-info", "-default-layer-specialization=enable")
    )
  )
}

Chisel is happy to emit that CHIRRTL but firtool rejects it:

FIRRTL version 4.2.0
circuit Foo :
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  public module Foo : @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    input clock : Clock @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    input reset : UInt<1> @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    output out : SInt<8> @[Users/koenig/work/t/sint-lit/chisel-example.scala 12:15]

    connect out, SInt<8>(0hde) @[Users/koenig/work/t/sint-lit/chisel-example.scala 14:7]


Exception in thread "main" circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /Users/koenig/Library/Caches/org.chipsalliance.llvm-firtool/1.109.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+533-a7effb7d-SNAPSHOT) was published against firtool version 1.109.0.
------------------------------------------------------------------------------
ExitCode:
1
STDOUT:

STDERR:
<stdin>:13:18: error: initializer too wide for declared width
    connect out, SInt<8>(0hde) @[Users/koenig/work/t/sint-lit/chisel-example.scala 14:7]
                 ^

------------------------------------------------------------------------------

This used to "work" because Chisel emitted it as an 8-bit unit cast to SInt (which is 9-bits) which then would be implicitly truncated:

FIRRTL version 3.3.0
circuit Foo :
  module Foo : @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    input clock : Clock @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    input reset : UInt<1> @[Users/koenig/work/t/sint-lit/chisel-example.scala 11:7]
    output out : SInt<8> @[Users/koenig/work/t/sint-lit/chisel-example.scala 12:15]

    connect out, asSInt(UInt<8>(0hde)) @[Users/koenig/work/t/sint-lit/chisel-example.scala 14:7]

tymcauley · 2025-03-11T21:06:43Z

Very interesting, looks like this regression happened somewhat recently, since that same code sample works fine on 7.0.0-M2+432-486b55dc-SNAPSHOT.

tymcauley · 2025-03-11T21:11:09Z

Okay, it works on 7.0.0-M2+509-2ec31591-SNAPSHOT, but is broken on 7.0.0-M2+510-c1b51350-SNAPSHOT: chipsalliance/chisel@c1b5135

jackkoenig · 2025-03-11T21:24:46Z

Yes it was chipsalliance/chisel#4748 that exposed this issue, but it's not a regression--it's exposing a bug that has been in Chisel for at least a while, possibly forever.

The observation is that the width should never affect the semantic meaning of a value (as long as the width is large enough to hold it). In Chisel 6.7.0 and I suspect all Chisel prior to the linked PR/commit, this was not true for SInt literals:

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.16"
//> using dep "org.chipsalliance::chisel:6.7.0"
//> using plugin "org.chipsalliance:::chisel-plugin:6.7.0"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
// _root_ disambiguates from package chisel3.util.circt if user imports chisel3.util._
import _root_.circt.stage.ChiselStage

class Foo extends Module {
  val out1 = IO(Output(Bool()))
  val out2 = IO(Output(Bool()))

  val reference = 0xde.S(64.W) // Big enough to easily hold the value
  // Could use assertions but this makes it more obvious in the Verilog
  out1 := reference === 0xde.S
  out2 := reference === 0xde.S(8.W)
}

object Main extends App {
  println(
    ChiselStage.emitSystemVerilog(
      gen = new Foo,
      firtoolOpts = Array("-disable-all-randomization", "-strip-debug-info") //, "-default-layer-specialization=enable")
    )
  )
}

Generates

// Generated by CIRCT firtool-1.62.1-1-gdf5ed6ea5
module Foo(
  input  clock,
         reset,
  output out1,
         out2
);

  assign out1 = 1'h1;
  assign out2 = 1'h0;
endmodule

out2 is incorrect here. I will fix this and backport to 6.x.

It seems likely that FixedPoint is doing this on purpose--using the binary representation of negative SInts--but the API its taking advantage of is fundamentally unsound and needs to be fixed. One can replicate this behavior by going through UInts and then casting to SInt. (EDIT: This used to mention truncating but that actually returns a UInt lol).

0xde.U.asSInt

Or by manipulating the 2s complement value's manually as BigInts.

tymcauley · 2025-03-12T01:31:33Z

Nice, after using a snapshot which includes chipsalliance/chisel#4786, we get this error, which is much nicer to work with:

[info] - Floor operation works *** FAILED ***
[info]   java.lang.IllegalArgumentException: requirement failed: The literal value 222 was elaborated with a specified width of 8 bits, but at least 9 bits are required.
[info]   at ... ()
[info]   at fixedpoint.FixedPoint$$anonfun$apply$1.apply(FixedPoint.scala:83)
[info]   at fixedpoint.FixedPoint$$anonfun$apply$1.apply(FixedPoint.scala:83)
[info]   at chisel3.RecordImpl.$anonfun$_makeLit$16(AggregateImpl.scala:957)
[info]   at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:75)
[info]   at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:35)
[info]   at chisel3.RecordImpl._makeLit(AggregateImpl.scala:957)
[info]   at chisel3.RecordImpl._makeLit$(AggregateImpl.scala:950)
[info]   at chisel3.Record._makeLit(Aggregate.scala:310)
[info]   at chisel3.experimental.package$BundleLiterals$AddBundleLiteralConstructor.Lit(package.scala:133)
[info]   ...
[info] - Ceil operation works *** FAILED ***
[info]   java.lang.IllegalArgumentException: requirement failed: The literal value 222 was elaborated with a specified width of 8 bits, but at least 9 bits are required.
[info]   at ... ()
[info]   at fixedpoint.FixedPoint$$anonfun$apply$1.apply(FixedPoint.scala:83)
[info]   at fixedpoint.FixedPoint$$anonfun$apply$1.apply(FixedPoint.scala:83)
[info]   at chisel3.RecordImpl.$anonfun$_makeLit$16(AggregateImpl.scala:957)
[info]   at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:75)
[info]   at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:35)
[info]   at chisel3.RecordImpl._makeLit(AggregateImpl.scala:957)
[info]   at chisel3.RecordImpl._makeLit$(AggregateImpl.scala:950)
[info]   at chisel3.Record._makeLit(Aggregate.scala:310)
[info]   at chisel3.experimental.package$BundleLiterals$AddBundleLiteralConstructor.Lit(package.scala:133)
[info]   ...

jackkoenig · 2025-03-12T15:08:02Z

CI is passing 👀

tymcauley · 2025-03-12T15:09:56Z

CI is passing 👀

Yup, finally! In the last commit, I updated those failing tests to not use out-of-range literals. I feel like we should keep this PR in draft state until it depends on actually-released Chisel 7, rather than a snapshot.

Thanks so much for all the help on this @jackkoenig!

jackkoenig · 2025-03-12T15:32:41Z

I feel like we should keep this PR in draft state until it depends on actually-released Chisel 7, rather than a snapshot.

Totally, I meant it as a vote of confidence toward releasing 7.0.0-RC1--still need a few things but getting very close.

src/test/scala/ChiselSpec.scala

seldridge

Really nice migration. I'm glad this also got the line count down by nuking ChiselRunners. I'm a huge fan of getting onto unified testing infrastructure. If there's things you're missing/needing, those can be added to ChiselSim as appropriate.

`UnknownWidth` became a `case object` in this Chisel PR: chipsalliance/chisel#4242 The `connectFromBits` method was removed in this Chisel PR: chipsalliance/chisel#4168 The `connectFromBits` API was replaced with the `_fromUInt` API in this Chisel PR: chipsalliance/chisel#4782 Wrapping unary negation (`unary_-%`) was deprecated in this PR: chipsalliance/chisel#4829 The sbt update is necessary to maintain compatibility with the latest Scala compiler version.

The `chisel3.testers` package was removed here: chipsalliance/chisel#4746 It doesn't look like ChiselSim has the same `Boolean` result from `simulate`, so we must check for simulation errors by catching exceptions.

For background: actions/setup-java#712 Also updated JDK to 21 (latest LTS), updated some GitHub actions to their latest versions, and using install-circt GitHub action to get firtool.

An 8-bit signed integer with a binary point at 2 effectively has a 6-bit signed number for the value to the left of the decimal point. That means it can represent values in the range [-2^5, 2^5-1], or [-32, 31]. After chipsalliance/chisel#4786 was merged, the `55` and `56` literals are now correctly flagged as being out-of-range for this type. I've subtracted 32 from these out-of-range values (changing the MSB from 1 to 0), and the tests now pass.

This update includes this Chisel PR, which deprecated everything in the `firrtl` package: chipsalliance/chisel#4878 Bump `firtool` version to latest as well.

Also update CI versions of firtool and verilator to latest.

tymcauley marked this pull request as draft July 16, 2024 15:57

tymcauley force-pushed the update-chisel-7.x branch from 218579f to 28b609f Compare July 16, 2024 16:03

jackkoenig reviewed Jan 6, 2025

View reviewed changes

src/main/scala/fixedpoint/FixedPoint.scala Outdated Show resolved Hide resolved

tymcauley force-pushed the update-chisel-7.x branch from 28b609f to ccaf0b4 Compare January 6, 2025 19:05

tymcauley mentioned this pull request Mar 6, 2025

Chisel 7 Support ucb-bar/chipyard#2205

Open

2 tasks

jackkoenig mentioned this pull request Mar 10, 2025

Change Data._fromUInt to protected chipsalliance/chisel#4782

Merged

14 tasks

tymcauley force-pushed the update-chisel-7.x branch 3 times, most recently from 14bc211 to b309d81 Compare March 11, 2025 20:28

jackkoenig mentioned this pull request Mar 11, 2025

Fix SInt literals to reject too small of widths chipsalliance/chisel#4786

Merged

14 tasks

chiselbot mentioned this pull request Mar 12, 2025

Fix SInt literals to reject too small of widths (backport #4786) chipsalliance/chisel#4788

Closed

14 tasks

tymcauley force-pushed the update-chisel-7.x branch from b309d81 to 6ff5009 Compare March 12, 2025 01:08

seldridge reviewed Mar 13, 2025

View reviewed changes

src/test/scala/ChiselSpec.scala Outdated Show resolved Hide resolved

src/test/scala/ChiselSpec.scala Outdated Show resolved Hide resolved

tymcauley force-pushed the update-chisel-7.x branch from 908293f to 22dcfd3 Compare March 13, 2025 19:24

seldridge reviewed Mar 13, 2025

View reviewed changes

tymcauley force-pushed the update-chisel-7.x branch 2 times, most recently from 42d427a to e90d9c9 Compare March 24, 2025 02:14

tymcauley force-pushed the update-chisel-7.x branch from e90d9c9 to a9f9856 Compare April 2, 2025 17:52

tymcauley added 2 commits April 2, 2025 14:09

Migrate from Chisel testers to ChiselSim

e0c1166

The `chisel3.testers` package was removed here: chipsalliance/chisel#4746 It doesn't look like ChiselSim has the same `Boolean` result from `simulate`, so we must check for simulation errors by catching exceptions.

tymcauley added 6 commits April 2, 2025 14:10

Replace ChiselRunners trait with direct use of ChiselSim

4378772

Update to latest scalafmt

d2cccde

lint: Apply latest scalafmt settings

edd3ecd

Update to latest scalatest version

af532c9

Fix missing sbt command in GitHub CI

ef5dbd4

For background: actions/setup-java#712 Also updated JDK to 21 (latest LTS), updated some GitHub actions to their latest versions, and using install-circt GitHub action to get firtool.

tymcauley force-pushed the update-chisel-7.x branch from a9f9856 to e0769d4 Compare April 2, 2025 21:10

tymcauley added 2 commits April 17, 2025 22:12

Update Chisel 7 snapshot from 639-5df5515f to 678-dca5fc11

99663fd

This update includes this Chisel PR, which deprecated everything in the `firrtl` package: chipsalliance/chisel#4878 Bump `firtool` version to latest as well.

Update Chisel version to 7.0.0-RC1

c370e04

Also update CI versions of firtool and verilator to latest.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: Update Chisel from 6.5.0 to 7.0.0 #11

WIP: Update Chisel from 6.5.0 to 7.0.0 #11

Uh oh!

tymcauley commented Jul 16, 2024 •

edited

Loading

Uh oh!

tymcauley commented Aug 20, 2024

Uh oh!

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

jackkoenig commented Mar 11, 2025 •

edited

Loading

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

jackkoenig commented Mar 11, 2025 •

edited

Loading

Uh oh!

tymcauley commented Mar 12, 2025

Uh oh!

jackkoenig commented Mar 12, 2025

Uh oh!

tymcauley commented Mar 12, 2025

Uh oh!

jackkoenig commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!

seldridge left a comment

Uh oh!

Uh oh!

WIP: Update Chisel from 6.5.0 to 7.0.0 #11

Are you sure you want to change the base?

WIP: Update Chisel from 6.5.0 to 7.0.0 #11

Uh oh!

Conversation

tymcauley commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

Uh oh!

tymcauley commented Aug 20, 2024

Uh oh!

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

jackkoenig commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

tymcauley commented Mar 11, 2025

Uh oh!

jackkoenig commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tymcauley commented Mar 12, 2025

Uh oh!

jackkoenig commented Mar 12, 2025

Uh oh!

tymcauley commented Mar 12, 2025

Uh oh!

jackkoenig commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tymcauley commented Jul 16, 2024 •

edited

Loading

jackkoenig commented Mar 11, 2025 •

edited

Loading

jackkoenig commented Mar 11, 2025 •

edited

Loading