llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.62k stars 281 forks source link

[FIRRTL] What is the purpose of ExpandWhens? #2636

Closed Kuree closed 2 years ago

Kuree commented 2 years ago

Hello,

I'm a little bit confused about the purpose of ExpandWhens pass in Firrtl, which runs before the IR is lowered to HW. It seems to me that it tries to convert a more high-level construct (if statement) into low-level muxes. From a layperson's point of view, I fail to understand the benefits:

  1. If it's trying to optimize for combinational logic, I don't think it is necessary. Synthesis tools such as DC are smart enough to identify the duplicated combinational logic and reuse it properly.
  2. It makes symbol table generation harder. I'm trying to insert lots of debug attributes in the pass to track conditional information. If the conditional structure is preserved, these hacks are unnecessary. This is the main reason I'm filing this issue.
  3. It makes the generated RTL orders of magnitude more difficult to understand. As someone who only learns designing hardware in SystemVerilog, I find it mentally exhausting to understand the generated code while debugging using standard EDA tools. Again, this is just my personal opinion. Maybe in the future we will have better debugging tools that support RTL generated by circt.
  4. Most EDA tools understand if statement inside always_ff block, so I don't think compatibility is an issue.
  5. As far as I know, simulation tools love if statement over mux. This is because branch predictor works better than cmov instruction. I believe Verilator has a pass that changes mux into if statement whenever possible. I could be wrong on this one.

My outsider's suggestions is to replace this pass with a simple legality check pass, and lower WhenOp directly into IfOp during the Firrtl to HW pass. However, since I'm not familiar with circt, this could be much harder than I imagine. Please let me know if you have any suggestions.

Best, Keyi

seldridge commented 2 years ago

Your intuition is spot-on. We would like to eventually replace this with a check that all signals are driven and then enable direct lowering to a Verilog representation that is closer to the original Chisel-emitted FIRRTL description.

However, our primary concern has been to build a Scala FIRRTL Compiler (SFC) -equivalent Chisel compiler. SFC has always had an ExpandWhens pass for two major reasons:

  1. A guarantee of Chisel is that all signals are driven and going from when (which has last-connect semantics) to mux (which effectively removes last-connect semantics) makes this analysis much easier.
  2. Writing compiler passes that run before ExpandWhens is much harder than writing them after all when statements are removed.

However, this is all SFC legacy stuff...

Why we're really doing this is that it's a lot easier to build an SFC-compliant FIRRTL compiler if we are doing the same lowerings. Part of building this has been running formal equivalence checking on SFC and MFC (MLIR-based FIRRTL Compiler, i.e., CIRCT) -produced Verilog as well as running large test suites on both. Getting formal equivalence to work or debugging test failures when the Verilog diff is large is borderline intractable. Hence, we've chosen to stay SFC-exact in our implementation and only deviate once we've replaced the SFC.

Another transform like this is LowerTypes. A goal of the MFC is to preserve Chisel-defined aggregates. However, doing this from the beginning would be enormously risky... Hence, we implemented LowerTypes and have only been slowly removing it (through a lot of hard work by @uenoku) and are at the point where we can preserve passive aggregates with an option and pass test suites.

Kuree commented 2 years ago

However, our primary concern has been to build a Scala FIRRTL Compiler (SFC) -equivalent Chisel compiler.

That makes a lot of sense to me now. Thanks for the explanation, @seldridge.

A follow up question that's been troubling me when I'm trying to implement the symbol table. What is the best course of action for me to extract out conditional information given ExpandWhens pass? This is critical for compute whether an emulated breakpoint should be triggered during simulation.

Kuree commented 2 years ago

Closing this now since I've found a relatively easy way to deal with lowered muxes. Thanks for the clarification again!

seldridge commented 2 years ago

Nice. I'm glad you got it sorted out.