Skip to content
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

Fix clang16 and clang17 builds #905

Merged
merged 18 commits into from
Jul 16, 2024
Merged

Fix clang16 and clang17 builds #905

merged 18 commits into from
Jul 16, 2024

Conversation

valassi
Copy link
Member

@valassi valassi commented Jul 11, 2024

Hi @oliviermattelaer these are small fixes for clang16 builds, can you have a look please? Thanks Andrea

@valassi valassi requested a review from oliviermattelaer July 11, 2024 12:29
@valassi valassi self-assigned this Jul 11, 2024
@valassi valassi linked an issue Jul 11, 2024 that may be closed by this pull request
@valassi
Copy link
Member Author

valassi commented Jul 11, 2024

This closes #904

@valassi
Copy link
Member Author

valassi commented Jul 11, 2024

In the meantime I also included similar fixes, and some wrappers, for clang16

@valassi valassi changed the title Fix clang16 builds Fix clang16 and clang17 builds Jul 11, 2024
valassi added 5 commits July 11, 2024 16:49
…ster for easier merging

git checkout upstream/master $(git ls-tree --name-only HEAD */CODEGEN*txt)
…ph5#904: remove link-time -no-pie, add compiler-time -fPIC to fortran
…5#904: remove link-time -no-pie, add compiler-time -fPIC to fortran
…ODEGEN logs from the latest upstream/master for easier merging

git checkout upstream/master $(git ls-tree --name-only HEAD */CODEGEN*txt)
@valassi
Copy link
Member Author

valassi commented Jul 11, 2024

And I improved the fix, adding -fPIC to fortran compilation instead of -no-pie to clang linking.

One word of motivation:

valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 12, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 12, 2024
…h.h madgraph5#905 and test_misc.cc/valgrind.h madgraph5#906

Add valgrind.h for all processes

for d in $(git ls-tree --name-only HEAD */SubProcesses); do git add $d/valgrind.h $d/*/valgrind.h; done
Copy link
Member

@oliviermattelaer oliviermattelaer left a comment

Choose a reason for hiding this comment

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

Here I'm confused...

I guess I will need to check #908 for the final code... So I put this in hold for the moment.

What confuse me is that you seems to want to fully forbid OpenMP for clang16 and clant17? But will check in #908

@oliviermattelaer
Copy link
Member

Hi Andrea,

Back on this one, given that #900 is merged and that our default is no openmp, would it not make more sense to simply let the code to crash (or refuse to compile) if the user ask for openmp with such compiler?

I guess that you face issue when compiling with such compiler or was it just to simplify some test?

Cheers,

Olivier

@valassi
Copy link
Member Author

valassi commented Jul 16, 2024

Hi @oliviermattelaer thanks!

True OpenMP is now disabled by default, but one can always reenable it.

Here what I wanted to makerk is that we know that we have not ported the openmp stuff to clang, we (well I) did not find a way to make it work. If you prefer I can replace the

else ifneq ($(shell $(CXX) --version | egrep '^clang version 16'),)
    override OMPFLAGS = # disable OpenMP on clang16 #904
else ifneq ($(shell $(CXX) --version | egrep '^clang version 17'),)
    override OMPFLAGS = # disable OpenMP on clang17 #904

by something like

else ifneq ($(shell $(CXX) --version | egrep '^clang version 16'),)
$(error OpenMP is not supported by cudacpp on clang16 (#904))
else ifneq ($(shell $(CXX) --version | egrep '^clang version 17'),)
$(error OpenMP is not supported by cudacpp on clang17 (#904))

Would that be better? This was impossible when OpenMP was the default, but now that it is disabled by default, this can be a way to signal that it is not supported.

In any case I would definitely do something to signal that this was tested and found problematic. There's so many use cases and combinations, that I think it is important to makrk in the code what we are able to do and what we are not able to do.

@oliviermattelaer
Copy link
Member

Yes, I like it :-)
As soon as you have done it, this is good to go.

Thanks,

Olivier

valassi added 4 commits July 16, 2024 13:44
…r if OpenMP builds are attempted on clang16/17 (as discussed with Olivier in madgraph5#905)
…s from the latest upstream/master for easier merging

git checkout upstream/master $(git ls-tree --name-only HEAD */CODEGEN*txt)
@valassi
Copy link
Member Author

valassi commented Jul 16, 2024

Yes, I like it :-)
As soon as you have done it, this is good to go.

Thanks Olivier :-)

Ok I have made the change and regenerated code. When the CI completes I will merge.

@valassi
Copy link
Member Author

valassi commented Jul 16, 2024

The CI is showing
163 successful, 3 in progress, and 3 failing checks
This is expected to end with the usual 6 failures (what is pending is pp_tt012j, which is expected to fail.

Merging now.

@valassi valassi merged commit 034eb18 into madgraph5:master Jul 16, 2024
163 of 169 checks passed
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 16, 2024
…aster with OMP madgraph5#900 and submod madgraph5#897) into gtest

Fix conflicts in epochX/cudacpp/gg_tt.mad/CODEGEN_mad_gg_tt_log.txt
  git checkout clang gg_tt.mad/CODEGEN_mad_gg_tt_log.txt

Note: MG5AMC has been updated including mg5amcnlo#107
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 16, 2024
…aster with clang madgraph5#905, OMP madgraph5#900 and submod madgraph5#897) into gtest

Fix conflicts in epochX/cudacpp/gg_tt.mad/CODEGEN_mad_gg_tt_log.txt
  git checkout clang gg_tt.mad/CODEGEN_mad_gg_tt_log.txt

Note: MG5AMC has been updated including mg5amcnlo#107
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 17, 2024
…ng PR madgraph5#905, constexpr_math.h PR madgraph5#908 and runTest/cudaDeviceReset PR madgraph5#909

Add valgrind.h and its symlink in the repo for gg_tt.mad

The new runTest.cc template now has a (commented out) proof of concept for including two tests (with/without multichannel) madgraph5#896, I will resume from there

After building bldall, the following succeeds
for bck in none sse4 avx2 512y 512z cuda; do echo $bck; ./build.${bck}_d_inl0_hrd0/runTest_*.exe; done

This instead is crashing (again?) for some AVX values
for bck in none sse4 avx2 512y 512z cuda; do echo $bck; valgrind ./build.${bck}_d_inl0_hrd0/runTest_*.exe; done
On closer inspection, this is because valgrind does not support AVX512, so this is ok
@oliviermattelaer oliviermattelaer added the enhancement A feature we want to develop label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature we want to develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix build errors with clang16 (and clang17)
2 participants