Update Chisel from 6.5.0 to 7.0.0

[tymcauley]

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.

[tymcauley]

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.

[tymcauley]

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]

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]

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]

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]

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]

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]

CI is passing 👀

[tymcauley]

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]

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.

[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.

[tymcauley]

This has been languishing for awhile now, just bumped to latest Chisel version and marked this as ready to merge.