make: parallel build #1202
make: parallel build #1202
Conversation
Drop shell for loops in favor of makefile pattern rules so that make can directly run targets in parallel (using -j). This doesn't affect Vivado's own settings. Implicitly, this also fixes missing build dependencies such as: make: *** No rule to make target '../../../library/axi_clkgen/component.xml', needed by 'adrv9009_zcu102.sdk/system_top.xsa'. Stop. As a benchmark: $ lscpu Architecture: x86_64 CPU op-mode(s): 32-bit, 64-bit Byte Order: Little Endian Address sizes: 46 bits physical, 48 bits virtual CPU(s): 20 On-line CPU(s) list: 0-19 Thread(s) per core: 1 Core(s) per socket: 14 Socket(s): 1 NUMA node(s): 1 Vendor ID: GenuineIntel CPU family: 6 Model: 154 Model name: 12th Gen Intel(R) Core(TM) i9-12900H Stepping: 3 CPU MHz: 2900.000 CPU max MHz: 5000.0000 CPU min MHz: 400.0000 BogoMIPS: 5836.80 ... $ make -C projects/adrv9009/zcu102/ clean-all $ time make -C projects/adrv9009/zcu102/ lib ... real 9m27.223s user 9m2.556s sys 0m32.358s $ make -C projects/adrv9009/zcu102/ clean-all $ time make -C projects/adrv9009/zcu102/ -j8 lib ... real 1m54.639s user 16m26.512s sys 1m2.317s i.e. about 5 times faster to build IP core dependencies. Signed-off-by: Liam Beguin <[email protected]>
liambeguin
requested review from
acostina,
AndreiGrozav and
PopPaul2021
as code owners
| @@ -83,22 +83,24 @@ ifneq ($(XILINX_DEPS),) | |||
|
|
|||
| XILINX_DEPS += $(GENERIC_DEPS) | |||
| XILINX_DEPS += $(HDL_LIBRARY_PATH)scripts/adi_ip_xilinx.tcl | |||
| XILINX_DEPS += $(foreach dep,$(XILINX_LIB_DEPS),$(HDL_LIBRARY_PATH)$(dep)/component.xml) | |||
| _XILINX_LIB_DEPS = $(foreach dep,$(XILINX_LIB_DEPS),$(HDL_LIBRARY_PATH)$(dep)/component.xml) | |||
There was a problem hiding this comment.
Could you also do this to library/Makefile ?
I mean, move the project list to a variable to be used as a dependencies, so we can also use the -j .
There, I think it is better to keep the list instead of globing.
(if not, I can do it later also)
There was a problem hiding this comment.
okay, I'll have a look
liambeguin
force-pushed
the
parallel-build
branch
from
ba54183 to
c15abbf
Compare
|
Hi @liambeguin, @AndreiGrozav commented that there is an issue with the pr that when a library fails during the generation of the component.xml file, the next Consider the following workflow Notice how the library axi_spi_engine_ip should rebuild after being edited. I think we have to:
|
library/scripts/library.mk
Outdated
| @@ -91,7 +91,7 @@ xilinx: component.xml | |||
| component.xml: $(XILINX_DEPS) $(_XILINX_INTF_DEPS) $(_XILINX_LIB_DEPS) | |||
| -rm -rf $(CLEAN_TARGET) | |||
| $(call build, \ | |||
| $(VIVADO) $(LIBRARY_NAME)_ip.tcl, \ | |||
| flock component.xml -c "$(VIVADO) $(LIBRARY_NAME)_ip.tcl", \ | |||
There was a problem hiding this comment.
Thanks you for your initiative and effort. This optimization will be appreciated by everyone.
Let me continue with what I would change. There are two issues at this line:
-
flock creates an empty component.xml, regardless of the success/fail status, an component.xml will result after the make call. So, when someone will call make, for the second time, a library that failed, will not be rebuild. Because the component.xml is there.
-
We want any error in `the library build to stop the whole build. You wrote this in another commit:
"Implicitly, this also fixes missing build dependencies such as:
make: *** No rule to make target '../../../library/axi_clkgen/component.xml', needed by 'adrv9009_zcu102.sdk/system_top.xsa'. Stop."
This is not a feature for us. If someone uses a release or stable branch, it does not matter that much. But in development it surely does. If a library build fails, the project build that depends on that library will fail in the end.
With this in mind the uglier scenarios will arise on the support forums. For us, trying to figure out why a project build fails for someone who does not give us the whole log or does not states the modifications of libraries in a reference design.
Also, working on a project with high resource demands, for which a build takes 3 hours or more will result in wasted time and resources for a developer or for our CI flow.
My suggestions is to remove the component.xml if the build fails and pass the error status$(call build, \ flock component.xml -c "$(VIVADO) $(LIBRARY_NAME)_ip.tcl", \ $(LIBRARY_NAME)_ip.log, \ $(HL)$(LIBRARY_NAME)$(NC) library) || (ERR=$$? && rm component.xml && exit $$ERR);
If you have a better solution, please add it.
There was a problem hiding this comment.
Hi @AndreiGrozav,
Thanks for testing this, and apologies for the delay.
- flock creates an empty component.xml, regardless of the success/fail status, an component.xml will result after the make call. So, when someone will call make, for the second time, a library that failed, will not be rebuild. Because the component.xml is there.
interesting! I definitely missed that. I like your approach, I can't really find anything within flock to take care of cleaning up if the command fails.
actually, maybe .DELETE_ON_ERROR would work here. I don't know if we want to be more conservative and take care of this in individual targets.
Do you have a preference?
- We want any error in `the library build to stop the whole build.
I definitely agree with that, the commit message might be misleading. If I'm not mistaken, this error message happens if you try using make -j before applying any commits from this branch.
Assuming I address the issue in 1. if an IP core build fails the project target won't get run because make will detect an issue in build dependencies. The only difference with master is that it would finish current builds.
There was a problem hiding this comment.
I don't have a preference.
projects/scripts/project-xilinx.mk
Outdated
| fi; \ | ||
| done | ||
|
|
||
| $(HDL_LIBRARY_PATH)%/component.xml: |
There was a problem hiding this comment.
This change causes the project to be unable to track library changes.
Library dependencies are managed in library.mk, so we should always run make on all library dependencies and let library.mk manage and skip them.
I will commit this change to your branch, if you do not like it, feel free to revert it.
(The other issue with "a library that failed will not be rebuilt" I'll leave to @liambeguin and @AndreiGrozav, as I have no preference for either solution).
There was a problem hiding this comment.
Isn't this related to what @AndreiGrozav was saying? about the fact that flock is creating a file even if the build fails?
There was a problem hiding this comment.
It's slightly different.
Andrei problem: a failed component.xml build have a newer timestamp than the dependencies (library.mk level).
My problem: if component.xml file exists, it never rebuilds the library even if the dependencies changes (xilinx-project.mk level).
gastmaier
force-pushed
the
parallel-build
branch
from
1a3229c to
a6d506e
Compare
library/Makefile
Outdated
| @@ -14,7 +14,7 @@ _LIBS := $(shell find . -mindepth 2 -name Makefile -exec dirname {} \; | sort) | |||
|
|
|||
| FORCE: | |||
| $(_LIBS): FORCE | |||
| $(MAKE) -C $@ $(TARGET) | |||
| $(MAKE) -C $@ | |||
There was a problem hiding this comment.
This is incorrect. $(TARGET) is a target-specific variables and is set on line 19. it also default to an empty string to build the default target.
There was a problem hiding this comment.
You are correct, sorry.
The issue was the lib: TARGET:="" which caused make (version 4.3) to throw the error:
make: *** empty string invalid as file name. Stop.
When merging, we are going to squash these commits.
There was a problem hiding this comment.
The issue was the
lib: TARGET:=""which caused make (version 4.3) to throw the error
sorry about that.
should I still make the flock change?
There was a problem hiding this comment.
Yes, we need to remove the component.xml file if the build fails.
There was a problem hiding this comment.
.DELETE_ON_ERROR: worked, thanks
During a highly parallelized build, it's possible that multiple identical targets be started simultaneously. This is exacerbated by long target execution time. Add an advisory lock on the component.xml file so that multiple instances of Vivado don't try to overwrite the same file. This doesn't prevent a target from being checked multiple times, but forces it to only be run a single time. DELETE_ON_ERROR is required here because flock will create component.xml even if the target fails breaking dependencies at the project level. Signed-off-by: Liam Beguin <[email protected]>
Search for all Makefiles under library/ and use the directory name to generate a list of libraries to build. With this scheme, `make lib` can be run in parallel. Signed-off-by: Liam Beguin <[email protected]>
Shrink and parallelize clean targets by leveraging target-specific variables. This allows us to reuse the generic component.xml target and trigger a clean instead of a build. Signed-off-by: Liam Beguin <[email protected]>
Library dependencies are managed in library.mk, this commit ensures that library.mk manage and skip them by setting a dummy FORCE target. When building a project, it is desired to rebuild all libraries that have been modified. Previously, the solution placed component.xml files as dependencies, skipping these targets if the file already exists, but ignored changes to them. Signed-off-by: Jorge Marques <[email protected]>
liambeguin
force-pushed
the
parallel-build
branch
from
a6edc6f to
bd63722
Compare
Since we now support parallel build, differ message from starting and finishing the build. A more sophisticated solution could be develop but Makefile is not good for shared global variables. Signed-off-by: Jorge Marques <[email protected]>
There was a problem hiding this comment.
The concerns have been addressed by the author and reviewers, and this PR effectively improves/speeds up our workflow and CI.
Great job @liambeguin!
|
There is a problem with intel libraries (e.g. intel/adi_jesd204 , intel/avl_adxcvr) that causes |
|
Thanks @gastmaier !
The same changes can be made to the intel build, but since I don't have a setup to test on, I thought that could be done in a second pass. |
Add support for touching .timestamp_intel as separated targets. Signed-off-by: Jorge Marques <[email protected]>
xilinx and intel were set at lib target, having empty value at the actual target (component.xml, .timestamp_intel), which default to building both. Signed-off-by: Jorge Marques <[email protected]>
I mistakenly deleted the FORCE line in the last commit. Signed-off-by: Jorge Marques <[email protected]>
Wrap library.mk files around a lock file, avoiding a vivado call when other dependency already queue a vivado library call. The next calls will properly resolve the dependencies against component.xml timestamp and skip it. Signed-off-by: Jorge Marques <[email protected]>
|
There is one issue with the flock approach. Before the last commit:BUILDING axi_clkgen BUILDING axi_i2s_adi BUILDING axi_spdif_tx Building axi_i2s_adi library [/home/me/Documents/adi/hdl-make2/library/axi_i2s_adi/axi_i2s_adi_ip.log] ... Building axi_clkgen library [/home/me/Documents/adi/hdl-make2/library/axi_clkgen/axi_clkgen_ip.log] ... Building axi_spdif_tx library [/home/me/Documents/adi/hdl-make2/library/axi_spdif_tx/axi_spdif_tx_ip.log] ... BUILDING axi_hdmi_tx Building axi_hdmi_tx library [/home/me/Documents/adi/hdl-make2/library/axi_hdmi_tx/axi_hdmi_tx_ip.log] ... BUILDING axi_ad4858 BUILDING util_cdc Building axi_ad4858 library [/home/me/Documents/adi/hdl-make2/library/axi_ad4858/axi_ad4858_ip.log] ... Building util_cdc library [/home/me/Documents/adi/hdl-make2/library/util_cdc/util_cdc_ip.log] ... BUILDING axi_sysid BUILDING sysid_rom BUILDING util_i2c_mixer Building axi_sysid library [/home/me/Documents/adi/hdl-make2/library/axi_sysid/axi_sysid_ip.log] ... Building sysid_rom library [/home/me/Documents/adi/hdl-make2/library/sysid_rom/sysid_rom_ip.log] ... Building util_i2c_mixer library [/home/me/Documents/adi/hdl-make2/library/util_i2c_mixer/util_i2c_mixer_ip.log] ... BUILDING util_cpack2 BUILDING util_cdc Building util_cpack2 library [/home/me/Documents/adi/hdl-make2/library/util_pack/util_cpack2/util_cpack2_ip.log] ... Building util_cdc library [/home/me/Documents/adi/hdl-make2/library/util_cdc/util_cdc_ip.log] ... BUILDING util_cdc Building util_cdc library [/home/me/Documents/adi/hdl-make2/library/util_cdc/util_cdc_ip.log] ... Build util_cdc library [/home/me/Documents/adi/hdl-make2/library/util_cdc/util_cdc_ip.log] FAILED For details see /home/me/Documents/adi/hdl-make2/library/util_cdc/util_cdc_ip.log After the last commit:BUILDING axi_clkgen Building axi_clkgen library [/home/me/Documents/adi/hdl-make/library/axi_clkgen/axi_clkgen_ip.log] ... BUILDING axi_i2s_adi Building axi_i2s_adi library [/home/me/Documents/adi/hdl-make/library/axi_i2s_adi/axi_i2s_adi_ip.log] ... BUILDING util_i2c_mixer BUILDING axi_hdmi_tx BUILDING axi_sysid Building util_i2c_mixer library [/home/me/Documents/adi/hdl-make/library/util_i2c_mixer/util_i2c_mixer_ip.log] ... BUILDING sysid_rom Building axi_hdmi_tx library [/home/me/Documents/adi/hdl-make/library/axi_hdmi_tx/axi_hdmi_tx_ip.log] ... Building axi_sysid library [/home/me/Documents/adi/hdl-make/library/axi_sysid/axi_sysid_ip.log] ... BUILDING axi_spdif_tx Building sysid_rom library [/home/me/Documents/adi/hdl-make/library/sysid_rom/sysid_rom_ip.log] ... BUILDING axi_ad4858 BUILDING util_cpack2 Building axi_spdif_tx library [/home/me/Documents/adi/hdl-make/library/axi_spdif_tx/axi_spdif_tx_ip.log] ... Building axi_ad4858 library [/home/me/Documents/adi/hdl-make/library/axi_ad4858/axi_ad4858_ip.log] ... Building util_cpack2 library [/home/me/Documents/adi/hdl-make/library/util_pack/util_cpack2/util_cpack2_ip.log] ... BUILDING util_cdc Building util_cdc library [/home/me/Documents/adi/hdl-make/library/util_cdc/util_cdc_ip.log] ... Build util_cdc library [/home/me/Documents/adi/hdl-make/library/util_cdc/util_cdc_ip.log] OK Build axi_sysid library [/home/me/Documents/adi/hdl-make/library/axi_sysid/axi_sysid_ip.log] OK Build util_i2c_mixer library [/home/me/Documents/adi/hdl-make/library/util_i2c_mixer/util_i2c_mixer_ip.log] OK BUILDING axi_pwm_gen Building axi_pwm_gen library [/home/me/Documents/adi/hdl-make/library/axi_pwm_gen/axi_pwm_gen_ip.log] ... BUILDING util_axis_fifo Building util_axis_fifo library [/home/me/Documents/adi/hdl-make/library/util_axis_fifo/util_axis_fifo_ip.log] ... Build sysid_rom library [/home/me/Documents/adi/hdl-make/library/sysid_rom/sysid_rom_ip.log] OK Build axi_clkgen library [/home/me/Documents/adi/hdl-make/library/axi_clkgen/axi_clkgen_ip.log] OK Build util_cpack2 library [/home/me/Documents/adi/hdl-make/library/util_pack/util_cpack2/util_cpack2_ip.log] OK Build axi_spdif_tx library [/home/me/Documents/adi/hdl-make/library/axi_spdif_tx/axi_spdif_tx_ip.log] OK Build axi_i2s_adi library [/home/me/Documents/adi/hdl-make/library/axi_i2s_adi/axi_i2s_adi_ip.log] OK Build axi_hdmi_tx library [/home/me/Documents/adi/hdl-make/library/axi_hdmi_tx/axi_hdmi_tx_ip.log] OK Build axi_pwm_gen library [/home/me/Documents/adi/hdl-make/library/axi_pwm_gen/axi_pwm_gen_ip.log] OK Build util_axis_fifo library [/home/me/Documents/adi/hdl-make/library/util_axis_fifo/util_axis_fifo_ip.log] OK BUILDING axi_dmac Building axi_dmac library [/home/me/Documents/adi/hdl-make/library/axi_dmac/axi_dmac_ip.log] ... Build axi_ad4858 library [/home/me/Documents/adi/hdl-make/library/axi_ad4858/axi_ad4858_ip.log] OK Build axi_dmac library [/home/me/Documents/adi/hdl-make/library/axi_dmac/axi_dmac_ip.log] OK Building ad4858_fmcz_zed project [/home/me/Documents/adi/hdl-make/projects/ad4858_fmcz/zed/ad4858_fmcz_zed_vivado.log] ... Both with |
|
@gastmaier I did notice the same issue with highly parallelized builds from 85e390e
but didn't think it was an issue since Since the issue seems to be coming from the parent task not protecting properly, why not move the flock call there instead of implementing a custom lock? maybe something like this: I think it also help to have |
component.xml cannot be used as lock file because generating the lock would create a new timestamp and library.mk would think the library is up-to-date. Signed-off-by: Jorge Marques <[email protected]>
|
I pushed a solution with flock. |
Catch error from the target command, instead of flock exit code. Flock only exits with a error code for issues with the command itself, like a timed out lock, never from target the command. Signed-off-by: Jorge Marques <[email protected]>
|
For reviewers, a list of checks that should be answered 'yes' to pass. Also, does the commit create new artifacts? Are they properly git gitnored? |
Signed-off-by: Jorge Marques <[email protected]>
Fixup "make clean all" not executing second target since the dependencies were the same. Same approach as projects-toplevel.mk. Signed-off-by: Jorge Marques <[email protected]>
|
I'll squash into a single, since individual commits are not functional in this case. |
The .lock file is used to forbid multiple concurrent builds of the same library at the same time, allowing safe parallelization. This should have been added to pr #1202. Signed-off-by: Jorge Marques <[email protected]>
Drop shell for loops in favor of makefile pattern rules, so make can run targets in parallel using -j. This doesn't affect Vivado's own settings. As a benchmark, 12th Gen Intel(R) Core(TM) i9-12900H 5GHz(max): $ make -C projects/adrv9009/zcu102/ clean-all $ time make -C projects/adrv9009/zcu102/ -j$CORES lib CORES=1: real 9m27.223s user 9m2.556s sys 0m32.358s CORES=8: real 1m54.639s user 16m26.512s sys 1m2.317s i.e. about 5 times faster to build IP core dependencies. Signed-off-by: Liam Beguin <[email protected]> Signed-off-by: Jorge Marques <[email protected]>
PR Description
allow users to build using
make -jto parallelize target builds.As stated in the first commit message on my system, this allows to build libraries about 5 times faster.
This was tested on branch
hdl_2021_r2, and rebased onmasterfor the PR.Unfortunately I can't test on v2023.1 at the moment, but since this doesn't touch the HDL or tcl scripts it should be alright.
PR Type
PR Checklist