Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "[FIRRTL] Enable Wire Elimination (#7073)" #7311

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jul 11, 2024

This reverts commit f5a0969.

Wire Elimination is causing some issues where wire names are overriding register names in an undesirable way. I think we just need to tweak our heuristic a bit then re-enable, but we would like to get a firtool release out ASAP so I think we should revert temporarily.

Example FIRRTL:

FIRRTL version 3.3.0
circuit Foo :
  module Foo :
    input clock : Clock
    input reset : UInt<1>
    input in : { fizz : { foo : UInt<8>}, buzz : { foo : UInt<8>}}
    output out1 : { foo : UInt<8>}
    output out2 : { foo : UInt<8>}

    reg myReg : { fizz : { foo : UInt<8>}, buzz : { foo : UInt<8>}}, clock
    wire w1 : { foo : UInt<8>}
    wire w2 : { foo : UInt<8>}
    connect myReg.buzz.foo, in.buzz.foo
    connect myReg.fizz.foo, in.fizz.foo
    connect w1.foo, myReg.fizz.foo
    connect w2.foo, myReg.buzz.foo
    connect out1.foo, w1.foo
    connect out2.foo, w2.foo

Before this revert:

// Generated by CIRCT firtool-1.76.0-171-g961bad58f
module Foo(
  input        clock,
               reset,
  input  [7:0] in_fizz_foo,
               in_buzz_foo,
  output [7:0] out1_foo,
               out2_foo
);

  reg [7:0] w1_foo;
  reg [7:0] w2_foo;
  always @(posedge clock) begin
    w1_foo <= in_fizz_foo;
    w2_foo <= in_buzz_foo;
  end // always @(posedge)
  assign out1_foo = w1_foo;
  assign out2_foo = w2_foo;
endmodule

After this revert (and also the behavior on firtool 1.76.0 and previous versions):

// Generated by CIRCT firtool-1.76.0-172-gd6f349bed
module Foo(
  input        clock,
               reset,
  input  [7:0] in_fizz_foo,
               in_buzz_foo,
  output [7:0] out1_foo,
               out2_foo
);

  reg [7:0] myReg_fizz_foo;
  reg [7:0] myReg_buzz_foo;
  always @(posedge clock) begin
    myReg_fizz_foo <= in_fizz_foo;
    myReg_buzz_foo <= in_buzz_foo;
  end // always @(posedge)
  assign out1_foo = myReg_fizz_foo;
  assign out2_foo = myReg_buzz_foo;
endmodule

Copy link
Member

@dobios dobios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any breaking issue with doing this so LGTM!

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jackkoenig jackkoenig merged commit 8bc5e93 into main Jul 12, 2024
4 checks passed
@jackkoenig jackkoenig deleted the dev/jackkoenig/disableWireElim branch July 12, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants