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

[Do-Not-Merge][FIRRTL][Sim][SV] Rework printf lowering pipeline #7973

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fzi-hielscher
Copy link
Contributor

@fzi-hielscher fzi-hielscher commented Dec 12, 2024

Do not merge!

This PR is intended to demonstrate and discuss a lowering pipeline for FIRRTL printf operations to SV using the sim dialect infrastructure, notably the sim.proc.print operation and the to-be-added trigger operations (#7676). The code is still rough and obviously not meant to be merged like this. But I'd like to give everyone the opportunity to try it out and talk about the general shape of it before I start breaking out chunks into separate PRs.

The relevant/affected passes are:

  • FIRRTLToHW
  • SerializeTriggers (new)
  • LowerTriggersToSV (new)
  • LowerSCFToSV (new, but currently not required for FIRRTL lowering)
  • LowerPrintsToSV (new)
  • ExportVerilog

This PR also adds the sim.trigger_gate operation, which can be used to conditionally disable portions of a trigger tree. This allows us to lower conditional prints without having to rely on scf.if.

I think the first point worth discussing is the ordering requirement on print operations. The current version of the FIRRTL spec (v4.0.0) states in section 16.1:

For clocked statements that have side effects in the environment (stop, print, and verification statements), the order of execution of any such statements that are triggered on the same clock edge is determined by their syntactic order in the enclosing module.

Going by the discussion with @seldridge in the ODM a couple of weeks ago this is effectively outdated. Since it is not obvious when two clock edges have to be considered to be the same, this requirement is hard to enforce. So, instead, any order should be considered legal. That's how I've implemented the conversion in FIRRTLToHW. Specifically, it does not create any sim.trigger_sequence op, which would impose an hard ordering requirement on its users.

On the other hand, @seldridge mentioned a "gentleman's agreement" to try and maintain the order of prints during compilation whenever possible. This is achieved by the SerializeTriggers pass, which runs right after FIRRTLToHW and removes any concurrency/non-determinism for triggered operations that are part of the same trigger tree at this point during compilation. It does so by inserting sim.trigger_sequence ops at any point where a trigger-typed value has more than one user. The produced sequence - conveniently - follows the syntactic order of its users.

Trying this all out in practice, running this chisel example through firtool yields:

  `ifndef SYNTHESIS	// src/main/scala/main.scala:11:9, :13:11, :15:11, :17:9
    always @(posedge clock) begin	// src/main/scala/main.scala:11:9, :13:11, :15:11, :17:9
      if ((`PRINTF_COND_) & ~reset) begin	// src/main/scala/main.scala:11:9, :13:11, :15:11, :17:9
        $fwrite(32'h80000002, "<");	// src/main/scala/main.scala:11:9
        if (en)	// src/main/scala/main.scala:13:11
          $fwrite(32'h80000002, "in = %d", $unsigned(in));	// src/main/scala/main.scala:13:11
        if (~en)	// src/main/scala/main.scala:12:13, :15:11
          $fwrite(32'h80000002, " - ");	// src/main/scala/main.scala:15:11
        $fwrite(32'h80000002, ">\n");	// src/main/scala/main.scala:17:9
      end
    end // always @(posedge)
  `endif // not def SYNTHESIS

Note that if we were to run CSE between FIRRTLToHW and SerializeTriggers, we'd get this instead:

  `ifndef SYNTHESIS	// src/main/scala/main.scala:11:9, :13:11, :15:11, :17:9
    always @(posedge clock) begin	// src/main/scala/main.scala:11:9, :13:11, :15:11, :17:9
      if ((`PRINTF_COND_) & ~reset) begin	// src/main/scala/main.scala:11:9, :13:11, :15:11
        $fwrite(32'h80000002, "<");	// src/main/scala/main.scala:11:9
        $fwrite(32'h80000002, ">\n");	// src/main/scala/main.scala:17:9
        if (en)	// src/main/scala/main.scala:13:11
          $fwrite(32'h80000002, "in = %d", $unsigned(in));	// src/main/scala/main.scala:13:11
        if (~en)	// src/main/scala/main.scala:12:13, :15:11
          $fwrite(32'h80000002, " - ");	// src/main/scala/main.scala:15:11
      end
    end // always @(posedge)
  `endif // not def SYNTHESIS

Which would be legal, but probably not match the user's expectations. Having these sort of shifts for non-obvious reasons makes me a little uneasy (in this case it is caused by CSEing trigger gates). But unless we go deep into clock domains to clearly define which clocks are the same, I think we'll have to live with this to some extent.

For reference, this is what the current main branch firtool produces for the same input:

  `ifndef SYNTHESIS	// src/main/scala/main.scala:11:9
    always @(posedge clock) begin	// src/main/scala/main.scala:11:9
      automatic logic _GEN = (`PRINTF_COND_) & ~reset;	// src/main/scala/main.scala:11:9
      if (_GEN)	// src/main/scala/main.scala:11:9
        $fwrite(32'h80000002, "<");	// src/main/scala/main.scala:11:9
      if ((`PRINTF_COND_) & en & ~reset)	// src/main/scala/main.scala:11:9, :13:11
        $fwrite(32'h80000002, "in = %d", in);	// src/main/scala/main.scala:13:11
      if ((`PRINTF_COND_) & ~en & ~reset)	// src/main/scala/main.scala:11:9, :12:13, :15:11
        $fwrite(32'h80000002, " - ");	// src/main/scala/main.scala:15:11
      if (_GEN)	// src/main/scala/main.scala:11:9, :17:9
        $fwrite(32'h80000002, ">\n");	// src/main/scala/main.scala:17:9
    end // always @(posedge)
  `endif // not def SYNTHESIS

There is more stuff to talk about, but I'll spare you any more walls of text before the holidays. 😉

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.

1 participant