-
Notifications
You must be signed in to change notification settings - Fork 283
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
enhance CUDA support in CP2K easyblock (WIP) #2349
base: develop
Are you sure you want to change the base?
Conversation
easybuild/easyblocks/c/cp2k.py
Outdated
if cuda_cc_sm_vals: | ||
gpu_targets = '--gpu-code=%s' % cuda_cc_sm_vals | ||
else: | ||
gpu_targets = '' |
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.
This needs a better default, and/or a warning if no CUDA compute capabilities were specified?
easybuild/easyblocks/c/cp2k.py
Outdated
'-ccbin="%s"' % os.getenv('CXX'), | ||
'-Xcompiler="%s"' % os.getenv('CXXFLAGS'), | ||
]) | ||
options['GPUVER'] = 'V100' |
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.
This is obviously hardcoded, should we auto-detect what the correct value should be for this?
Not sure what the significance of this is, actually...
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.
Is this the point where you said in the call that only a single GPU arch is supported?
I just searched the source of CP2K and didn't find any place where this is actually used: https://github.com/cp2k/cp2k/search?q=GPUVER
All uses are either in the autotools build chain, which we don't use (I'd guess we could?) So I guess we don't need it at all. Or did I miss anything?
I'd say only the NVFLAGS are required
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.
No, this is really required, I tried without it. It's used by the DBCSR component.
I have an updated version of this, which I apparently haven't pushed yet, doing that right now (see 68e15c5)
else: | ||
gpu_targets = '' | ||
|
||
options['DFLAGS'] += ' -D__ACC -D__DBCSR_ACC -D__PW_CUDA -D__GRID_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.
Do we need easyconfig parameters to opt-out of using -D__PW_CUDA
and -D__GRID_CUDA
?
…s + don't set GPUVER
… support + derive value for GPUVER setting and allow speciyfing it via custom 'gpuver' easyconfig parameter
(replacement for #1703)
I'm not sure I'm entirely happy with these changes, there's quite a bit of hardcoding going on, this probably deserves one or more additional custom easyconfig parameters?
edit: example easyconfig relying on these changes: https://gist.github.com/boegel/69c00c280a1af0614e0ecc03e4adb1af