ucb-bar / chisel-tutorial

chisel tutorial exercises and answers
Other
696 stars 197 forks source link

question about Firrtl(IR) optimization #142

Open YingkunZhou opened 5 years ago

YingkunZhou commented 5 years ago
// See LICENSE.txt for license details.
package solutions

import chisel3._

// Problem:
//
// 'out' should be the sum of 'in0' and 'in1'
// Adder width should be parametrized
//
class Adder(val w: Int) extends Module {
  val io = IO(new Bundle {
    val in0 = Input(UInt(w.W))
    val in1 = Input(UInt(w.W))
    val out = Output(UInt(w.W))
  })
  io.out := io.in0 + io.in1
}

above is the scala code for Adder in $TUT_DIR/src/main/scala/problems/ when I run ./run-solution.sh Adder --backend-name verilator I got the corresponding Verilog code module Adder( input clock, input reset, input [7:0] io_in0, input [7:0] io_in1, output [7:0] io_out ); wire [8:0] _T_11; // @[Adder.scala 17:20] assign _T_11 = io_in0 + io_in1; // @[Adder.scala 17:20] assign io_out = io_in0 + io_in1; // @[Adder.scala 17:10] endmodule

Here assign _T_11 = io_in0 + io_in1; // @[Adder.scala 17:20] is a line of deadcode, which in my eyes wastes a build-in adder thus should be eliminated. And it is easy to fulfill using dead code elimination for IR. So is it a bug for current chisel3 implementation?

edwardcwang commented 5 years ago

While I'm not sure why FIRRTL generated _T_11, synthesis tools will almost always prune those kinds of lines so it will not consume any physical resources.

YingkunZhou commented 5 years ago

But you cannot expect that synthesis tools will do these things.

edwardcwang commented 5 years ago

From our experience it usually does. Feel free to ask some others or try it yourself.

grebe commented 5 years ago

@YingkunZhou it is definitely fair to say that _T_11 shouldn't be there, regardless of how it impacts final QoR. Is there firrtl output you can also post? It should be in the same directory as the verilog and have a ".fir" file extension. I think this is a backend problem more than a frontend problem, but having firrtl would be helpful in verifying that. Perhaps something is being const-prop'd and then DCE is not being run afterwards.

YingkunZhou commented 5 years ago

;buildInfoPackage: chisel3, version: 3.1.5, scalaVersion: 2.11.12, sbtVersion: 1.1.1, builtAtString: 2018-12-14 23:45:23.572, builtAtMillis: 1544831123572 circuit Adder : module Adder : input clock : Clock input reset : UInt<1> output io : {flip in0 : UInt<8>, flip in1 : UInt<8>, out : UInt<8>}

node _T_11 = add(io.in0, io.in1) @[Adder.scala 17:20]
node _T_12 = tail(_T_11, 1) @[Adder.scala 17:20]
io.out <= _T_12 @[Adder.scala 17:10]
YingkunZhou commented 5 years ago

not familiar with firrtl, though

grebe commented 5 years ago

My output is

module Adder( // @[:test.fir@2.2]
  input        clock, // @[:test.fir@3.4]
  input        reset, // @[:test.fir@4.4]
  input  [7:0] io_in0, // @[:test.fir@5.4]
  input  [7:0] io_in1, // @[:test.fir@5.4]
  output [7:0] io_out // @[:test.fir@5.4]
);
  assign io_out = io_in0 + io_in1; // @[Adder.scala 17:10:test.fir@9.4]
endmodule

Perhaps the tutorial is using an older version of firrtl?

YingkunZhou commented 5 years ago

so you use master branch? I forget when and how I installed firrtl

shunshou commented 5 years ago

Did the default behavior of + change to +& instead of +%? By default firrtl width inference adds an extra bit to the output of add and it has to trim it back down to the original width (that you specified for the output). Was it ever smart enough to get rid of the extra T? If you left the output width as unknown, it wouldn’t generate the extra T, I think, and would give you an output with width w + 1. I also don’t really like relying on tools to be able to do optimization’s that firrtl should be able to take care of either.

On Saturday, December 22, 2018, YingkunZhou notifications@github.com wrote:

so you use master branch? I forget when and how I installed firrtl

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ucb-bar/chisel-tutorial/issues/142#issuecomment-449606736, or mute the thread https://github.com/notifications/unsubscribe-auth/AGTTFpk6nLrMrV9sseoCIBalhQfrwNiCks5u7tL1gaJpZM4ZfQ9k .

shunshou commented 5 years ago

Oops didn’t see Paul’s last reply.

On Saturday, December 22, 2018, Angie shunshou@gmail.com wrote:

Did the default behavior of + change to +& instead of +%? By default firrtl width inference adds an extra bit to the output of add and it has to trim it back down to the original width (that you specified for the output). Was it ever smart enough to get rid of the extra T? If you left the output width as unknown, it wouldn’t generate the extra T, I think, and would give you an output with width w + 1. I also don’t really like relying on tools to be able to do optimization’s that firrtl should be able to take care of either.

On Saturday, December 22, 2018, YingkunZhou notifications@github.com wrote:

so you use master branch? I forget when and how I installed firrtl

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ucb-bar/chisel-tutorial/issues/142#issuecomment-449606736, or mute the thread https://github.com/notifications/unsubscribe-auth/AGTTFpk6nLrMrV9sseoCIBalhQfrwNiCks5u7tL1gaJpZM4ZfQ9k .

grebe commented 5 years ago

You may not have installed firrtl- it's included as a dependency of chisel (see here). Perhaps forcing the latest 3.1.5 for chisel will fix things? The release is a little over a week old.