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

[SYCL][ROCm] Fix building libclc for AMD #4199

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Conversation

npmiller
Copy link
Contributor

This file uses cl_mem_fence_flags, CLK_GLOBAL_MEM_FENCE,
CLK_LOCAL_MEM_FENCE, which are defined by clc.h.

It wasn't included because when this file was written the OpenCL headers
were implicitely included, this has changed so now including the header
is necessary. See the discussion on #4133 for more details.

This file uses `cl_mem_fence_flags`, `CLK_GLOBAL_MEM_FENCE`,
`CLK_LOCAL_MEM_FENCE`, which are defined by `clc.h`.

It wasn't included because when this file was written the OpenCL headers
were implicitely included, this has changed so now including the header
is necessary. See the discussion on intel#4133 for more details.
@npmiller npmiller requested a review from bader as a code owner July 27, 2021 17:09
@bader
Copy link
Contributor

bader commented Jul 27, 2021

This file uses cl_mem_fence_flags, CLK_GLOBAL_MEM_FENCE,
CLK_LOCAL_MEM_FENCE, which are defined by clc.h.

These must be defined by the OpenCL compiler (see an example in clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl), which builds barrier.cl.
I'm not sure I understand the purpose of clc.h to say which way to define these symbols is the right one, but by default I would say we should just rely on OpenCL compiler built-in capabilities.

Maybe the right way to include the header is to add -finclude-default-header instead of this include?
@Naghasan, do you have any thoughts on that?

@Naghasan
Copy link
Contributor

These must be defined by the OpenCL compiler (see an example in clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl), which builds barrier.cl.
I'm not sure I understand the purpose of clc.h to say which way to define these symbols is the right one, but by default I would say we should just rely on OpenCL compiler built-in capabilities.

libclc tries to be compatible with older clang versions, so might not have access to those definitions or may held unparsable attributes. Instead they provide their own definition to unify things. Also as it is defined by the OpenCL standard, so it is not really a moving target. It is annoying from our perspective, but that's how it is.

libclc recently prevent this inclusion by adding -fno-include-default-header and got included in the sycl branch with this pull #4133 (comment).

I think this patch is inline with the general design of libclc.

@bader
Copy link
Contributor

bader commented Jul 28, 2021

@Naghasan, thank you for clarification.

@bader bader merged commit 7981e60 into intel:sycl Jul 28, 2021
zahiraam pushed a commit to zahiraam/llvm-1 that referenced this pull request Aug 2, 2021
This file uses `cl_mem_fence_flags`, `CLK_GLOBAL_MEM_FENCE`,
`CLK_LOCAL_MEM_FENCE`, which are defined by `clc.h`.

It wasn't included because when this file was written the OpenCL headers
were implicitely included, this has changed so now including the header
is necessary. See the discussion on intel#4133 for more details.
@bader bader added libclc libclc project related issues hip Issues related to execution on HIP backend. labels Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hip Issues related to execution on HIP backend. libclc libclc project related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants