circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRRTL][Dedup] Fix more thoroughly re:structure

Open dtzSiFive opened this issue 1 year ago • 0 comments

This is just another example of the sort that was recently fixed (#7415), and probably the hashing should have some sort of values added to the hash that indicate the structure so that this this is fixed properly/completely.

Here's something that dedup's but shouldn't:

FIRRTL version 4.0.0

circuit Block: %[[
{ "class": "firrtl.passes.InlineAnnotation",
  "target": "~Block|A" },
{ "class": "firrtl.passes.InlineAnnotation",
  "target": "~Block|B" }
]]
  module A:
    input x : UInt<1>
    output y : UInt<1>

    connect y, UInt<1>(0)

    node n = UInt<1>(1)
    when x :
      skip
    else :
      skip
    connect y, n

  module B:
    input x : UInt<1>
    output y : UInt<1>

    connect y, UInt<1>(0)

    node n = UInt<1>(1)
    when x :
      skip
    else :
      skip
      connect y, n

  public module Block:
    input x : UInt<1>
    output y : UInt<1>

    inst a of A
    connect a.x, x
    inst b of B
    connect b.x, x

    connect y, and(a.y, b.y)

Note that the connect y, n is AFTER the when in the first module, but in the else of the when in the second.

This currently produces (firtool):

// Generated by CIRCT firtool-1.79.0-24-g20cb546d1
module Block(
  input  x,
  output y
);

  assign y = 1'h1;
endmodule

The correct functionality for Block is produced if you block dedup, such as by annotation one of the modules "nodedup". Updated portion of the input:

circuit Block: %[[
{ "class": "firrtl.passes.InlineAnnotation",
  "target": "~Block|A" },
{ "class": "firrtl.passes.InlineAnnotation",
  "target": "~Block|B" },
{ "class": "firrtl.transforms.NoDedupAnnotation",
  "target": "~Block|B" }
]]

This yields:

// Generated by CIRCT firtool-1.79.0g20240801_20cb546
module Block(
  input  x,
  output y
);

  assign y = ~x;
endmodule

Note: The inlining is just to make the difference in behavior easier to observe in terms of final functionality, but isn't necessary to reproduce.

dtzSiFive avatar Aug 02 '24 17:08 dtzSiFive