-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add support for building Clang with OpenMP offload support #2229
Add support for building Clang with OpenMP offload support #2229
Conversation
In the current setup, code with OpenMP will compile, but will encounter a strange error at runtime. This update adds support for defining the OpenMP CUDA capabilities which remedies the runtime error. This change builds on the existing CUDA support already defined in the build process.
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.
LGTM, thanks!
@@ -85,6 +85,8 @@ def extra_options(): | |||
'skip_all_tests': [False, "Skip running of tests", CUSTOM], | |||
# The sanitizer tests often fail on HPC systems due to the 'weird' environment. | |||
'skip_sanitizer_tests': [True, "Do not run the sanitizer tests", CUSTOM], | |||
'default_cuda_capability': [None, "Default CUDA capability specified for clang, e.g. '7.5'", CUSTOM], | |||
'cuda_compute_capabilities': [[], "List of CUDA compute capabilities to build with", CUSTOM], |
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 already a general easyconfig parameter (since easybuilders/easybuild-framework#3382), so adding cuda_compute_capabilities
here too shouldn't be needed...
@nordmoen Do you have a Clang
easyconfig file that requires the enhancement you implemented here?
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.
@boegel The default parameter is quite nice if a user has specified more than one compute compatibility and doesn't want to use the minimum.
I added the cuda_compute_capabilities
based on feedback from @Flamefire on Slack. In this way it resembles other similar easyconfig
files like TensorFlow
. I have used the built-in --cuda-compute-capabilites
flag in EB and for me it should not be needed.
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.
There's two different things here:
-
The
cuda_compute_capabilities
easyconfig parameter, which is a general easyconfig parameter (so you don't need to define it inextra_options
for it to be defined & picked up), see "eb -a | grep cuda
". -
The
--cuda-compute-capabilities
EasyBuild configuration option, which is what you're picking up viabuild_option('cuda_compute_capabilities')
(I know it can be confusing...).
I was merely pointing out that add cuda_compute_capabilities
as a knwon easyconfig parameter is not needed at all, since it's already a known general easyconfig parameter. There's no real downside though (except for adding to the confusion, maybe).
In the current setup, code with OpenMP will compile, but will encounter
a strange error at runtime. This update adds support for defining the
OpenMP CUDA capabilities which remedies the runtime error. This change
builds on the existing CUDA support already defined in the build
process.