when statements should prevent asserts from evaluating or printfs from printing.

Current proposal:
The assert statement should take a condition and a clock (to know when to sample), then the when-expansion pass ands the when conditions to the assert condition to create a new assert.

Example:

when p :
assert cond, clk
==>
assert and(cond,p), clk

In the initial-blocks that initialize registers to 0 don't use the right syntax for literal 0. Writing e.g. {13{0}} doesn't produce 13-bit-wide zero. Counterintuitively, it results in a 32*13-bit-wide zero. 13'h0 would do fine.

Relatedly, we probably should be randomizing the initial values, not zeroing them, but we can cross that bridge later.

Running https://github.com/ucb-bar/chisel-tutorial/blob/chisel3prep/solutions/VecShiftRegisterParam.scala
via the Launcher.scala in the same directory
Using chisel3 and the new_unit_test branch
Causes an error

Starting High Form Check
[email protected]: [module VecShiftRegisterSimple]  Reference T_8 is not declared.
   at /Users/chick/Aspire/IdeaProjects/firrtl/src/lib/stanza/compiler/[email protected]
   at /Users/chick/Aspire/IdeaProjects/firrtl/src/lib/stanza/core/[email protected]

VecShiftRegisterParamTests5829367966270114615.fir.txt

Leading zeros on UInt constants should be stripped to avoid lint errors (e.g. emit 4'h2 instead of 4'h02).

I get java.lang.StackOverflowError trying to build the Scala version of firrtl with Scala 2.11, sbt 0.13.9, and (Oracle) Java 1.8.0_74 on MacOSX 10.9.5, and Scala 2.11, sbt 0.13.9, and (Oracle) Java 1.8.0_45 on Ubuntu 14.04.2 LTS.
This still occurs after deleting everything in ~/.ivy2 and ~/.m2

This doesn’t happen with Java 1.7.0_76 (on MacOSX) or 1.7.0_80 (on Ubuntu).

Here's the bottom of the Ubuntu stack dump:

java.lang.StackOverflowError
    at scala.tools.nsc.typechecker.Typers$Typer.checkDead(Typers.scala:111)
    at scala.tools.nsc.typechecker.Typers$Typer.typedSelectOrSuperCall$1(Typers.scala:4811)
    at scala.tools.nsc.typechecker.Typers$Typer.typedInAnyMode$1(Typers.scala:5343)
    at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5359)
    at scala.tools.nsc.typechecker.Typers$Typer.runTyper$1(Typers.scala:5395)
    at scala.tools.nsc.typechecker.Typers$Typer.scala$tools$nsc$typechecker$Typers$Typer$$typedInternal(Typers.scala:5422)
    at scala.tools.nsc.typechecker.Typers$Typer.body$2(Typers.scala:5369)
    at scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5373)
    at scala.tools.nsc.typechecker.Typers$Typer$$anonfun$99.apply(Typers.scala:4524)
    at scala.tools.nsc.typechecker.Typers$Typer$$anonfun$99.apply(Typers.scala:4524)
    at scala.tools.nsc.typechecker.Typers$Typer.silent(Typers.scala:680)
    at scala.tools.nsc.typechecker.Typers$Typer.normalTypedApply$1(Typers.scala:4523)
    at scala.tools.nsc.typechecker.Typers$Typer.typedApply$1(Typers.scala:4579)
    at scala.tools.nsc.typechecker.Typers$Typer.typedInAnyMode$1(Typers.scala:5342)
    at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5359)
    at scala.tools.nsc.typechecker.Typers$Typer.runTyper$1(Typers.scala:5395)
    at scala.tools.nsc.typechecker.Typers$Typer.scala$tools$nsc$typechecker$Typers$Typer$$typedInternal(Typers.scala:5422)
    at scala.tools.nsc.typechecker.Typers$Typer.body$2(Typers.scala:5369)
    at scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5373)
    at scala.tools.nsc.typechecker.Typers$Typer.typedQualifier(Typers.scala:5471)
    at scala.tools.nsc.typechecker.Typers$Typer.typedQualifier(Typers.scala:5477)
    at scala.tools.nsc.typechecker.Typers$Typer.typedSelectOrSuperCall$1(Typers.scala:4811)
    at scala.tools.nsc.typechecker.Typers$Typer.typedInAnyMode$1(Typers.scala:5343)
    at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5359)
    at scala.tools.nsc.typechecker.Typers$Typer.runTyper$1(Typers.scala:5395)

Current master version of chisel3 test chiselTest.Counter.scala creates a firrtl file (attached) that is parsed successfully by stanza firrtl but gives the following error

firrtl -i *fir -o scala.vcd -X verilog
line 8:19 mismatched input '?' expecting IntLit
line 8:20 extraneous input '>' expecting {'circuit', '}', 'module', 'extmodule', 'input', 'output', 'UInt', 'SInt', 'Clock', '[', 'wire', 'reg', 'with', 'reset', 'mem', 'data-type', 'depth', 'read-latency', 'write-latency', 'read-under-write', 'reader', 'writer', 'readwriter', 'cmem', 'smem', 'inst', 'node', 'is', 'invalid', 'when', 'else', 'stop(', 'printf(', 'skip', 'infer', 'read', 'write', 'rdwr', 'old', 'new', 'undefined', 'UBits', 'SBits', '.', 'mux(', 'validif(', 'stop', 'printf', 'mux', 'validif', 'add(', 'sub(', 'mul(', 'div(', 'rem(', 'lt(', 'leq(', 'gt(', 'geq(', 'eq(', 'neq(', 'pad(', 'asUInt(', 'asSInt(', 'asClock(', 'shl(', 'shr(', 'dshl(', 'dshr(', 'cvt(', 'neg(', 'not(', 'and(', 'or(', 'xor(', 'andr(', 'orr(', 'xorr(', 'cat(', 'bits(', 'head(', 'tail(', Id}
line 8:27 no viable alternative at input 'clkwith'
line 9:4 extraneous input 'node' expecting {'}', 'module', 'extmodule'}
Exception in thread "main" firrtl.ParserException: 4 syntax error(s) detected
    at firrtl.Parser$.parse(Parser.scala:64)
    at firrtl.Driver$.compile(Driver.scala:48)
    at firrtl.Driver$.main(Driver.scala:125)
    at firrtl.Driver.main(Driver.scala)

with scala firrtl

EnableTester3110576757137255563.fir.txt

The depth is emitted as [0:N], rather than [0:N-1].

The following says the constant is too small, even though it's not:

circuit Top : 
  module Top : 
    output foo : UInt<1>
    foo := UInt<1>("h1")

Need compelling use cases for early exploration.

So I think there are some things that could/should change in the Scala IR definition that would clean it up a little. Some ideas are to match scala style, some are more my own stylistic opinions.

https://github.com/ucb-bar/firrtl/blob/master/src/main/scala/firrtl/IR.scala

1. Change all case class Name() to case object Name
   1. This would match scala style
2. Abbreviate/Rename various vertices
   1. Rename Expression to Exp (we use Stmt for Statements so why not have Exp to match?)
   2. Rename DefRegister to DefReg (to match abbreviation in concrete syntax)
   3. Rename DefInstance to DefInst (to match abbreviation in concrete syntax)
   4. Rename DefMemory to DefMem (to match abbreviation in concrete syntax)
   5. Rename Conditionally to When (less important but Conditionally is kind of long)
   6. Rename Print to Printf (to match name in spec)
3. Change Primops to camel-case
   1. ADD_OP -> AddOp (perhaps just Add?)
4. Make primops match concrete syntax name
   1. GREATER_EQ_OP -> GeqOp
   2. Do we gain anything by making the name long? This makes it confusing to know what names in a .fir file match the various PrimOps
   3. I'm sympathetic to the self-documenting names idea, but DYN_SHIFT_RIGHT_OP just looks ridiculous when that information can easily be put in a scala-doc
5. Use camelcase within case classes
   1. read_latency -> readLatency (or readLat)
6. Rename Direction to PortDir
7. Rename Flip to FieldDir
   1. DEFAULT is just confusing since the flip keyword really means REVERSE
   2. I think Default and Flip should be the case objects, to match "flip" keyword
8. Rename BulkConnect to PartialConnect (because spec)
9. Rename top-level trait from AST to ???
   • Something about the name bothers me. While FIRRTL circuits are ASTs, hardware is really a DAG.
   • I'd like it to be like FirrtlNode, but Node is something else, what about like FirrtlVertex?
10. Standardize use of FIRRTL vs Firrtl vs firrtl
    • Formerly, I put FIRRTL everywhere, but that doesn't really work well with camelcase
    • FIRRTLException is awkwad, I'd like FirrtlException better.

This issue may sound stupid but I'd like feedback before I change things so I won't change anything I shouldn't

@azidar I'd appreciate your feedback in particular

Failing input: http://www.eecs.berkeley.edu/~waterman/whenBug.fir

The following code snippet

reg out_slow_bits : UInt<17>, clock, reset
out_slow_bits := tohost_q.deq.bits
when fromhost_q.deq.valid :
  out_slow_bits := fromhost_q.deq.bits

generates the following incorrect Verilog:

always @(posedge clock) begin
  out_slow_bits <= fromhost_q$deq$bits;
end

Of course, the correct code is

always @(posedge clock) begin
  out_slow_bits <= fromhost_q$deq$valid ? fromhost_q$deq$bits : tohost_q$deq$bits;
end

When I write something like the following in Chisel

val reg = Reg(init = UInt(0))
when (foo) { reg := a }
when (bar) { reg := b }

I end up with Verilog like the following:

assign reg_enable = reset ? 1 : (bar ? 1 : foo);
assign reg_data = reset ? 0 : (bar ? b : a);
always @(posedge clock)
  if (reg_enable)
    reg <= reg_data;

That code results in meaningfully worse QoR than the following, presumably because DC pattern-matches the coding style and can perform more analysis. We should change Verilog codegen accordingly.

always @(posedge clock)
  if (reset)
    reg <= 0;
  else if (bar)
    reg <= b;
  else if (foo)
    reg <= a;

Need pass to generate writemasks for ram's in general (ASICs and FPGAs). Mem of vec should just work, but needs to be implemented both in frontend and firrtl.

There are a set of useful utility functions in Utils.scala that we use to apply a function to the children of a particular FIRRTL graph vertex.

In the quick port to Scala, @azidar had to create lots of different named functions (eg. sMap, eMap...) because type erasure does not allow us to overload functions when the different parameter type is another function. For example, we can't do the following:

def map(f: Expression => Expression, stmt: Stmt): Stmt = ...
def map(f: Stmt => Stmt, stmt: Stmt): Stmt = ...

Because to the JVM, both fs look the same. This was fixed for a subset before with the commented out StmtMagnet code you can still see in Utils. I am planning to move back to this style, but I have a few questions first:

1. Should we continue to call this function "map"? There obviously is similarity to the scala.collections map function. That being said, these scala.collections are collections whereas these FIRRTL tree vertices are not, and our semantics for map here are to apply the function to the vertex's children. Perhaps this is fine, or perhaps a name like mapChildren is more appropriate. @azidar, I remember your saying previously that LLVM uses map in this context, do you have any documentation to compare to? I've been looking but I don't entirely understand LLVM.
2. Perhaps this is irrelevant, but should these mappers be a method you call on FIRRTL vertices, or a function you pass the vertices to? See below. We use implicit methods because we don't want to muddy up the IR definition which is currently all case classes, and Scala does not allow you to define methods on classes unless you do so at declaration.

def func(s: Stmt): Stmt = ...

// Function you call on Stmt vertices
def map(f: Stmt => Stmt, stmt: Stmt): Stmt = ...
// Call:
map(func, stmt)

// Implicit method on Stmt vertices
implicit class StmtMappers(stmt: Stmt) {
    def map(f: Stmt => Stmt): Stmt = ...
}
// Call:
stmt map (func)
stmt.map(func)

I think we should settle on a style for Scala FIRRTL.

While they disagree occasionally with the official Scala Style Guide, our AMPLab neighbors have perhaps the largest open-source Scala project: Databricks.

The Databricks Style Guide has a lot about readability and even some good pointers for performance (when we get to that point).

Any thoughts?

Following firrtl ir causes error

circuit DecoupledAdderTests : 
  module NewDecoupledAdder : 
    input clock : Clock
    input reset : UInt<1>
    output io : {flip in : {flip ready : UInt<1>, valid : UInt<1>, bits : {a : UInt<16>, b : UInt<16>}}, out : {flip ready : UInt<1>, valid : UInt<1>, bits : {c : UInt<16>}}}

    io.out.bits.c := UInt<1>("h00")
    io.out.valid := UInt<1>("h00")
    io.in.ready := UInt<1>("h00")
    reg ready : UInt<1>, clock, reset
    reg busy : UInt<1>, clock, reset
    reg a_reg : UInt<16>, clock, reset
    reg b_reg : UInt<16>, clock, reset
    io.in.ready := ready
    when io.in.valid :
      a_reg := io.in.bits.a
      b_reg := io.in.bits.b
      io.in.ready := UInt<1>("h00")
      ready := UInt<1>("h00")
      busy := UInt<1>("h01")
      skip
    node T_45 = and(busy, io.out.ready)
    when T_45 :
      node T_46 = addw(a_reg, b_reg)
      io.out.bits.c := T_46
      io.out.valid := UInt<1>("h01")
      io.in.ready := UInt<1>("h01")
      busy := UInt<1>("h00")
      skip

  module DecoupledAdderTests : 
    input clock : Clock
    input reset : UInt<1>
    output io : {}

    inst device_under_test of NewDecoupledAdder
    device_under_test.io.out.ready := UInt<1>("h00")
    device_under_test.io.in.bits.b := UInt<1>("h00")
    device_under_test.io.in.bits.a := UInt<1>("h00")
    device_under_test.io.in.valid := UInt<1>("h00")
    device_under_test.clock := clock
    device_under_test.reset := reset
    reg T_10 : UInt<33>, clock, reset
    onreset T_10 := UInt<33>("h00")
    node T_12 = addw(T_10, UInt<1>("h01"))
    T_10 := T_12
    node T_14 = eq(reset, UInt<1>("h00"))
    when T_14 :
      printf(clock, "ticker %d", T_10)
      skip
    node T_16 = gt(T_10, UInt<7>("h064"))
    when T_16 :
      node T_18 = eq(reset, UInt<1>("h00"))
      when T_18 :
        stop(clock, 0)
        skip
      skip
    node T_20 = eq(reset, UInt<1>("h00"))
    when T_20 :
      printf(clock, "device out ready %d, valid %d", device_under_test.io.out.ready, device_under_test.io.out.valid)
      skip
    node T_22 = eq(reset, UInt<1>("h00"))
    when T_22 :
      printf(clock, "device in ready %d, valid %d", device_under_test.io.in.ready, device_under_test.io.in.valid)
      skip
    reg T_24 : UInt<1>, clock, reset
    onreset T_24 := UInt<1>("h00")
    reg T_26 : UInt<1>, clock, reset
    onreset T_26 := UInt<1>("h00")
    node T_27 = and(T_24, T_26)
    when T_27 :
      node T_29 = eq(reset, UInt<1>("h00"))
      when T_29 :
        stop(clock, 0)
        skip
      skip
    reg T_31 : UInt<1>, clock, reset
    onreset T_31 := UInt<1>("h00")
    reg T_33 : UInt<1>, clock, reset
    onreset T_33 := UInt<1>("h00")
    wire T_43 : {flip ready : UInt<1>, valid : UInt<1>, bits : {a : UInt<16>, b : UInt<16>}}[1]
    T_43[0] <> device_under_test.io.in
    wire T_64 : UInt<16>[1]
    T_64[0] := UInt<16>("h04")
    node T_68 = eq(T_24, UInt<1>("h00"))
    infer accessor T_69 = T_43[T_31]
    node T_78 = and(T_68, T_69.ready)
    when T_78 :
      node T_80 = eq(reset, UInt<1>("h00"))
      when T_80 :
        printf(clock, "input_event_counter %d", T_31)
        skip
      infer accessor T_81 = T_64[T_31]
      device_under_test.io.in.bits.a := T_81
      skip
    wire T_84 : UInt<16>[1]
    T_84[0] := UInt<16>("h07")
    node T_88 = eq(T_24, UInt<1>("h00"))
    infer accessor T_89 = T_43[T_31]
    node T_98 = and(T_88, T_89.ready)
    when T_98 :
      node T_100 = eq(reset, UInt<1>("h00"))
      when T_100 :
        printf(clock, "input_event_counter %d", T_31)
        skip
      infer accessor T_101 = T_84[T_31]
      device_under_test.io.in.bits.b := T_101
      skip
    node T_103 = eq(T_24, UInt<1>("h00"))
    infer accessor T_104 = T_43[T_31]
    node T_113 = and(T_103, T_104.ready)
    when T_113 :
      infer accessor T_114 = T_43[T_31]
      T_114.valid := UInt<1>("h01")
      node T_125 = addw(T_31, UInt<1>("h01"))
      T_31 := T_125
      node T_127 = geq(T_31, UInt<1>("h00"))
      T_24 := T_127
      skip
    node T_129 = eq(T_26, UInt<1>("h00"))
    when T_129 :
      node T_131 = addw(T_33, UInt<1>("h01"))
      T_33 := T_131
      node T_133 = geq(T_33, UInt<1>("h00"))
      T_26 := T_133
      skip
    wire T_136 : UInt<1>[1]
    T_136[0] := UInt<1>("h01")
    wire T_141 : UInt<16>[1]
    T_141[0] := UInt<16>("h03")
    node T_145 = eq(T_26, UInt<1>("h00"))
    infer accessor T_146 = T_136[T_33]
    node T_147 = and(T_145, T_146)
    node T_148 = and(T_147, device_under_test.io.out.valid)
    when T_148 :
      node T_150 = eq(reset, UInt<1>("h00"))
      when T_150 :
        printf(clock, "output_event_counter %d", T_33)
        skip
      infer accessor T_151 = T_141[T_33]
      node T_152 = neq(device_under_test.io.out.bits.c, T_151)
      when T_152 :
        infer accessor T_153 = T_141[T_33]
        node T_155 = eq(reset, UInt<1>("h00"))
        when T_155 :
          printf(clock, "Error: event %d out.bits.c was %x should be %x", T_33, device_under_test.io.out.bits.c, T_153)
          skip
        skip
      skip

Need pass to remove smem's and replace with blackboxes

Both Lower To Ground and Expand Whens have some performance issues.

My code generates two input Vecs of length N and 1 expected output Vec of length N, the runtime I get is:

Looks like the algorithms are O(n^2)

Proposal to introduce a "poison" circuit component:

poison <name> : <type>

It can only be used as a source, (its type cannot contain flips, like node). Backends interpret it differently - simulation creates random numbers, with the hope that things get poisoned and break tests, and asic backends eliminate it. The new circuit would be written as :

module top :
  output out : UInt<32>
  poison q : UInt<32>
  out := q                                ; mux not inferred for ASIC backends
  smem m : UInt<32>[128]
  read accessor x = m[i]
  when p :
    out := x

In my mind, this is analogous to a dynamic cast in a programming language - it gives the programmer a way to bypass some checks by the compiler, with the assumption a runtime check will detect it.

This construct also allows output ports to be undefined (always initialized to poison), which enables current chisel code to be supported (as output ports did not always need to be assigned to, if they were never read from).

As of commit 0ec0032 rocket-chip no longer builds; the check genders pass segfaults on build-fast or bails out with the error no-branch for the normal build.

The following test case manifests the bug: http://www.eecs.berkeley.edu/~waterman/gender.fir

Uninferred widths are not caught gracefully.
The following needs to be checked:

• No VarWidth/UnknownWidth
• All widths are positive, and nonzero
• Width invariants are checked

Two current proposals:

• a printf node that takes a clock, a string literal, and N arguments
• a print node that merely takes a clock and a Vec of UInts, interpreting each element as a character

In the latter case, the formatting code would take the form of RTL that fills in the characters in the Vec.

There appears to be some difference between scala and stanza in the formatting of some
printf statements. Here is a characteristic diff output, note the quotes around printf args

diff scala-firrtl.v stanza-firrtl.v
402c402
<             $fwrite(32'h80000002,"setting tbl(%h) to %h\n",io_load_routing_table_request_bits_addr,io_load_routing_table_request_bits_data);

---
>             $fwrite(32'h80000002,"setting tbl(%h) to %h\n","io_load_routing_table_request_bits_addr","io_load_routing_table_request_bits_data");

files.zip

make build-scala doesn't rerun if you change a source file. It needs to depend on .scala files.

At this point, the scala implementation should be default, so make build should probably be equivalent to make build-scala (the current make build could become make build-stanza and a new command could do the actual building of stanza)

Need to add this pass

It seems constant widths are checked at the wrong point in compilation. The following is obviously invalid but is not caught:

circuit Top : 
  module Top : 
    node foo = UInt<1>("h2")

Include explanation of using a vector of single width wires.

Syntax is the following:

rdwr accessor x = m[i]

These can be inferred from an infer accessor if it is both written to and read from. The behavior is undefined if it is both written to and read from on the same cycle.

While this is potentially unintuitive for cmem's and reg of vec, it makes semantic sense so it will be left in.

There is a problem where existing Verilog boxes have ports that can be driven by different places, i.e. I2C bus. This is represented by the Verilog "inout" construct. While we don't want to directly support modeling tri-state buses, we do need a way to connect black boxed Verilog designs within FIRRTL to other black boxed Verilog designs.

As a result, we need a way to indicate that two ports are connected, but that we can make no other assumptions about it (i.e. you cannot buffer the signal, or introduce a mux, etc.)

My proposal is to add two new constructs:

1. An Analog type. An analog type has the property that no two Analog types are equivalent. Thus, the following FIRRTL causes a type error:

wire x : Analog
wire y : Analog
x <= y   ;ERROR: x and y do not have equivalent types
y <= x   ;ERROR: x and y do not have equivalent types

Another benefit is a bundle type that contains an Analog type will also fail when assigning to/from:

wire x : {a : Analog, b : UInt<32>}
wire y : {a : Analog, b : UInt<32>}
x <= y   ;ERROR: x and y do not have equivalent types
y <= x   ;ERROR: x and y do not have equivalent types

Finally, no primitive operation, mux, or node will accept an Analog-typed argument.

2. An attach operator. The attach operator takes a variable number of Analog-typed expressions, and attaches them together.

wire x : Analog
wire y : Analog
attach(x,y)

Assuming a, b, and c are all Analog types, the following two examples are equivalent:

attach(a,b)
attach(b,c)

and

attach(a,b,c)

Middle FIRRTL and Low FIRRTL will have the restriction that no two attach operators will have the same referenced expressions.

These two constructs give us the following property that Analog types will never interact with our existing connection semantics. This is for good reason - and Analog signal's direction is dynamic (or non-existent), while the connection semantics assumes a statically known direction. Rather than change our existing connection semantics and worry about how it interacts with other constructs, its better to never cause an interaction.

Thoughts?
@aswaterman @jackkoenig @jackbackrack @sdtwigg and any others?

The following FIRRTL code

node T_144549 = bits(UInt<1>("h00"), 29, 10)

results in the following illegal Verilog:

assign T_144531$ppn = 1'h00[29:10];

The issue is that you're not allowed to extract from a constant; you need to first assign the constant to a temporary. (Of course, you could constant-fold the expression instead.)

For now I'm going to work around this by using shifts instead of extract ops.

The following circuit generates invalid verilog.

FIRRTL:

circuit Top :  
  module Top :  
    output out : SInt
    out <= shr(SInt(-1),5)

Verilog:

module Top(
   output  out 
);
   assign out = 1'sh-1;
endmodule

The reason is the way BigInt is being created. A detailed look is needed at how this interacts with the FIRRTL spec, so I'm leaving this as an open issue for now.

Out-of-range extracts don't generate an error. That's a philosophical issue, but generating bad Verilog isn't.

I'm not sure if these are actually two separate bugs, but the following FIRRTL:

circuit Top :
  module Top :
    input clk : Clock
    input reset : UInt<1>

    reg sterling : UInt<4>, clk
    node malory = bits(sterling, 6, 2)
    node archer = bits(sterling, 8, 5)

generates the following Verilog:

reg [3:0] sterling;
wire [4:0] malory;
wire [3:0] archer;
assign malory = sterling[6:2];
assign archer = sterling;

The first is bad Verilog, and presumably it was supposed to be caught earlier in FIRRTL compilation.

The second omits the extraction because width(sterling) == width(archer). This is sensible, provided the bounds were also checked earlier in FIRRTL compilation.

I've gotten Resolve Kinds to crap out on FIRRTL commit ede7df1. Here's a failing input: http://www.eecs.berkeley.edu/~waterman/dedup.fir

Instead of emitting e.g.

x >> 3

we should emit

x[W-1:3]

where W is the width of x. This avoids a Verilog with mismatch that causes VCS warnings.

The following simple circuit causes FIRRTL to crash during width inference:

circuit Test : 
  module Test : 
    input clock : Clock
    input reset : UInt<1>

    reg out_buf : UInt, clock, reset

    out_buf := shr(out_buf, 16)

Actually, should be added to CheckTypes. Also, add check that initialization value is the same as the register type, and the type of reset is UInt<1> (or Clock for asynchronous reset? Needs more thought)

The following should add a read enable of pred to the generated verilog:

smem mem : UInt<20>[128],clk
poison xxx : UInt<6>
wire data : UInt<20>
read accessor readport = mem[mux(pred,index,xxx)]
out := readport

See bug fix in b487ab2
This was due to rocket regression having memory of depth = 1, thus address width = 0. This makes sense because the address port is unnecessary in this case.

If I recall correctly, support for zero-width wires was mentioned as future work? But if they are supported in Chisel, I think it should be now work.

The following circuit fails initialization check:

circuit top :
   module top :
      wire outs : UInt<32>[2][1]
      write accessor out = outs[UInt(0)]
      outs[0][0] := UInt(1)
      outs[0][1] := UInt(1)
      out[0] := UInt(1)

Error: shown during run is
%Error: /var/folders/ls/0xl3wjy949b2b9j36yn36v3c0000gn/T/BrokenUnitTester5010042410248580564/BrokenUnitTester5551577526393356957.v:333: Illegal bit or array select; type does not have a bit range, or bad dimension: type is logic

Following files are relevant

BrokenUnitTester5551577526393356957.fir.txt
BrokenUnitTester5551577526393356957.v.txt

Source scala is
package examples

import Chisel._
import Chisel.testers._

//A n-bit adder with carry in and carry out
class Broken(val n:Int) extends Module {
val io = new Bundle {
val A = UInt(INPUT, n)
val C = UInt(OUTPUT, n)
}
//create a vector of FullAdders
val FAs = Vec((0 until n).map { x => Module(new FullAdder()).io } )
val carry = Wire(Vec(n+1, UInt(width = 1)))
val sum = Wire(Vec(n, Bool()))

//wire up the ports of the full adders
for (i <- 0 until n) {
FAs(i).a := io.A(i)
FAs(i).b := UInt(1)
FAs(i).cin := UInt(0)
}
io.C := FAs(0).sum.toBool
}

class BrokenUnitTester(nBits: Int) extends SteppedHWIOTester {
val device_under_test = Module(new Broken(nBits))
val c = device_under_test

}

EDIT: Not an error, this is correct behavior, see comments below.

I'm not sure if the error is necessarily in the Infer Widths pass but something is wrong somewhere.
(Note: Perhaps this is not legal Chisel? I thought it was though)

Given the input Chisel with a Reg "count" that has an unknown width, but is initialized to 0.

class InferWidthError extends BasicTester {
  val count = Reg(init = UInt(0))
  count := count + UInt(1)
  when(count === UInt(10)) {
    io.done := Bool(true)
    io.error := UInt(0)
  }
}   

Chisel3 gives me the FIRRTL:

circuit InferWidthError :
  module InferWidthError :
    input clock : Clock
    input reset : UInt<1>
    output io : {done : UInt<1>, error : UInt<4>}

    io.done := UInt<1>("h00")
    io.error := UInt<1>("h00")
    reg count : UInt<?>, clock, reset
    onreset count := UInt<1>("h00")
    node T_10 = addw(count, UInt<1>("h01"))
    count := T_10
    node T_12 = eq(count, UInt<4>("h0a"))
    when T_12 :
      io.done := UInt<1>("h01")
      io.error := UInt<1>("h00")
      skip

Which is correct, reg count has an unknown width. The problem is that FIRRTL spits out the following Verilog

module InferWidthError(
    input  [0:0] clock,
    input  [0:0] reset,
    output [0:0] io$done,
    output [3:0] io$error
);
  wire [3:0] io$done_2;
  wire [0:0] io$done_1;
  wire [3:0] io$error_3;
  wire [0:0] io$error_2;
  wire [0:0] io$error_1;
  wire [0:0] count_1;
  reg  [0:0] count;
`ifndef SYNTHESIS
  integer initvar;
  initial begin
    #0.002;
    count = {1{$random}};
  end
`endif
  assign io$done_2 = {{3'd0 }, count };
  assign io$done_1 = io$done_2 == 4'ha;
  assign io$error_3 = {{3'd0 }, count };
  assign io$error_2 = io$error_3 == 4'ha;
  assign io$error_1 = io$error_2 ? 1'h0 : 1'h0;
  assign count_1 = count + 1'h1;
  assign io$done = io$done_1 ? 1'h1 : 1'h0;
  assign io$error = {{3'd0 }, io$error_1 };
  always @(posedge clock) begin
    count <= reset ? 1'h0 : count_1;
  end
endmodule

Which is wrong, particularly look at the line assign io$done_2 = {{3'd0 }, count }; and that count is for some reason a 1 bit variable.

If I give count a width in the Chisel code, it all works correctly, so something is being inferred wrong, or there needs to be an error telling me that this code is invalid.

Consider the following FIRRTL:

circuit Bug :
  module Bug :
    input clk : Clock
    input a : UInt<1>
    input b : UInt<1>

    printf(clk, UInt<1>(1), "%d\n", and(a, b))

Scala FIRRTL currently compiles to the following invalid Verilog:

module Bug(
   input   clk,
   input   a,
   input   b
);
   always @(posedge clk) begin
      `ifndef SYNTHESIS
         if(1'h1) begin
            $fwrite(32'h80000002,"%d\n",and(a, b));
         end
      `endif
   end
endmodule

Stanza FIRRTL does the same thing it does in #64, it puts quotes around "and(a,b)".

The spec just says the arguments to printf must have a ground type, which primops do, so I believe this should be handled by a pass. I'm not 100% on what all of the passes do but it looks like it might belong in Split Expressions or something similar. Documentation on what each pass does would be helpful.

@azidar please advise.

Using the latest versions of Chisel3 master: c85f9ff
and latest version of firrtl: 55ab528
In chisel3 directory

sbt
> test-only chiselTests.TblSpec

In either master or from the merge associated with this pull request
I see errors like:

Starting High Form Check
Fatal Uncaught Exception.
NoFileInfo: [module Tbl]  Width cannot be negative or zero.
NoFileInfo: [module Tbl]  Width cannot be negative or zero.
[email protected]: [module Tbl]  Width cannot be negative or zero.
   at /Users/chick/Aspire/IdeaProjects/firrtl/src/lib/stanza/compiler/[email protected]
   at /Users/chick/Aspire/IdeaProjects/firrtl/src/lib/stanza/core/[email protected]

I am seeing the following error

Starting Check Genders
Fatal Uncaught Exception.
[email protected]: [module Router]  Expression tbl.T_194.wmask is used as a source but can only be used as a sink.
[email protected]: [module Router]  Expression tbl.T_194.wdata is used as a source but can only be used as a sink.
[email protected]: [module Router]  Expression tbl.T_194.wmask is used as a source but can only be used as a sink.

from the firrtl file
RouterUnitTester6706371722218934244.fir.txt

And
Router.scala.txt

The FIRRTL below produces invalid Verilog

circuit ExpansionBug :
  module ExpansionBug :
    output io : { flip a : UInt<1>, b : UInt<1> }

    node io_a = io.a
    io.b <= io_a

module ExpansionBug(
  input   io_a,
  output  io_b
);
  wire  io_a;
  assign io_b = io_a;
  assign io_a = io_a;                                                                                                                                                                                                                                                         
endmodule

According to the spec section 10.1, the lowering of the io bundle should result in io$a and io$b instead of io_a and io_b

Appears to be a/the cause of chipsalliance/chisel#125

This bug is present in both implementations.

In the following FIRRTL

circuit Bug :                                                                                                                                                                                                                                                                                                              
  module Bug : 
    input clk : Clock
    input reset : UInt<1>

    wire buggyReset : {valid : UInt<1>}
    buggyReset is invalid
    reg buggyReg : {valid : UInt<1>, bits : UInt<3>}, clk with : (reset => (reset, buggyReset))

buggyReg is initialized, but because it is of BundleType, it is not fully initialized by buggyReset.

This leads to an IndexOutOfBounds Exception during LowerTypes when the pass tries to connect each subfield of buggyReset to buggyReg (similar error in Stanza FIRRTL).

It seems to me that this should be detected and an error thrown during CheckInitialization. I don't entirely understand Working IR Void, but they seem to be added in ExpandWhens. I'll look into this later but @azidar , maybe the bug is obvious to you.

To support funky characters in string literals, C supports escaping them as using \x followed by their ASCII value in hex. For example, \n can instead be written as \xa or \xA, and space can be written as \x20. We should support this. For now, I'll disallow unprintable characters in Chisel printf.

This problem manifests in both Scala and Stanza FIRRTL

Consider the FIRRTL:

circuit Bug :                                                                                                                                                                                                                                 
  module Bug :
    input in : UInt<1>
    output out : UInt<1>

    out <= shr(in, 1)

This results in the following invalid Verilog:

module Bug(
   input   in,                                                                                                                                                                                                                                
   output  out 
);  
   assign out = in[0:1];                                                                                               
endmodule

Shifting by more than the width of the operand gives similar results:

circuit Bug : 
  module Bug : 
    input in : UInt<5>
    output out : UInt<5>

    out <= shr(in, 6)     

module Bug(
   input  [4:0] in,
   output [4:0] out                                                                                                                                                                                                                           
);  
   assign out = in[4:6];                                                                                               
endmodule

First of all, the spec requires that the shift amount be strictly less than the width of the operand, so neither implementation is correctly implementing the spec here.

Secondly, and more importantly, since Chisel relies on FIRRTL for width-inference, it cannot prevent itself from emitting invalid FIRRTL here. Moreover, dynamic shift right does not have any constraint like this (as it obviously cannot) and just seems to punt to Verilog as in this example:

circuit Bug : 
  module Bug : 
    input in : UInt<5>
    output out : UInt<5>

    out <= dshr(in, UInt(6))                         

module Bug(                                                                                                                                                                                                                                   
   input  [4:0] in,
   output [4:0] out
);
   assign out = in >> 3'h6;
endmodule

Thirdly, allowing for shift amounts greater than or equal to the width of the operand makes it easier to write generators in that you don't have add special cases in your code.

I think FIRRTL should define the semantics of right shifting a number by an amount equal to or greater than the number's width to be (both are width = 1):

• 0 when unsigned
• sign bit when signed

These semantics strike me as both intuitive and necessary since they can be consistent between static and dynamic right shift.