byuccl / bfasst

Tools for FPGA Assurance Flows
Apache License 2.0
12 stars 5 forks source link

added wafove #428

Closed KeenanRileyFaulkner closed 8 months ago

KeenanRileyFaulkner commented 8 months ago

I have a couple questions and comments about this PR before it is merged.

First, there are a lot of additional outputs from the wafove tool that are not specified in the ninja workflow. Because they are "leaf nodes" and unused by the flow, this does not cause an error. However, I don't know if that will be the case if future flows use wafove and feed it into something else. Is there a reason to explicitly specify these outputs and if so, which ones? Some of them are redundant. For example, the xray-reversed netlist is copied over by the wafove utility for no reason. And even some of the unique ones seem less than useful.

Second, it looks like wafove pretty much throws warnings out to standard error and print/logging info out to stdout. I am using ninja to catch those and redirect them to files. I am unsure if there were a non-zero exit status if ninja would report it in the terminal or not. If we don't do this, though, then we get tons of warnings about parameters not being found in IBUF instances. What is the preferred approach. @dallinjdahl will ninja still report that there was an error in the terminal even if stderr for the command itself is redirected?

Third, there are a large number of unused options from wafove, including things like seeding and opening waveforms, that I have not added, in order to keep the render dictionary for the associated build snippet small and more readable, and because for the use case of the provided flow, all we care about is equivalence. I propose that if we decide we want to take advantage of the rest of the many flags/options that wafove has, that they come in on other PRs as new flows require them.

dallinjdahl commented 8 months ago

Ideally we should list every output that wafove generates, and we should remove outputs from wafove that are irrelevant. That being said, to ease work, we can probably just list the unique wafove outputs and trust that nobody will depend on copied files, etc.

I just tested ninja with the following:

run:

#!/bin/sh
exec 2>&- #close stderr
exec 1>&- #close stdout
exit 1

build.ninja:

rule run
   command = touch $out; ./run

build phony: run

Running ninja produced the following output:

[1/1] touch phony; ./run
FAILED: phony 
touch phony; ./run
ninja: build stopped: subcommand failed.

I think we're good. In general, it's impossible for a child process to redirect the output from a parent process, so it shouldn't have any impact on ninja itself.

I think you're third point is fine. I would prefer to eliminate most of those flags entirely, but again, not critical right now.