asUInt packing inconsistent for Bundles with type parameters

[maxbjurling]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:
The code below defines a Bundle with type parameters and four fields, all resolving to type Bool().

Generating Verilog for this code:

import chisel3._
import _root_.circt.stage.ChiselStage

class Types {
  val D = Bool()
  val B = Bool()
}

class InBundle [T <: Types, U <: Data] (TypeC: U, TypeBD: T) extends Bundle {
  val a = Bool()
  val b = TypeBD.B
  val c = TypeC
  val d = TypeBD.D
}

class BundleTest extends Module() {
  val io = IO (new Bundle {
    val in1  =  Input(new InBundle (TypeBD = new Types, TypeC  = Bool()))
    val in2  =  Input(new InBundle (TypeC  = Bool(),    TypeBD = new Types))
    val out1 = Output(UInt(in1.getWidth.W))
    val out2 = Output(UInt(in2.getWidth.W))
  })
    io.out1 := io.in1.asUInt
    io.out2 := io.in2.asUInt
  }

object VerilogMain extends App {
  ChiselStage.emitSystemVerilogFile(new BundleTest)
}

results in that a, b, c, and d (all bools) are packed differently in out1 and out2 in the generated Verilog file:

assign io_out1 = {io_in1_d, io_in1_b, io_in1_c, io_in1_a}; // {d, b, c, a}
assign io_out2 = {io_in2_c, io_in2_d, io_in2_b, io_in2_a}; // {c, d, b, a}

What is the current behavior?

Packing of Bundle fields depend on:

• the order of type definitions
  • e.g. Types.D defined before Types.B in the code above
• the order in which parameters are assigned at the creation of the Bundle
  • TypeBD = new Types, TypeC = Bool() gives different result than TypeC = Bool(), TypeBD = new Types
• the order of declaring the fields of the Bundle

What is the expected behavior?

Packing order should only be influenced by the order of the declaration of the Bundle fields. In the example above I expect both outputs to be assigned to {a, b, c, d}

Please tell us about your environment:

• version: 6.3.0

Other Information

What is the use case for changing the behavior?

Interfacing with pure Verilog modules requires the field positions of a bundle to be controllable. Having to write an explicit mapping function from Bundle fields to bits of a bit vector shouldn't be needed.

[jackkoenig]

I'm shocked this hasn't come up before, but yeah this is a pretty big bug in Bundle field ordering. The order of fields in a Bundle is defined by the order the actual objects representing the fields were constructed (which in most cases matches Bundle field order, but as shown in the issue, may not). Using the order fields were constructed is legacy behavior from before we really had a way to determine field order. We can determine field order now in the compiler plugin so we can fix this.

Here is an example using the latest release (6.4.0) showing the issue (and how by-name generator parameters interact with it):

//> using scala "2.13.12"
//> using dep "org.chipsalliance::chisel:6.4.0"
//> using plugin "org.chipsalliance:::chisel-plugin:6.4.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 Bundle1(gen: => UInt) extends Bundle {
  val a = UInt(8.W)
  val b = gen
  val c = UInt(8.W)
}
class Bundle2(gen: UInt) extends Bundle {
  val a = UInt(8.W)
  val b = gen
  val c = UInt(8.W)
}

class Foo extends Module {
  val in0 = IO(Input(new Bundle1(UInt(8.W))))
  val in1 = IO(Input(new Bundle2(UInt(8.W))))
  val out0 = IO(Output(UInt(24.W)))
  val out1 = IO(Output(UInt(24.W)))

  out0 := in0.asUInt
  out1 := in1.asUInt
}

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

This gives:

// Generated by CIRCT firtool-1.62.0
module Foo(
  input         clock,
                reset,
  input  [7:0]  in0_a,
                in0_b,
                in0_c,
                in1_b,
                in1_a,
                in1_c,
  output [23:0] out0,
                out1
);

  assign out0 = {in0_a, in0_b, in0_c};
  assign out1 = {in1_b, in1_a, in1_c};
endmodule

[jackkoenig]

Fixing this involves:

1. Ensuring the order coming out of the compiler plugin is correct (including with inheritance and virtual methods)
2. Getting rid of the sorting by _id here.
3. Providing some migration path since fixing this is a silent change to behavior. Yes it's a bug fix but users may accidentally rely on the current order so we need a way to ensure their code doesn't silently break. Easiest option is probably to add a migration flag that emulates the old behavior (a la Change the width of static shift right #3824) so that users can generate Verilog with and without the flag and diff or LEC the result to see if their are any issues.

[adkian-sifive]

what is the "right order" here? follow up, what's the right order for the bundle elements a and b in something like

case class GenericBundle[T <: Data, U <: Data](val a: T, val b: U) extends Bundle
val in1 = IO(Input(GenericBundle(a = Bool(), b = UInt(8.W))))
val in2 = IO(Input(GenericBundle(b = UInt(8.W), a = Bool())))

[jackkoenig]

The right order is the order of the fields in the definition of the Bundle, so for your GenericBundle it should always be a then b no matter how it's instantiated with named parameters.

[jackkoenig]

The fix will be in the next 7.0 line release (probably 7.0.0-M3 or 7.0.0-RC1)