Emit a fileset-grouped manifest from Golden Gate #1117

davidbiancolin · 2022-07-02T07:10:48Z

This should make it more clear what files are used where, allowing me to clean up some of the replace-rtl logic (we can drop per-platform junk), and easing integration into other build systems.

This output file manifest is rooted at TargetDir, all listed paths are relative to that directory. Note this does not include verilog blackboxes, and the .f. Collecting those files and including them here is going to be a chunk of work that might require duplicating a bunch of FIRRTL code since black box file emission is done ad-hoc does not reuse the CustomFileEmission machinery.

Here's an example emission:

{
  "BitstreamCompile":[
    "FireSim-generated.synthesis.xdc",
    "FireSim-generated.implementation.xdc",
    "FireSim-generated.env.tcl",
    "FireSim-generated.ila_firesim.ipgen.tcl",
    "FireSim-generated.defines.vh"
  ],
  "MetasimulatorCompile":[
    "FireSim-generated.const.h",
    "FireSim-generated.const.vh"
  ],
  "RuntimeDeployment":[
    "FireSim-generated.runtime.conf"
  ],
  "Unclassified":[
    
  ],
  "EmittedVerilogFiles":[
    "FireSim-generated.sv"
  ]
}

Some of the scala names are a little jank, i think Fileset is a better, shorter description of what i was trying to capture with DownstreamFlow / Dependency.

Related PRs / Issues

UI / API Impact

Verilog / AGFI Compatibility

Contributor Checklist

Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
Did you add Scaladoc/docstring/doxygen to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous prints/debugging code?
Did you state the UI / API impact?
Did you specify the Verilog / AGFI compatibility impact?

If applicable, did you regenerate and publicly share default AGFIs?

If applicable, did you apply the ci:fpga-deploy label?

If applicable, did you apply the Please Backport label?

Reviewer Checklist (only modified by reviewer)

Is the title suitable for inclusion in the changelog and does the PR have a changelog:<topic> label?
Did you mark the proper release milestone?
Did you check whether all relevant Contributor checkboxes have been checked?

timsnyder-siv

Overall, I think it is an improvement to have GG explicitly enumerate the optional artifacts that it is creating so that the makefile read that manifest and use it to do the right thing rather than having hardcoded artifact lists that are far removed from GG. I also think it is a good idea to use a build-tool-agnostic representation of the manifest. JSON is fine.

However, I think the current implementation isn't correct because it won't copy files that are added into the manifest by any GoldenGate run done by the current make invocation. I think the solution to this is to:

create a tool that reads your JSON and spits out a generated makefrag representation of it that you explicitly include (note the intentional use of include instead of -include)
Add an explicit dependence from the makefrag to the manifest to the inputs of GoldenGate
Rethink how Distributed Elaboration works because 1&2 is definitely doing to break the 'cute' introspection of the replace-rtl.sh script I send to the distributed builders. One possiblity might be to add an additional cmdline argument like --gen-manifest-only to GoldenGate where it doesn't actually run and generate the 'real' outputs because those are computationally expensive (more like, my worst nightmare), but it does generate the manifest correctly (because hopefully that should be easy by comparison).

timsnyder-siv · 2022-07-08T22:24:36Z

sim/midas/src/main/scala/midas/stage/GoldenGateFileEmission.scala

+
+  val fileManifestSuffix = ".file-manifest.json"
+  val emittedVerilogKey = "EmittedVerilogFiles"
+  val unclassifiedKey   = "UnclassifiedFiles"


Is it a good idea to comment on the behavior of the unclassifiedKey here? Without RTC, I'm guessing that those files will go to all downstream flows? 🤷

Yeah, you point to this object from other places to say 'go look here', you need to explain what is expected for unclassifiedKey.

timsnyder-siv · 2022-07-08T22:31:29Z

sim/target-agnostic.mk

+# Assembles strings that can be used in $(shell <query>) commands to look up
+# filesets from GG's output manifest. These are strings so that can be emitted
+# literally into a script to run on a remote as part of distributed elaboration.


Aww, how thoughtful... you actually tried not to break my unmerged and unloved feature even more than it has been already. 😍

timsnyder-siv · 2022-07-08T22:47:33Z

sim/target-agnostic.mk

 	cp -f $< $@

 # Goes as far as setting up the build directory without running the cad job
 # Used by the manager before passing a build to a remote machine
-replace-rtl: $(fpga_delivery_files) $(fpga_sim_delivery_files)
+replace-rtl: $(fpga_delivery_files) $(fpga_sim_delivery_files) $(fpga_work_dir)/stamp
+	if [[ "$(firstword $(fpga_delivery_files))" != "$<" ]]; then \


I need a decoder ring to understand what this conditional is doing, when is $(firstword $(fpga_delivery_files)) not going to be $<? I thought that $< is the first prerequisite.

You're doing something special here and handling the case where $(fpga_delivery_files) is the emptystring because you haven't run before and the manifest doesn't exist yet, so $< will be stamp... probably worth a comment at least.

Why would you not always defer evaluation of the manifest until the downstream target recipe is executing? I think it would guard against the case where the manifest is updated by rerunning GG to contain more or less files.

To be more verbose, I'm concerned about:

make starts with manifest in existence

when replace-rtl: target is defined (during reading of the makefiles), $(fpga_delivery_files) has contents defined by the then-existing manifest.

For one of many reasons, GoldenGate needs to be rerun and it changes the manifest to include more stuff.

Your recipe here in replace-rtl: won't copy the 'more stuff' in the manifest.

If you're worried about unnecessary copying causing the timestamps to be updated in $(fpga_whatever_dir), you could use rsync instead of cp since it will use the files hash to determine whether to really copy it...

timsnyder-siv · 2022-07-08T23:23:52Z

sim/target-agnostic.mk

+# Assembles strings that can be used in $(shell <query>) commands to look up
+# filesets from GG's output manifest. These are strings so that can be emitted
+# literally into a script to run on a remote as part of distributed elaboration.
+manifest_query             = cat $(gg_file_manifest) 2> /dev/null | jq -r '.$(1) | .[]'


Why not just:

manifest_query = jq -r '.$(1) | .[]' $(gg_file_manifest)

?

Oh, nevermind, I see what happens, jq just sits there if $(gg_file_manifest) is empty string because no arg gets passed to jq and it sits spinning on STDIN... What if you put quotes around '$(gg_file_manifest)' does it avoid that problem and make it slightly easier to read?

An alternative that I will agree is not easier to read would be to put another make function call in your function because that's when $(gg_file_manifest) has to be defined and make it slightly more obvious to users when the makefile logic is broken:

Suggested change

manifest_query = cat $(gg_file_manifest) 2> /dev/null | jq -r '.$(1) | .[]'

manifest_query = $(if $(gg_file_manifest), jq -r '.$(1) | .[]' '$(gg_file_manifest)', $(error gg_file_manifest must be defined))

NOTE: my call to $(if) is using the documented gnumake convention that any non-empty string evaluates to true

davidbiancolin added the changelog:added Put PR title in 'Added' section of changelog label Jul 2, 2022

GG: emit a fileset-grouped manifest

7aa9235

davidbiancolin force-pushed the fileset-based-manifest branch from fc29bd9 to 7aa9235 Compare July 2, 2022 07:14

davidbiancolin assigned russell-horvath Jul 5, 2022

davidbiancolin added 2 commits July 6, 2022 16:06

conda: Add jq

4ab2a38

make: Implement replace-rtl using manifest

aa93a5a

timsnyder-siv requested changes Jul 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit a fileset-grouped manifest from Golden Gate #1117

Emit a fileset-grouped manifest from Golden Gate #1117

davidbiancolin commented Jul 2, 2022

timsnyder-siv left a comment

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

timsnyder-siv Jul 8, 2022

	manifest_query = cat $(gg_file_manifest) 2> /dev/null \| jq -r '.$(1) \| .[]'
	manifest_query = $(if $(gg_file_manifest), jq -r '.$(1) \| .[]' '$(gg_file_manifest)', $(error gg_file_manifest must be defined))

Emit a fileset-grouped manifest from Golden Gate #1117

Are you sure you want to change the base?

Emit a fileset-grouped manifest from Golden Gate #1117

Conversation

davidbiancolin commented Jul 2, 2022

Related PRs / Issues

UI / API Impact

Verilog / AGFI Compatibility

Contributor Checklist

Reviewer Checklist (only modified by reviewer)

timsnyder-siv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment