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

Various fixes #95

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Various fixes #95

wants to merge 16 commits into from

Conversation

luhuhis
Copy link
Collaborator

@luhuhis luhuhis commented Jul 12, 2022

@luhuhis
Copy link
Collaborator Author

luhuhis commented Jul 12, 2022

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.
@luhuhis
Copy link
Collaborator Author

luhuhis commented Aug 23, 2022

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.

@luhuhis
Copy link
Collaborator Author

luhuhis commented Aug 23, 2022

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;
Copy link
Collaborator

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)

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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

src/base/math/grnd.h Outdated Show resolved Hide resolved
} else
#endif
{
#if defined(CPUONLY) || defined(USE_CPU_ONLY)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants