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.

Sign up for GitHub

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

Jump to bottom

Emit a fileset-grouped manifest from Golden Gate #1117

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Emit a fileset-grouped manifest from Golden Gate #1117

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7aa9235
GG: emit a fileset-grouped manifest
davidbiancolin Jul 2, 2022
4ab2a38
conda: Add jq
davidbiancolin Jul 6, 2022
aa93a5a
make: Implement replace-rtl using manifest
davidbiancolin Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
make: Implement replace-rtl using manifest
  • Loading branch information
@davidbiancolin
davidbiancolin committed Jul 8, 2022
commit aa93a5a0cdf29d7a288cb38a66d45317b91bab24
58 changes: 44 additions & 14 deletions sim/target-agnostic.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ common_ld_flags := $(TARGET_LD_FLAGS) -lrt
header := $(GENERATED_DIR)/$(BASE_FILE_NAME).const.h
# The midas-generated simulator RTL which will be baked into the FPGA shell project
simulator_verilog := $(GENERATED_DIR)/$(BASE_FILE_NAME).sv
# Enumerates the different filesets produced by a GG compilation
gg_file_manifest := $(GENERATED_DIR)/$(BASE_FILE_NAME).file-manifest.json

# Call out some key files that will always be emitted by GG
statically_known_gg_outputs := \
$(gg_file_manifest) \
$(header) \
$(simulator_verilog)

####################################
# Golden Gate Invocation #
Expand All @@ -79,7 +87,7 @@ compile: $(simulator_verilog)
# empty recipe to help make understand multiple targets come from single recipe invocation
# without using the new (4.3) '&:' grouped targets see https://stackoverflow.com/a/41710495
.SECONDARY: $(simulator_verilog).intermediate
$(simulator_verilog) $(header) $(fame_annos): $(simulator_verilog).intermediate ;
$(statically_known_gg_outputs): $(simulator_verilog).intermediate ;

# Disable FIRRTL 1.4 deduplication because it creates multiple failures
# Run the 1.3 version instead (checked-in). If dedup must be completely disabled,
Expand All @@ -97,6 +105,19 @@ $(simulator_verilog).intermediate: $(FIRRTL_FILE) $(ANNO_FILE) $(SCALA_BUILDTOOL
grep -sh ^ $(GENERATED_DIR)/firrtl_black_box_resource_files.f | \
xargs cat >> $(simulator_verilog) # Append blackboxes to FPGA wrapper, if any

# 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.
Comment on lines +108 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fpga_src_files_query = $(call manifest_query,BitstreamCompile)
runtime_deploy_files_query = $(call manifest_query,RuntimeDeployment)


# If GG has run for this tuple before, these will be populated with the correct output files
# they will be empty otherwise.
fpga_src_files := $(addprefix $(GENERATED_DIR)/, $(shell $(fpga_src_files_query)))
runtime_deploy_files := $(addprefix $(GENERATED_DIR)/, $(shell $(runtime_deploy_files_query)))

####################################
# Runtime-Configuration Generation #
####################################
Expand Down Expand Up @@ -212,17 +233,17 @@ endif

fpga_work_dir := $(board_dir)/cl_$(name_tuple)
fpga_build_dir := $(fpga_work_dir)/build
fpga_design_dir:= $(fpga_work_dir)/design
verif_dir := $(fpga_work_dir)/verif
repo_state := $(fpga_work_dir)/design/repo_state
fpga_driver_dir:= $(fpga_work_dir)/driver

# Enumerates the subset of generated files that must be copied over for FPGA compilation
fpga_delivery_files = $(addprefix $(fpga_work_dir)/design/$(BASE_FILE_NAME), \
.sv .defines.vh .env.tcl \
.synthesis.xdc .implementation.xdc)

fpga_delivery_files = $(subst $(GENERATED_DIR)/,$(fpga_design_dir)/,$(fpga_src_files) $(simulator_verilog))

# Files used to run FPGA-level metasimulation
fpga_sim_delivery_files = $(addprefix $(fpga_driver_dir)/$(BASE_FILE_NAME), .runtime.conf) \
fpga_sim_delivery_files := \
$(subst $GENERATED_DIR,$(fpga_driver_dir)/,$(runtime_deploy_files)) \
$(fpga_driver_dir)/$(DESIGN)-$(PLATFORM)

$(fpga_work_dir)/stamp: $(shell find $(board_dir)/cl_firesim -name '*')
Expand All @@ -233,20 +254,29 @@ $(fpga_work_dir)/stamp: $(shell find $(board_dir)/cl_firesim -name '*')
$(repo_state): $(simulator_verilog) $(fpga_work_dir)/stamp
$(firesim_base_dir)/../scripts/repo_state_summary.sh > $(repo_state)

$(fpga_work_dir)/design/$(BASE_FILE_NAME)%: $(simulator_verilog) $(fpga_work_dir)/stamp
cp -f $(GENERATED_DIR)/*.ipgen.tcl $(@D) || true
cp -f $(GENERATED_DIR)/$(@F) $@
$(fpga_work_dir)/%/:
mkdir -p $@

$(fpga_driver_dir)/$(BASE_FILE_NAME)%: $(simulator_verilog) $(fpga_work_dir)/stamp
mkdir -p $(@D)
cp -f $(GENERATED_DIR)/$(@F) $@
$(fpga_design_dir)/%: $(GENERATED_DIR)/% | $(fpga_design_dir)/
cp -f $< $@

$(fpga_driver_dir)/%: $(GENERATED_DIR)/% | $(fpga_driver_dir)/
cp -f $< $@

$(fpga_driver_dir)/$(DESIGN)-$(PLATFORM): $($(PLATFORM))
$(fpga_driver_dir)/$(DESIGN)-$(PLATFORM): $($(PLATFORM)) | $(fpga_driver_dir)/
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 \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. make starts with manifest in existence
  2. when replace-rtl: target is defined (during reading of the makefiles), $(fpga_delivery_files) has contents defined by the then-existing manifest.
  3. For one of many reasons, GoldenGate needs to be rerun and it changes the manifest to include more stuff.
  4. Your recipe here in replace-rtl: won't copy the 'more stuff' in the manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

cd $(GENERATED_DIR); \
mkdir -p $(fpga_design_dir); \
cp -f $$($(fpga_src_files_query)) $(fpga_design_dir); \
cp -f $(simulator_verilog) $(fpga_design_dir); \
mkdir -p $(fpga_driver_dir); \
cp -f $$($(runtime_deploy_files_query)) $(fpga_driver_dir); \
fi \

.PHONY: replace-rtl

Expand Down