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.

Sign up for GitHub

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

Jump to bottom

cudacpp.mk should include make_opts #787

Closed
oliviermattelaer opened this issue Nov 2, 2023 · 5 comments · Fixed by #780
Closed

cudacpp.mk should include make_opts #787

oliviermattelaer opened this issue Nov 2, 2023 · 5 comments · Fixed by #780
Assignees
@valassi

Comments

@oliviermattelaer
Copy link
Member

The title says it all.
It is fine for the CI/CD, fine on mac but problematic for @valassi. Sorry Andrea, I assign this to you.
@oliviermattelaer oliviermattelaer mentioned this issue Nov 2, 2023
Merged
@valassi
Copy link
Member

valassi commented Nov 3, 2023

Thanks Olivier.

I found the main problem I was having: adding make_opts was breaking the definition of CUDACPP_MAKEFILE. So this was really a bug.

I fixed this as

override CUDACPP_MAKEFILE := $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST))

What really makes the diffrence here is the ':='. Without that, the expression is evaluated when used, so it points corrrectly to cudacpp.mk before including make_opts, but it was then pointing to ../../Source/make_opts after including make_opts.

The override is kind of optional, it just ensures that the variable cannot be modified from the outside. Just a reminder that this can be used (and is heavily used elsewhere) to encapsulate the internal environment of a makefile.

I will do a few more tests, comparing the build commands with and without make_opts.

If there are issues later on, we can always come back iteratively and fix issues as they appear.

@oliviermattelaer can you in any case make one or two specific examples of which variables you want to take from make_opts? Just to understand better (and for instance remove some overrides elsewhere if we need to inherit your settings, or add overrides if we do not want to inherit them).

Also copying @hageboeck for info.

@valassi
Copy link
Member

valassi commented Nov 3, 2023

Ah, and I added a protection, as make_opts does not exist in standalone (gg_tt.sa) builds

ifneq ($(wildcard ../../Source/make_opts),)
include ../../Source/make_opts
endif

valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in CODEGEN cudacpp.mk, only include Source/make_opts if Sou…
d69bd0b 
…rce exists (e.g it is missing in .sa directories) madgraph5#787
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] regenerate ggtt.mad including Olivier's patches - 'make cle…
6f97c08 
…anall' now fails because make_opts is included madgraph5#787

This boils down to

make -f cudacpp.mk cleanall
  OMPFLAGS=
  AVX=512y
  FPTYPE=d
  HELINL=0
  HRDCOD=0
  RNDGEN=hasCurand
  Building in BUILDDIR=. for tag=512y_d_inl0_hrd0_hasCurand (USEBUILDDIR is not set)
  make USEBUILDDIR=0 clean -f ../../Source/make_opts
  make[1]: Entering directory '/data/avalassi/GPU2023/madgraph4gpuX/epochX/cudacpp/gg_tt.mad/SubProcesses/P1_gg_ttx'
  make[1]: *** No rule to make target 'clean'.  Stop.
  make[1]: Leaving directory '/data/avalassi/GPU2023/madgraph4gpuX/epochX/cudacpp/gg_tt.mad/SubProcesses/P1_gg_ttx'
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, add some debug printouts to show ho…
2a911b7 
…w including make_opts causes issues here: the CUDACPP_MAKEFILE variable is corrupted madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='../../Source/make_opts'
  cudacpp.mk:42: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, try to fix the CUDACPP_MAKEFILE iss…
c6904a1 
…ue with override, this is irrelevant! madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='../../Source/make_opts'
  cudacpp.mk:42: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, use ':=' to set CUDACPP_MAKEFILE on…
8a27432 
…ly once at the beginning and avoid corruption when make_opts is included madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.

However, this is still sensitive to variables coming from outside

make -f cudacpp.mk cleanall CUDACPP_MAKEFILE=pippo
  CUDACPP_MAKEFILE='pippo'
  CUDACPP_MAKEFILE='pippo'
  cudacpp.mk:43: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, add override on top of ':=' to ensu…
4d08b2c 
…re that CUDACPP_MAKEFILE is an internal variable that cannot be set externally madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.

This is now insensitive to variables coming from outside

make -f cudacpp.mk cleanall CUDACPP_MAKEFILE=pippo
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, remove debug printouts - 'make clea…
c65bba0 
…nall' now succeeds after including make_opts madgraph5#787

NB: I also checked that 'make cleanall; make' gives exactly the same output in my environment whether make_opts is included or not
@valassi valassi linked a pull request Nov 3, 2023 that will close this issue
Patches and test results over the latest PRs #780
Merged
@oliviermattelaer
Copy link
Member Author

Excellent, perfect andrea, I have made one PR: #788
Where I added a new entry in the run_card to pass information to the cudacpp compiler (in that case how to setup FPtype).

@valassi
Copy link
Member

valassi commented Nov 3, 2023

Thanks Olivier! I checked that, after fixing the cudacpp.mk bug, I now get the same 'make cleanall; make' output with and without make_opts included, so that is good for me. I close this.

@valassi valassi closed this as completed Nov 3, 2023
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in CODEGEN, backport the fixes in cudacpp.mk for make_opts …
fd5277a 
…from ggtt.mad madgraph5#787
@valassi
Copy link
Member

valassi commented Nov 3, 2023

Just one point on Mac and gfortran: GNU make assumes FC=f77 if FC is not defined. So indeed in my Linux environment I explicitly export FC=gfortran. But I think this is the correct way
https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html

valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] regenerate ggtt.mad including Olivier's patches - 'make cle…
75fbc33 
…anall' now fails because make_opts is included madgraph5#787 (*NB OpenMP is now disabled by default!*)

This boils down to

make -f cudacpp.mk cleanall
  OMPFLAGS=
  AVX=512y
  FPTYPE=d
  HELINL=0
  HRDCOD=0
  RNDGEN=hasCurand
  Building in BUILDDIR=. for tag=512y_d_inl0_hrd0_hasCurand (USEBUILDDIR is not set)
  make USEBUILDDIR=0 clean -f ../../Source/make_opts
  make[1]: Entering directory '/data/avalassi/GPU2023/madgraph4gpuX/epochX/cudacpp/gg_tt.mad/SubProcesses/P1_gg_ttx'
  make[1]: *** No rule to make target 'clean'.  Stop.
  make[1]: Leaving directory '/data/avalassi/GPU2023/madgraph4gpuX/epochX/cudacpp/gg_tt.mad/SubProcesses/P1_gg_ttx'
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, add some debug printouts to show ho…
707065e 
…w including make_opts causes issues here: the CUDACPP_MAKEFILE variable is corrupted madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='../../Source/make_opts'
  cudacpp.mk:42: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, try to fix the CUDACPP_MAKEFILE iss…
c92ec8e 
…ue with override, this is irrelevant! madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='../../Source/make_opts'
  cudacpp.mk:42: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, use ':=' to set CUDACPP_MAKEFILE on…
113d332 
…ly once at the beginning and avoid corruption when make_opts is included madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.

However, this is still sensitive to variables coming from outside

make -f cudacpp.mk cleanall CUDACPP_MAKEFILE=pippo
  CUDACPP_MAKEFILE='pippo'
  CUDACPP_MAKEFILE='pippo'
  cudacpp.mk:43: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, add override on top of ':=' to ensu…
784f37c 
…re that CUDACPP_MAKEFILE is an internal variable that cannot be set externally madgraph5#787

make -f cudacpp.mk cleanall
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.

This is now insensitive to variables coming from outside

make -f cudacpp.mk cleanall CUDACPP_MAKEFILE=pippo
  CUDACPP_MAKEFILE='cudacpp.mk'
  CUDACPP_MAKEFILE='cudacpp.mk'
  cudacpp.mk:43: *** exit.  Stop.
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in ggtt.mad cudacpp.mk, remove debug printouts - 'make clea…
418c8b1 
…nall' now succeeds after including make_opts madgraph5#787

NB: I also checked that 'make cleanall; make' gives exactly the same output in my environment whether make_opts is included or not
valassi added a commit to valassi/madgraph4gpu that referenced this issue Nov 3, 2023
@valassi
[oct23av] in CODEGEN, backport the fixes in cudacpp.mk for make_opts …
b13ae49 
…from ggtt.mad madgraph5#787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@valassi valassi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Patches and test results over the latest PRs valassi/madgraph4gpu
2 participants
@valassi @oliviermattelaer