-
Notifications
You must be signed in to change notification settings - Fork 9
IP Caching #99
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
base: dev
Are you sure you want to change the base?
IP Caching #99
Changes from all commits
5f173ba
5587203
e549108
9b74125
9d5d782
4d33232
eea8347
f148987
f6c9549
e763690
bb1bdda
8fb44bb
2d72340
a890ec3
9d7c2dd
6c3b276
f6d6a0c
4d525cf
9241374
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,7 @@ | |
| from finn.transformation.fpgadataflow.insert_dwc import InsertDWC | ||
| from finn.transformation.fpgadataflow.insert_fifo import InsertFIFO | ||
| from finn.transformation.fpgadataflow.insert_tlastmarker import InsertTLastMarker | ||
| from finn.transformation.fpgadataflow.ip_cache import CachedIPGen | ||
| from finn.transformation.fpgadataflow.make_driver import ( | ||
| MakeCPPDriver, | ||
| MakePYNQDriverInstrumentation, | ||
|
|
@@ -117,7 +118,7 @@ | |
| from finn.transformation.streamline.reorder import MakeMaxPoolNHWC | ||
| from finn.transformation.streamline.round_thresholds import RoundAndClipThresholds | ||
| from finn.util.basic import get_liveness_threshold_cycles, get_rtlsim_trace_depth | ||
| from finn.util.exception import FINNUserError | ||
| from finn.util.exception import FINNConfigurationError, FINNUserError | ||
| from finn.util.logging import log | ||
| from finn.util.test import execute_parent | ||
|
|
||
|
|
@@ -521,6 +522,49 @@ def step_minimize_bit_width(model: ModelWrapper, cfg: DataflowBuildConfig): | |
| return model | ||
|
|
||
|
|
||
| def _make_hls_estimate_report(model: ModelWrapper, cfg: DataflowBuildConfig) -> None: | ||
| report_dir = cfg.output_dir + "/report" | ||
| os.makedirs(report_dir, exist_ok=True) | ||
| estimate_layer_resources_hls = model.analysis(hls_synth_res_estimation) | ||
| estimate_layer_resources_hls["total"] = aggregate_dict_keys(estimate_layer_resources_hls) | ||
| with open(report_dir + "/estimate_layer_resources_hls.json", "w") as f: | ||
| json.dump(estimate_layer_resources_hls, f, indent=2) | ||
|
|
||
|
|
||
| def step_ip_generation(model: ModelWrapper, cfg: DataflowBuildConfig) -> ModelWrapper: | ||
| """Unified step, that does what step_hw_codegen and step_hw_ipgen did before. (With cache!).""" | ||
| if cfg.use_ip_caching: | ||
| clk = cfg._resolve_hls_clk_period() | ||
| if clk is None: | ||
| # TODO: Change into a logging error instead of an exception? | ||
| raise FINNConfigurationError( | ||
| "Please specify synth_clk_period_ns in your build " | ||
| "config (and optionally hls_clk_period_ns) before " | ||
| "generating IPs!" | ||
| ) | ||
| model = model.transform( | ||
| CachedIPGen( | ||
| cfg.ip_cache_hashfunction, | ||
| include_prepare_ip=True, | ||
| cache_clock=cfg.cache_hls_clk_period, | ||
| fpgapart=cfg._resolve_fpga_part(), | ||
| clk=clk, | ||
| cache_fpgapart=cfg.cache_fpgapart, | ||
| ) | ||
| ) | ||
| else: | ||
| model = model.transform(PrepareIP(cfg._resolve_fpga_part(), cfg._resolve_hls_clk_period())) | ||
| model = model.transform(HLSSynthIP()) | ||
| model = model.transform(ReplaceVerilogRelPaths()) | ||
| _make_hls_estimate_report(model, cfg) | ||
|
|
||
| if VerificationStepType.NODE_BY_NODE_RTLSIM in cfg._resolve_verification_steps(): | ||
| model = model.transform(PrepareRTLSim()) | ||
| model = model.transform(SetExecMode("rtlsim")) | ||
| verify_step(model, cfg, "node_by_node_rtlsim", need_parent=True) | ||
| return model | ||
|
|
||
|
|
||
| def step_hw_codegen(model: ModelWrapper, cfg: DataflowBuildConfig): | ||
| """Generate Vitis HLS code to prepare HLSBackend nodes for IP generation. | ||
| And fills RTL templates for RTLBackend nodes.""" | ||
|
|
@@ -533,15 +577,36 @@ def step_hw_ipgen(model: ModelWrapper, cfg: DataflowBuildConfig): | |
| """Run Vitis HLS synthesis on generated code for HLSBackend nodes, | ||
| in order to generate IP blocks. For RTL nodes this step does not do anything.""" | ||
|
|
||
| model = model.transform(HLSSynthIP()) | ||
| model = model.transform(ReplaceVerilogRelPaths()) | ||
| report_dir = cfg.output_dir + "/report" | ||
| os.makedirs(report_dir, exist_ok=True) | ||
| estimate_layer_resources_hls = model.analysis(hls_synth_res_estimation) | ||
| estimate_layer_resources_hls["total"] = aggregate_dict_keys(estimate_layer_resources_hls) | ||
| with open(report_dir + "/estimate_layer_resources_hls.json", "w") as f: | ||
| json.dump(estimate_layer_resources_hls, f, indent=2) | ||
| if cfg.use_ip_caching: | ||
| log.info("Using IP cache to fetch generated IPs...") | ||
| clk = cfg._resolve_hls_clk_period() | ||
| if clk is None and cfg.cache_hls_clk_period: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this level of error handling if somehow no (HLS) clock is specified. It adds bloat to this already long file and this condition shouldn't be possible, especially with the latest changes to DataflowBuildConfig, where synth_clk_period_ns is no longer The same comment applies to step_ip_generation.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was before the Small Fixes PR, so the clocks could still be None, and thus i checked them. Going to remove this. |
||
| log.critical( | ||
| "No HLS/general synthesis clock period was specified, but required for " | ||
| "caching (cfg.cache_hls_clk_period). Skipping caching for safety. " | ||
| "Executing just HLSSynthIP()..." | ||
| ) | ||
| model = model.transform(HLSSynthIP()) | ||
| else: | ||
| # If clk is None but we don't use it anways, give it some placeholder value | ||
| if clk is None: | ||
| clk = 0 | ||
| model = model.transform( | ||
| CachedIPGen( | ||
| cfg.ip_cache_hashfunction, | ||
| cache_clock=cfg.cache_hls_clk_period, | ||
| include_prepare_ip=False, | ||
| fpgapart=cfg._resolve_fpga_part(), | ||
| clk=clk, | ||
| cache_fpgapart=cfg.cache_fpgapart, | ||
| ) | ||
| ) | ||
| else: | ||
| log.info("Generating all IPs from scratch...") | ||
| model = model.transform(HLSSynthIP()) | ||
|
|
||
| model = model.transform(ReplaceVerilogRelPaths()) | ||
| _make_hls_estimate_report(model, cfg) | ||
| if VerificationStepType.NODE_BY_NODE_RTLSIM in cfg._resolve_verification_steps(): | ||
| model = model.transform(PrepareRTLSim()) | ||
| model = model.transform(SetExecMode("rtlsim")) | ||
|
|
@@ -1059,6 +1124,7 @@ def step_deployment_package(model: ModelWrapper, cfg: DataflowBuildConfig): | |
| "step_apply_folding_config": step_apply_folding_config, | ||
| "step_minimize_bit_width": step_minimize_bit_width, | ||
| "step_generate_estimate_reports": step_generate_estimate_reports, | ||
| "step_ip_generation": step_ip_generation, | ||
| "step_hw_codegen": step_hw_codegen, | ||
| "step_hw_ipgen": step_hw_ipgen, | ||
| "step_set_fifo_depths": step_set_fifo_depths, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,27 @@ def resolve_deps_path(deps: Path | None, settings: dict) -> Path | None: | |
| return None | ||
|
|
||
|
|
||
| def resolve_cache_path(cache: Path | None, settings: dict) -> Path: | ||
| """Resolve the path to the IP cache. Always returns a valid Path. | ||
|
|
||
| Resolution order is: | ||
| Command Line Argument -> Environment var -> Settings -> Default (finn-plus/FINN_IP_CACHE) | ||
| """ | ||
| if cache is not None: | ||
| return cache | ||
| if "FINN_IP_CACHE" in os.environ.keys(): | ||
| p = Path(os.environ["FINN_IP_CACHE"]) | ||
| if p.is_absolute(): | ||
| return p | ||
| return Path(__file__).parent.parent.parent.parent / p | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. REMINDER: This needs (and following occurences) to be updated potentially since directory structure changed. |
||
| if "FINN_IP_CACHE" in settings.keys(): | ||
| p = Path(settings["FINN_IP_CACHE"]) | ||
| if p.is_absolute(): | ||
| return p | ||
| return Path(__file__).parent.parent.parent.parent / p | ||
| return Path(__file__).parent.parent.parent.parent / "FINN_IP_CACHE" | ||
|
|
||
|
|
||
| def resolve_num_workers(num: int, settings: dict) -> int: | ||
| """Resolve the number of workers to use. Uses 75% of cores available as default fallback""" | ||
| if num > -1: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step_hw_ipgen has more info logging here than this step. Should be consistent between both (and probably lean towards verbose logging for a critical feature like this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean generally more logging during the IP Cache transformation or during the steps?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wherever it fits best. I think we should always log summary information, like
And in verbose/debug mode info like this should be printed per-layer.
I think most of this is already in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look what gets logged when and revise it if necessary.