chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
3.92k stars 589 forks source link

Lexical scope is not properly checked for ports of submodules #4405

Open jackkoenig opened 1 day ago

jackkoenig commented 1 day ago

This is a good example of why we need to overhaul our lexical scope checking (usually called "visibility" within Chisel). We really ought to add a specific Scope datastructure that is used for all scopes (Module, when, and layer) rather than individual things for checking each one (and I think layer scopes aren't even properly checked at the moment!)

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

Consider the following Scala-CLI using latest main:

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.14"
//> using dep "org.chipsalliance::chisel:7.0.0-M2+76-ecda00a5-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin:7.0.0-M2+76-ecda00a5-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 Child extends Module {
  val in = IO(Input(UInt(8.W)))
  val out = IO(Output(UInt(8.W)))

  out := in
}

class Foo extends Module {
  val in = IO(Input(UInt(8.W)))
  val out = IO(Output(UInt(8.W)))

  var c: Child = null
  when (true.B) {
    c = Module(new Child)
  }
  c.in := in
  out := c.out
}

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

What is the current behavior?

This will print the buggy FIRRTL and show the firtool error:

FIRRTL version 4.0.0
circuit Foo :
  layer Verification, bind, "Verification" :
    layer Assert, bind, "Verification/Assert" :
    layer Assume, bind, "Verification/Assume" :
    layer Cover, bind, "Verification/Cover" :
  module Child : @[Users/koenig/work/t/scope/chisel-example.scala 11:7]
    input clock : Clock @[Users/koenig/work/t/scope/chisel-example.scala 11:7]
    input reset : Reset @[Users/koenig/work/t/scope/chisel-example.scala 11:7]
    input in : UInt<8> @[Users/koenig/work/t/scope/chisel-example.scala 12:14]
    output out : UInt<8> @[Users/koenig/work/t/scope/chisel-example.scala 13:15]

    connect out, in @[Users/koenig/work/t/scope/chisel-example.scala 15:7]

  public module Foo : @[Users/koenig/work/t/scope/chisel-example.scala 18:7]
    input clock : Clock @[Users/koenig/work/t/scope/chisel-example.scala 18:7]
    input reset : UInt<1> @[Users/koenig/work/t/scope/chisel-example.scala 18:7]
    input in : UInt<8> @[Users/koenig/work/t/scope/chisel-example.scala 19:14]
    output out : UInt<8> @[Users/koenig/work/t/scope/chisel-example.scala 20:15]

    when UInt<1>(0h1) : @[Users/koenig/work/t/scope/chisel-example.scala 23:17]
      inst Child of Child @[Users/koenig/work/t/scope/chisel-example.scala 24:15]
      connect Child.clock, clock
      connect Child.reset, reset
    connect Child.in, in @[Users/koenig/work/t/scope/chisel-example.scala 26:8]
    connect out, Child.out @[Users/koenig/work/t/scope/chisel-example.scala 27:7]
Exception in thread "main" circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /Users/koenig/Library/Caches/org.chipsalliance.llvm-firtool/1.86.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+76-ecda00a5-SNAPSHOT) was published against firtool version 1.86.0.
------------------------------------------------------------------------------
ExitCode:
1
STDOUT:

STDERR:
<stdin>:25:13: error: use of unknown declaration 'Child'
    connect Child.in, in @[Users/koenig/work/t/scope/chisel-example.scala 26:8]
            ^

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

What is the expected behavior?

Chisel should error

What is the use case for changing the behavior?

dtzSiFive commented 16 hours ago

Yes, there's only scope checking for when's, and not layers (tested but also apparent from inspection re:WhenContext, so on).

FWIW I think the checking is wrong among sibling blocks for the same when as well:

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.14"
//> using dep "org.chipsalliance::chisel:7.0.0-M2+76-ecda00a5-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin:7.0.0-M2+76-ecda00a5-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 Child extends Module {
  val in = IO(Input(UInt(8.W)))
  val out = IO(Output(UInt(8.W)))

  out := in
}

class Foo extends Module {
  val in = IO(Input(UInt(8.W)))
  val out = IO(Output(UInt(8.W)))

  var c: Bool = null
  when (true.B) {
    c = WireInit(Bool(), true.B)
    out := c
  } .otherwise {
    c := false.B
    out := c
  }
}

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

Which generates the following output:

circuit Foo :
  layer Verification, bind, "Verification" :
    layer Assert, bind, "Verification/Assert" :
    layer Assume, bind, "Verification/Assume" :
    layer Cover, bind, "Verification/Cover" :
  public module Foo : @[scope-when-else.scala 18:7]
    input clock : Clock @[scope-when-else.scala 18:7]
    input reset : UInt<1> @[scope-when-else.scala 18:7]
    input in : UInt<8> @[scope-when-else.scala 19:14]
    output out : UInt<8> @[scope-when-else.scala 20:15]

    when UInt<1>(0h1) : @[scope-when-else.scala 23:17]
      wire _WIRE : UInt<1> @[scope-when-else.scala 24:17]
      connect _WIRE, UInt<1>(0h1) @[scope-when-else.scala 24:17]
      connect out, _WIRE @[scope-when-else.scala 25:9]
    else :
      connect _WIRE, UInt<1>(0h0) @[scope-when-else.scala 27:7]
      connect out, _WIRE @[scope-when-else.scala 28:9]

Exception in thread "main" circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /home/will/.nix-profile/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+76-ecda00a5-SNAPSHOT) was published against firtool version 1.86.0.
------------------------------------------------------------------------------
ExitCode:
1
STDOUT:

STDERR:
<stdin>:18:15: error: use of unknown declaration '_WIRE'
      connect _WIRE, UInt<1>(0h0) @[scope-when-else.scala 27:7]
              ^

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

As you say, probably motivates a scope overhaul 👍 . I can file separate issue if you'd prefer? But anyway just wanted to drop this somewhere.

jackkoenig commented 10 hours ago

Thanks for the extra example! I think it's fine as part of this issue but feel free to file a separate one if you want.