[FIRRTL] Use PRINTF_FD macro instead of 0x80000002 as printf fd

[Clo91eaf]

updates the FIRRTLToHW conversion to use the PRINTF_FD macro for specifying the file descriptor, allowing the fd to be obtained externally. This change enhances flexibility by enabling the file descriptor to be defined outside the conversion logic.

now we can define a log package like this:

package log_pkg;
  bit log_cond;
  int log_fd;

  function automatic void log_open(string log_path);
    log_fd = $fopen(log_path, "w");
    if (log_fd == 0) $fatal(1, "failed to open file for write");
    log_cond = 1'b1;
  endfunction

  function automatic void log_close();
    if (log_cond) begin
      $fclose(log_fd);
      log_cond = 1'b0;
      log_fd = 0;
    end
  endfunction
endpackage

initial begin
  log_pkg::log_open("xxx");
end

final begin
  log_pkg::log_close();
end

and add compile option to import log_fd

+define+PRINTF_FD=log_pkg::log_fd
+define+PRINTF_COND=log_pkg::log_cond

[seldridge]

@darthscsi: What do you think about this?

We can just make this part of the FIRRTL ABI (which PRINTF_COND already should be).

This would be less configurable than other proposals (e.g., chipsalliance/firrtl-spec#213). However, it avoids some of the problems of things like introducing file descriptors into FIRRTL.

The downside of this is that it would take some clever SystemVerilog to be configurable. E.g., I expect that we could include an evolving SystemVerilog file which uses the PRINTF_COND and PRINTF_FD to control what prints are enabled and to which files certain prints go to. This would require the SystsemVerilog to be more complicated. However, that is likely much better than complicating the FIRRTL unnecessarily.

WDYT?

[seldridge]

CC: @girishpai: As we circle back to ideas you have had about a "logging library" for Chisel, I am wondering if only PRINTF_COND and PRINTF_FD with sufficient clever SystemVerilog is enough. SystemVerilog is such a large language that I expect we could do something here. Anyone, I'm curious if you have thoughts about this, too.

[FanShupei]

I'd like to explain motivations behind this PR.

We generate logs by chisel prinf and collect it for later analysis/verification. However, chisel printf is not configurable and it never works reliably since too many components (simulator/other C libraries) will also print to stdout/stderr. We struggle with it for a long time. chipsalliance/firrtl-spec#213 is proposed to solve the issue, however it seems people are not in favor of it since it looks too specific and too intrusive.

So I come up with a clever trick, implemented by my co-workers in this PR. It solves our issue by doing minimal
change on Circt side.

For the general "logging library" design, I'd like to share my thoughts as follows. We have several ways to extend firrtl.printf lowering:

1. This PR. This may be the least intrusive way. Though the code looks a bit tricky.
   It also serves other users who just want to redirect all chisel printf to a specific log file.

2. Lower to a macro call like `chisel_print($sformatf(...)). This should be the most generic way.
   Note unfortunately SV does not have variadic macro, though it could be emulated by some ugly trick
   For this way, maybe there's some performance concern:
   Is $write(...) and $write("%s", $sformatf(...)) the same performant?

3. the strategy taken by [major] Add fd operand to printf statement chipsalliance/firrtl-spec#213 .
   We may reconsider this strategy later if chisel/firrtl decide to systematically support verbatim SV types and expressions.(Like most C/C++ compilers support inline-asm anyway)

[darthscsi]

Do we add comments for other chisel macros? I didn't think we do. There should be a user friendly form of the chisel abi which outlines the macros created to control these things.

[sequencer]

Do we add comments for other chisel macros?

These macro are not exposed in Chisel, I think Chisel doesn't have a clear ABI for now.

But instead we should add these to firrtl spec, and chisel users can be point to there.

[darthscsi]

I don't have an opposition to a minimal change like this. My concern is it might not be general enough to really accomplish what everyone wants. But it is probably better to use the macro than a constant. As long as everyone involved is happy to replace it in the nearish future with something a bit better, I'm fine. It doesn't need to touch the firrtl abi as it is just providing an implementation dependent hook (yes, yes) for dynamically changing the abi behavior.

[fzi-hielscher]

I'm also fine with this as a short term solution. It could also easily be adopted in #7973 .

My half-baked plan for the future is to add an optional symbol reference operand to the sim.proc.print operation, representing an abstract output stream. The only hard requirement would be, that the backend has to provide some way of discriminating the streams. Whether the simulation will then actually use (and manage) different IO resources, or just dump everything to stdout with a unique prefix is up to the lowering passes and the simulator environment. We could then add (discardable) attributes to the operation declaring the symbol, which provide a hint on the desired specifics of that stream. E.g., "this is an error log", "this should go to a file called xyz.log", etc.

[sequencer]

I think we could add a new attribute to printf and related IR, which can be assigned and frontend. This attribute should have a default implementation like what we are currently assigning with stderr. User can provide enum, and these enums can be used in the lowering path, e.g. synthesis to different macro or goes to different XDMA channel in FPGA printf synthesis.

[seldridge]

This needs tests. Also, I agree that there probably isn't a good reason to add comments about what this is as it just bloats the IR.

Otherwise, this is good to proceed with.

[Clo91eaf]

@seldridge: Sorry, I only had time today to fix the issue in the test. Could you please take a look for me?