Skip to content

Add WarpX_UNITY_BUILD - #5702

Merged
EZoni merged 3 commits into
BLAST-WarpX:developmentfrom
ax3l:topic-unity-build
Mar 24, 2025
Merged

Add WarpX_UNITY_BUILD#5702
EZoni merged 3 commits into
BLAST-WarpX:developmentfrom
ax3l:topic-unity-build

Conversation

@ax3l

@ax3l ax3l commented Feb 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Unity builds combine all .cpp files into a single one (a single translation unit, TU). This is useful for performance optimization, computer science experiments, or differentiable programming.

FYI: A CPU unity build of ImpactX is around 18sec on my laptop (BLAST-ImpactX/impactx#861), for WarpX around 3:30min. In both cases, this refers to the lib* of ImpactX/WarpX part.

@ax3l ax3l added the install label Feb 24, 2025
@ax3l
ax3l requested a review from WeiqunZhang February 24, 2025 22:42
@ax3l ax3l mentioned this pull request Feb 24, 2025
Open
@ax3l
ax3l requested review from EZoni, RemiLehe and aeriforme February 24, 2025 23:13
github-advanced-security[bot]
github-advanced-security AI found potential problems Feb 24, 2025
View reviewed changes
Comment thread Source/Diagnostics/WarpXOpenPMD.cpp
if (write_real_comp[idx]) {
// handle scalar and non-scalar records by name
const auto [record_name, component_name] = detail::name2openPMD(real_comp_names[idx]);
const auto [record_name, component_name] = ::detail::name2openPMD(real_comp_names[idx]);

Check notice

Code scanning / CodeQL

Unused local variable

Variable component_name is not used.
Comment thread Source/Diagnostics/WarpXOpenPMD.cpp
if (write_int_comp[idx]) {
// handle scalar and non-scalar records by name
const auto [record_name, component_name] = detail::name2openPMD(int_comp_names[idx]);
const auto [record_name, component_name] = ::detail::name2openPMD(int_comp_names[idx]);

Check notice

Code scanning / CodeQL

Unused local variable

Variable component_name is not used.
@lucafedeli88

lucafedeli88 commented Feb 26, 2025

Copy link
Copy Markdown
Member

Uh! We have to be careful with anonymous namespaces if we use unity builds!

@EZoni

EZoni commented Mar 1, 2025

Copy link
Copy Markdown
Member

@ax3l @lucafedeli88

This comment by Luca,

Uh! We have to be careful with anonymous namespaces if we use unity builds!

sounds like something we need to discuss properly before we proceed with moving more functions to anonymous namespaces. At this time I don't have enough knowledge of unity builds to have an informed understanding of Luca's statement, but if you do, it is probably a good idea to touch base on this and see what is the best way to move forward.

@ax3l

ax3l commented Mar 1, 2025

Copy link
Copy Markdown
Member Author

To clarify: anonymous namespaces are not the problem, identically named global variables repeated with the same name in different .cpp files are what one wants to avoid (in or outside global namespaces).

@lucafedeli88

lucafedeli88 commented Mar 3, 2025

Copy link
Copy Markdown
Member

To clarify: anonymous namespaces are not the problem, identically named global variables repeated with the same name in different .cpp files are what one wants to avoid (in or outside global namespaces).

Yes, sorry, I should have explained that better.... My point is that we are using anonymous namespaces more often than before, so the risk of conflicts may increase in the future. Maybe we could consider not using anonymous namespaces, but parent_namespace::details in the .cpp file to reduce the risk of conflicts. What do you think?

@ax3l

ax3l commented Mar 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

That is possible for sure and clean :)

Anonymous namespaces are generally still great to use, because what they do is they do not create an unnecessary symbol in the final binary and thus keep the binary a bit smaller. (That said, we can use compiler symbol hiding flags when we want to optimize for that, we do that in pybind11 for non-debug builds.)

Another solution I could have done for this one collision here: move the global variable to a header file (in a regular namespace) and define it inline or static.

aeriforme
aeriforme reviewed Mar 5, 2025
View reviewed changes
Comment thread Source/Diagnostics/ReducedDiags/DifferentialLuminosity2D.cpp

@aeriforme aeriforme Mar 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have updated this file in PR #5726.
If you merge development, there would be a few additional synchronizations Gpu::streamSynchronize();, which I guess need to be updated into amrex:: Gpu::streamSynchronize();

@ax3l ax3l Mar 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! That will be automatically merged and does not create a merge conflict, as far as I can see.

@ax3l

ax3l commented Mar 7, 2025

Copy link
Copy Markdown
Member Author

Let me know if you have any further questions, but this is otherwise ready to go. cc @EZoni

EZoni
EZoni reviewed Mar 7, 2025
View reviewed changes

@EZoni EZoni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we add a CI job that builds WarpX with -DWarpX_UNITY_BUILD=ON?

@ax3l

ax3l commented Mar 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Yes, def!

I added one in ImpactX already but was refraining from one just yet here because it still threw a few (mild) warnings. Would do in a follow-up where I fix those warnings, so we can build the unity build also with -Werror.

ax3l added 2 commits March 10, 2025 10:05
@ax3l
Add WarpX_UNITY_BUILD
c1a3cf7 
Unity builds combine all `.cpp` files into a single one
(a single translation unit, TU). This is useful for
performance optimization, computer science experiments,
or differentiable programming.
@ax3l
Fix Unity Build Issues
41f3585 
- Errors: redefinitions (fixed)
- Warnings: shadowing (started)

Ref.:
https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD_UNIQUE_ID.html
@ax3l
ax3l force-pushed the topic-unity-build branch from 2900120 to 41f3585 Compare March 10, 2025 17:06
@EZoni

EZoni commented Mar 10, 2025

Copy link
Copy Markdown
Member

I restarted the cartesian_3d tests since there was a single failure that appeared to be random.

@EZoni

EZoni commented Mar 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I merge development since the latest commit here is a couple of weeks old and then enable auto-merge, so that this work is ready to be picked up again when needed.

@EZoni
EZoni enabled auto-merge (squash) March 24, 2025 11:48
@EZoni
EZoni merged commit 258d331 into BLAST-WarpX:development Mar 24, 2025
@ax3l
ax3l deleted the topic-unity-build branch March 24, 2025 13:47
dpgrote pushed a commit to dpgrote/WarpX that referenced this pull request Apr 15, 2025
@ax3l @EZoni @dpgrote
Add WarpX_UNITY_BUILD (BLAST-WarpX#5702)
977c158 
Unity builds combine all `.cpp` files into a single one (a single
translation unit, TU). This is useful for performance optimization,
computer science experiments, or differentiable programming.

FYI: A CPU unity build of ImpactX is around 18sec on my laptop
(BLAST-ImpactX/impactx#861), for WarpX around
3:30min. In both cases, this refers to the `lib*` of ImpactX/WarpX part.

---------

Co-authored-by: Edoardo Zoni <ezoni@lbl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeriforme aeriforme aeriforme left review comments
@EZoni EZoni EZoni approved these changes
@WeiqunZhang WeiqunZhang Awaiting requested review from WeiqunZhang
@RemiLehe RemiLehe Awaiting requested review from RemiLehe

Assignees

@EZoni EZoni

Labels

install

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ax3l @lucafedeli88 @EZoni @aeriforme @github-advanced-security