Add WarpX_UNITY_BUILD
#5702
Add WarpX_UNITY_BUILD
#5702
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -846,13 +846,13 @@ | |||
| for (auto idx=0; idx<pc->NumRealComps(); idx++) { | |||
| 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 Note
| @@ -869,13 +869,13 @@ | |||
| for (auto idx=0; idx<int_counter; idx++) { | |||
| 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 Note
|
Uh! We have to be careful with anonymous namespaces if we use unity builds! |
|
This comment by Luca,
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. |
|
To clarify: anonymous namespaces are not the problem, identically named global variables repeated with the same name in different |
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 |
|
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 |
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
Thanks! That will be automatically merged and does not create a merge conflict, as far as I can see.
|
Let me know if you have any further questions, but this is otherwise ready to go. cc @EZoni |
|
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 |
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.
- Errors: redefinitions (fixed) - Warnings: shadowing (started) Ref.: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD_UNIQUE_ID.html
Unity builds combine all
.cppfiles 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.