[libclc] Build for OpenCL 3.0 #135733
[libclc] Build for OpenCL 3.0 #135733
Conversation
This PR is cherry-pick of intel/llvm@cba338e5fb1c This allows adding OpenCL 2.0 built-ins, e.g. ctz, and OpenCL 3.0 extension built-ins, including generic address space variants. llvm-diff shows this PR has no change in amdgcn--amdhsa.bc.
|
@frasercrmck please help to review. thanks. |
|
I don't think we want to unconditionally do this - we probably want a mechanism whereby targets can opt for a specific version. We should definitely add this capability, though. Targets may even want to compile libclc for multiple different versions? That might introduce extra complexity (we'd need "an extra loop", more compilation time, new naming/versioning schemes for the build artifacts). |
libclc/CMakeLists.txt
Outdated
| "+__opencl_c_3d_image_writes," | ||
| "+__opencl_c_images," | ||
| "+cl_khr_3d_image_writes") | ||
| list( APPEND build_flags -cl-std=CL3.0 "-Xclang" ${CL_3_0_EXTENSIONS} ) |
There was a problem hiding this comment.
We would need a more target-configurable way of enabling OpenCL extensions. Not all targets do fp16/fp64, not all do 3D image writes, etc.
There was a problem hiding this comment.
I've reverted OpenCL extensions change in this PR in 4facfec
I find that target should define its supported extension via setSupportedOpenCLOpts API.
You're right. We can't set 3.0 for all targets. I've refine this PR to set the supported OpenCL C version for each target. Default version is 1.2 which is Clang's default OpenCL C version defined at |
wenju-he
changed the title
An application may compile using different -cl-std version, but IIUC such usage is incompatible with a target which supports a specific OpenCL version. So compiling for the target's supported OpenCL version might be enough. |
There was a problem hiding this comment.
Yes I think on reflection it's probably okay to compile for the highest supported OpenCL C version. I believe they're supersets of one another, so a 3.0 builtins library would still contain all of the 1.2 builtins? I might be missing some of the nitty gritty here though.
Another question would be whether or not we should be advertising a 2.X or 3.X library if we don't have all the builtins? Is this a degradation? That might depend on downstream toolchains and if/how they react to versioning info in the LLVM IR.
libclc/CMakeLists.txt
Outdated
| set( opencl_lang_std "CL1.2" ) | ||
|
|
||
| if ( ${DARCH} STREQUAL spirv ) | ||
| set( opencl_lang_std "CL3.0" ) |
There was a problem hiding this comment.
Does this look okay to you, @airlied?
libclc/CMakeLists.txt
Outdated
| elseif( ARCH STREQUAL clspv OR ARCH STREQUAL clspv64 ) | ||
| elseif( ${DARCH} STREQUAL clspv ) | ||
| # Refer to https://github.com/google/clspv for OpenCL version. | ||
| set( opencl_lang_std "CL3.0" ) |
There was a problem hiding this comment.
Does this look okay, @rjodinchr?
libclc/CMakeLists.txt
Outdated
| set( build_flags "-Wno-unknown-assumption" -DCLC_CLSPV ) | ||
| set( opt_flags -O3 ) | ||
| set( MACRO_ARCH CLSPV32 ) | ||
| if( ARCH STREQUAL clspv64 ) | ||
| set( MACRO_ARCH CLSPV64 ) | ||
| endif() | ||
| elseif( ${DARCH} STREQUAL nvptx ) | ||
| # Refer to https://www.khronos.org/opencl/ for OpenCL version in NV implementation. |
There was a problem hiding this comment.
I'm not sure who could verify this, or who even uses libclc built for nvptx.
There was a problem hiding this comment.
updated link to https://developer.nvidia.com/opencl
libclc/CMakeLists.txt
Outdated
| set( MACRO_ARCH ${ARCH} ) | ||
| elseif( ${DARCH} STREQUAL amdgcn OR ${DARCH} STREQUAL amdgcn-amdhsa ) | ||
| # Refer to https://github.com/ROCm/clr/tree/develop/opencl for OpenCL version. | ||
| set( opencl_lang_std "CL2.0" ) |
There was a problem hiding this comment.
I thought we already picked out device compatible default versions in clang?
There was a problem hiding this comment.
I thought we already picked out device compatible default versions in clang?
OpenCL C version is 1.2 for all targets by default in clang if the version isn't specified in command line:
There is TargetInfo api setSupportedOpenCLOpts for target to define its supported extensions and features.
libclc/CMakeLists.txt
Outdated
| # 1.2 is Clang's default OpenCL C language standard to compile for. | ||
| set( opencl_lang_std "CL1.2" ) | ||
|
|
||
| if ( ${DARCH} STREQUAL spirv ) |
There was a problem hiding this comment.
I'd prefer if( DARCH STREQUAL spirv ) which better fits the current style.
Yeah I think libclc should compile for the version that target claims supporting.
3.0 is backward compatible with 1.2,1.1,1.0. |
We can incrementally add missing builtins. If we don't set the version, the newer builtins in libclc are not built at all and effectively dead code. |
There was a problem hiding this comment.
I'd expect the libclc build (or any other runtime support library) to consistently use the same language version independent of the target. If the target doesn't support that version (which IIRC isn't actually a hard error anywhere) and fails to compile on some feature, those should be carved out into separate version dependent build (as in, this isn't a compile target property but a specific build target property)
LGTM. That means we compile for the last OpenCL version, which is 3.0, and enable all OpenCL extensions/features, right? |
Yes. Otherwise you have to still write the code to work with the lowest common denominator. |
Sounds good to me. I hope that by having the internal CLC library we can largely avoid the last scenario, as CLC should be less dependent on the OpenCL C version. An OpenCL 1.2 module could still call a builtin that makes internal use of CLC Anyway, sorry for complicating the PR with my suggestion. I was being unnecessarily conservative. |
Yes, you're right. |
done in commit b3653cd |
wenju-he
changed the title
wenju-he
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/29653 Here is the relevant piece of the build log for the reference |
|
See #136871 for a fix for the CI issues. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/9454 Here is the relevant piece of the build log for the reference |
This PR is modified cherry-pick of intel/llvm@cba338e5fb1c This PR sets OpenCL language version to be the same, which is 3.0, for every target and device, in order to unify the build process. Target should define supported extensions and features via setSupportedOpenCLOpts API. llvm-diff shows one change to amdgcn--amdhsa.bc: * ctz symbols are added since they are now enabled for amdgcn.
This PR is modified cherry-pick of intel/llvm@cba338e5fb1c This PR sets OpenCL language version to be the same, which is 3.0, for every target and device, in order to unify the build process. Target should define supported extensions and features via setSupportedOpenCLOpts API. llvm-diff shows one change to amdgcn--amdhsa.bc: * ctz symbols are added since they are now enabled for amdgcn.
This PR is modified cherry-pick of intel/llvm@cba338e5fb1c This PR sets OpenCL language version to be the same, which is 3.0, for every target and device, in order to unify the build process. Target should define supported extensions and features via setSupportedOpenCLOpts API. llvm-diff shows one change to amdgcn--amdhsa.bc: * ctz symbols are added since they are now enabled for amdgcn.
This PR is modified cherry-pick of intel/llvm@cba338e5fb1c
This PR sets OpenCL language version to be the same, which is 3.0,
for every target and device, in order to unify the build process.
Target should define supported extensions and features via setSupportedOpenCLOpts API.
llvm-diff shows one change to amdgcn--amdhsa.bc: