Support default "tool" and "flow" flags when using flow API - #799
Support default "tool" and "flow" flags when using flow API#799mxsparks wants to merge 1 commit into
Conversation
| except SyntaxError as e: | ||
| logger.error(str(e)) | ||
| exit(1) | ||
| except RuntimeError as e: | ||
| logger.error(str(e)) | ||
| exit(1) |
There was a problem hiding this comment.
This could be simplify:
| except SyntaxError as e: | |
| logger.error(str(e)) | |
| exit(1) | |
| except RuntimeError as e: | |
| logger.error(str(e)) | |
| exit(1) | |
| except (SyntaxError, RuntimeError) as e: | |
| logger.error(str(e)) | |
| exit(1) |
| if flow_options and "tool" in flow_options and "tool" not in flags: | ||
| flags["tool"] = flow_options["tool"] |
There was a problem hiding this comment.
How about setting the tool flag based on the flow if the tool is not present 🤔 For example, vivado flow (because it is using the Vivado tool):
| if flow_options and "tool" in flow_options and "tool" not in flags: | |
| flags["tool"] = flow_options["tool"] | |
| if "tool" not in flags: | |
| flags["tool"] = flow_options["tool"] if flow_options and "tool" in flow_options else flow |
|
This is actually by design. The tool_TOOLNAME flags don't make sense for the flow API as there can be flows using multiple tools. The intention is to deprecate the Adding an implicit flow_FLOWNAME flag could still be considered, but I would argue that it is better to set an explicit flag in the relevant targets to convey the intention more clearly. e.g. instead of checking that the flow is sim, the actual thing we want to do is to use a simulation model or decrease some reset timer. Perhaps we also want to use the shortened reset timer in a formal flow, or do some simulations without the simulation model. That also helps with reuse, when the core files are perhaps used in a new flow that the original author didn't consider. |
@olofk But there is also the if flow_options and "tool" in flow_options and "tool" not in flags:
flags["tool"] = flow_options["tool"]It is using the files:
unisim:
depend:
- xilinx:unisim:vcomponents
targets:
default:
filesets:
- '!tool_vivado ? (!tool_xsim ? (unisim))'
flow: sim
flow_options:
tool: xceliumIn case of Xilinx Then user can select simulator in the I have similar changes like @mxsparks is proposing in my forked version of FuseSoC. Otherwise handling simulation libraries when using the |
Agreed. We currently have a mix of tool and flow API usage, and the current behavior requires me to revisit every flow-API target in several hundred dependent cores and add a redundant |
Fixes an issue where the default
tool_TOOLNAMEflags were never being set if not using the tool API. Also adds a defaultflow_FLOWNAMEflag.Passes ruff and tox (no new unit tests added).