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

Emit FIRRTL andr, orr for Bits.{andR, orR} #1315

Merged
merged 1 commit into from
Feb 6, 2020
Merged

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Jan 28, 2020

Change the emission strategy for Bits methods andR and orR to emit FIRRTL bitwise reduce operations andr and orr. The CHIRRTL emission strategy for these two operations now matches the emission of xorR (which already used xorr).

Add two tests that assert the correct behavior of these operations in BitwiseOpsSpec.

This should fix one of the Verilog emission problems shown in chipsalliance/firrtl#1338 of:

class Foo(n: Int) extends RawModule {
  val in = IO(Input(UInt(n.W)))
  val out = IO(Output(Vec(3, Bool())))
  out(0) := in.andR
  out(1) := in.orR
  out(2) := in.xorR
}

This will now emit the following CHIRRTL for Foo(4):

circuit Foo : 
  module Foo : 
    input in : UInt<4>
    output out : UInt<1>[3]
    
    node _T = andr(in)
    out[0] <= _T
    node _T_1 = orr(in)
    out[1] <= _T_1
    node _T_2 = xorr(in)
    out[2] <= _T_2

And the resulting Verilog:

module Foo(
  input  [3:0] in,
  output       out_0,
  output       out_1,
  output       out_2
);
  assign out_0 = &in;
  assign out_1 = |in;
  assign out_2 = ^in;
endmodule

For the case of a zero-width wire, you get the reductions that @albert-magyar introduced in chipsalliance/firrtl#1362.

module Foo(
  output  out_0,
  output  out_1,
  output  out_2
);
  assign out_0 = 1'h1;
  assign out_1 = |1'h0;
  assign out_2 = ^1'h0;
endmodule

Related issue:

Type of change: other enhancement

Impact: API modification (to emitted CHIRRTL)

Development Phase: implementation

Release Notes

  • Change andR, orR emission to use FIRRTL andr, orr

@seldridge seldridge requested a review from a team as a code owner January 28, 2020 14:50
Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

This looks straightforward to me. It makes me wonder though if we should have some document that specifically documents changes we make to emitted firrtl and verilog

@seldridge
Copy link
Member Author

(Added this above...) This PR does change how andR behaves for a zero-width UInt.

Previously, this would generate:

outAnd = 1'h1;

This PR changes this emission to:

outAnd = &1'h0;

@seldridge seldridge added this to the 3.3.0 milestone Jan 29, 2020
@aswaterman
Copy link
Member

The old behavior (andR on a 0-bit UInt returns 1) is correct, for the same reason that Seq().forall(f) returns true. Otherwise, invariants like (a ## b).andR === a.andR && b.andR don't hold up.

@seldridge
Copy link
Member Author

While we can special case this in Chisel, this should be fixed in FIRRTL first. That Chisel is generating the following FIRRTL which results in the Verilog that is incorrect as @aswaterman identifies.

circuit Foo_0 : 
  module Foo_0 : 
    input in : UInt<0>
    output outAnd : UInt<1>

    node _T = andr(in)
    outAnd <= _T

@chick
Copy link
Contributor

chick commented Feb 3, 2020

@seldridge can we address @aswaterman concern? Requires a firrtl change?

Change the emission strategy for Bits methods andR and orR to emit
FIRRTL bitwise reduce operations andr and orr.

Add two tests that assert the correct behavior of these operations in
BitwiseOpsSpec.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
@seldridge
Copy link
Member Author

With the @albert-magyar fixes in chipsalliance/firrtl#1362, this is now doing the right thing in terms of Verilog emission. The language up top has been updated accordingly. I'm going to go ahead and merge as there is no effective change to the content of this PR.

@seldridge seldridge merged commit a505d23 into master Feb 6, 2020
@seldridge seldridge deleted the emit-orr-andr branch March 30, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants