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

enhance CUDA support in CP2K easyblock (WIP) #2349

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

boegel
Copy link
Member

@boegel boegel commented Feb 26, 2021

(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

if cuda_cc_sm_vals:
gpu_targets = '--gpu-code=%s' % cuda_cc_sm_vals
else:
gpu_targets = ''
Copy link
Member Author

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?

'-ccbin="%s"' % os.getenv('CXX'),
'-Xcompiler="%s"' % os.getenv('CXXFLAGS'),
])
options['GPUVER'] = 'V100'
Copy link
Member Author

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...

Copy link
Contributor

@Flamefire Flamefire Mar 31, 2021

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

Copy link
Member Author

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'
Copy link
Member Author

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?

@boegel boegel modified the milestones: 4.4.0, release after 4.4.0 May 26, 2021
@boegel boegel modified the milestones: 4.4.2, release after 4.4.2 Sep 1, 2021
@boegel boegel modified the milestones: 4.5.1, release after 4.5.1 Dec 7, 2021
@boegel boegel modified the milestones: 4.5.2, release after 4.5.2 Jan 14, 2022
@boegel boegel modified the milestones: 4.5.3, release after 4.5.3 Feb 9, 2022
@boegel boegel removed this from the 4.5.5 milestone Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants