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

[FIRRTL] design with layers not optimized as well as without #7533

Closed
seldridge opened this issue Aug 19, 2024 · 1 comment · Fixed by #7639 · May be fixed by #7534
Closed

[FIRRTL] design with layers not optimized as well as without #7533

seldridge opened this issue Aug 19, 2024 · 1 comment · Fixed by #7639 · May be fixed by #7534
Labels
FIRRTL Involving the `firrtl` dialect

Comments

@seldridge
Copy link
Member

seldridge commented Aug 19, 2024

This was uncovered while trying to move verification statements automatically into layers in Chisel. I'm seeing a missed opportunity for eliminating a circt_chisel_ifelsefatal intrinsic.

Consider the following where the assertion predicate reduces to 0:

FIRRTL version 4.0.0
circuit Foo:
  layer A, bind:
  public module Foo:
    input clock: Clock
    input a: UInt<2>
    output c: UInt<2>

    node b = bits(a, 1, 0)
    layerblock A :
      intrinsic(circt_chisel_ifelsefatal<format = "bar">, clock, eq(a, b), UInt<1>(1))

    ; This user of b is important!
    connect c, b

When compiled firtool Foo.fir this leaves a trivially dead assertion in the design:

// Generated by CIRCT firtool-1.81.1-18-g6aa871500

// Users can define 'STOP_COND' to add an extra gate to stop conditions.
`ifndef STOP_COND_
  `ifdef STOP_COND
    `define STOP_COND_ (`STOP_COND)
  `else  // STOP_COND
    `define STOP_COND_ 1
  `endif // STOP_COND
`endif // not def STOP_COND_

// Users can define 'ASSERT_VERBOSE_COND' to add an extra gate to assert error printing.
`ifndef ASSERT_VERBOSE_COND_
  `ifdef ASSERT_VERBOSE_COND
    `define ASSERT_VERBOSE_COND_ (`ASSERT_VERBOSE_COND)
  `else  // ASSERT_VERBOSE_COND
    `define ASSERT_VERBOSE_COND_ 1
  `endif // ASSERT_VERBOSE_COND
`endif // not def ASSERT_VERBOSE_COND_
module Foo_A(
  input [1:0] a,
              b,
  input       clock
);

  `ifndef SYNTHESIS
    always @(posedge clock) begin
      if (a != b) begin
        if (`ASSERT_VERBOSE_COND_)
          $error("bar");
        if (`STOP_COND_)
          $fatal;
      end
    end // always @(posedge)
  `endif // not def SYNTHESIS
endmodule

module Foo(
  input        clock,
  input  [1:0] a,
  output [1:0] c
);

  assign c = a;
endmodule


// ----- 8< ----- FILE "layers_Foo_A.sv" ----- 8< -----

// Generated by CIRCT firtool-1.81.1-18-g6aa871500
`ifndef layers_Foo_A
`define layers_Foo_A
bind Foo Foo_A a_0 (
  .a     (a),
  .b     (a),
  .clock (clock)
);
`endif // layers_Foo_A

However, if there is nothing to block LayerSink from moving the node b into the layerblock and optimization happens. This can be shown with the slightly modified circuit which removes the user of b:

FIRRTL version 4.0.0
circuit Foo:
  layer A, bind:
  public module Foo:
    input clock: Clock
    input a: UInt<2>
    output c: UInt<2>

    node b = bits(a, 1, 0)
    layerblock A :
      intrinsic(circt_chisel_ifelsefatal<format = "bar">, clock, eq(a, b), UInt<1>(1))

    invalidate c
// Generated by CIRCT firtool-1.81.1-18-g6aa871500
module Foo(
  input        clock,
  input  [1:0] a,
  output [1:0] c
);

  assign c = 2'h0;
endmodule


// ----- 8< ----- FILE "layers_Foo_A.sv" ----- 8< -----

// Generated by CIRCT firtool-1.81.1-18-g6aa871500
`ifndef layers_Foo_A
`define layers_Foo_A
`endif // layers_Foo_A

I haven't tracked down the issue, yet. I expect it is something to do with optimizations or canonicalizations not working across layerblocks and this then relying on LayerSink to make the optimization available.

The canonicalizer is responsible for removing this pattern. This runs after LowerLayers. At that point, this analysis is no longer easy to do as it would require doing canonicalization across a module boundary or some notion of "inter module value numbering".

@seldridge seldridge added the FIRRTL Involving the `firrtl` dialect label Aug 19, 2024
@seldridge
Copy link
Member Author

A further reduced example is provided below. The issue is not related to asserts and entirely to canonicalization probably needing to run before LowerLayers for best effect.

FIRRTL version 4.0.0
circuit Foo: %[[
  {"class": "firrtl.transforms.DontTouchAnnotation", "target": "~Foo|Foo>c"}
]]
  layer A, bind:
  public module Foo:
    input clock: Clock
    input a: UInt<2>
    output d: UInt<2>

    node b = bits(a, 1, 0)
    layerblock A :
      node c = eq(a, b)

    invalidate d
    connect d, b

seldridge added a commit that referenced this issue Aug 19, 2024
Run canonicalization before LowerLayers in order to optimize things which
are difficult to do after layerblocks have been converted to modules.  The
conversion to modules means that many trivial canonicalizers are now very
difficult to do as they require inter-module analysis.

Ideally, it would be best if LowerLayers ran as late as
possible.  (Generally any outlining should run as late as possible.)
However, we aren't quite ready to make this harder move of pushing
LowerLayers to the end of the FIRRTL pipeline.

Fixes #7533.

Signed-off-by: Schuyler Eldridge <[email protected]>
@dtzSiFive dtzSiFive changed the title [FIRRTL] circt_intrinsic_ifelsefatal missed optimization opportunity [FIRRTL] design with layers not optimized as well as without Aug 30, 2024
@dtzSiFive dtzSiFive linked a pull request Sep 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
1 participant