-
Notifications
You must be signed in to change notification settings - Fork 13
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
Various fixes #95
base: main
Are you sure you want to change the base?
Various fixes #95
Conversation
luhuhis
commented
Jul 12, 2022
- implement RESULT loglevel as described in Add a log level for results #93
- update CheckConf and CheckRand to use the new loglevel
- add cmake error when using GPU_P2P without using GPU_AWARE_MPI (mentioned in Improvements to/small issues with communication #34)
The cmake error should only be thrown at RUNTIME when using multiple nodes. On a single node this is fine. |
# Conflicts: # src/base/IO/logging.h
… for CheckConf and CheckRand. note: iterateOverBulkAllMu needs some changes, I ignored that for now.
added experimental support to compile some executables (namely CheckConf and CheckRand) without a GPU backend by setting -DBACKEND=cpu when calling cmake. note: iterateOverBulkAllMu needs some changes, I ignored that for now. |
There are a lot of things that still need to be changed for full cpu backend support. almost every file needs to be touched for that. In c2364e1 I just want to demonstrate how that can be done |
c.x = real; | ||
c.y = imag; | ||
cREAL = real; | ||
cIMAG = imag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should keep these defines (cREAL and cIMAG). I think from a design perspective I would drop them entirely and instead wrap floatT2 in a more C++ friendly way (e.g. a templated struct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are also "artifacts" of some define replacements I did. I didn't bother to change them back, because there was no difference. For now I could just remove the defines and leave it as c.x and c.y everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upon further inspection, these defines are used in a lot of other files as well. you could argue that it makes the code a bit easier to read from a physics viewpoint
} else | ||
#endif | ||
{ | ||
#if defined(CPUONLY) || defined(USE_CPU_ONLY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we have CPUONLY
and USE_CPU_ONLY
. That might be confusing. Do we need both? What was the difference again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the naming should be changed.
USE_CPU_ONLY comes from the cmake parameter "-DBACKEND=cpu" and means: compile (the whole code) without a GPU backend. The name is like this because for the other cases it also says "USE_*".
CPUONLY is a parameter I introduced for CheckConf and CheckRand. It means: you can compile this executable with a GPU backend, but the executable will run (on cpu only) even if no GPU is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose this:
USE_CPU_ONLY --> BACKEND_CPU
USE_HIP_NVIDIA --> BACKEND_HIP_NVIDIA
USE_HIP_AMD --> BACKEND_HIP_AMD
USE_CUDA --> BACKEND_CUDA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!