This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add memory_scope_all_devices
ClosedPublic

Authored by svenvh on May 27 2021, 4:28 AM.

Details

Summary

Add the memory_scope_all_devices enum value, which is restricted to
OpenCL 3.0 or newer and the __opencl_c_atomic_scope_all_devices
feature. Also guard memory_scope_all_svm_devices accordingly, which
is already available in OpenCL 2.0.

The __opencl_c_atomic_scope_all_devices feature is header-only, so
set its define to 1 in opencl-c-base.h. This is done
unconditionally at the moment, as the mechanism for disabling
header-only options hasn't been decided yet.

This patch only adds a negative test for now. Ideally adding a CL3.0
run line to atomic-ops.cl should suffice as a positive test, but we
cannot do that yet until (at least) generic address spaces and program
scope variables are supported in OpenCL 3.0 mode.

Diff Detail

Event Timeline

svenvh created this revision.May 27 2021, 4:28 AM
svenvh requested review of this revision.May 27 2021, 4:28 AM
azabaznov added inline comments.May 27 2021, 4:41 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

This feature is header only. We had a lot of discussions on that and the main idea was not to declare header only features/extensions in OpenCLExtensions.def and use -D__opencl_c_atomic_scope_all_devices=1 instead, @Anastasia can comment on this.

I personally would like to introduce new flag for OpenCL options in OpenCLExtensions.def which will indicate that feature/extension is header-only, and thus all of such options can be declared in OpenCLExtensions.def: if flag is set to true it can be stripped out from the parser. What do you think about this?

Anastasia added inline comments.May 27 2021, 5:31 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

Yes, I agree the idea is to align with C/C++ directions for scalability i.e. we should only add what is absolutely necessary to the compiler and implement the rest using libraries - just like regular C and C++. We won't be able to scale if we keep adding everything in the compiler. In fact, we already have a huge scalability issue with our builtin functions. If we look at modern C++ - more than 70% of features are not in the compiler at all.

Would it be possible to do something like suggested here: https://reviews.llvm.org/D91531#change-AKXhB4ko4nAO for cl_khr_depth_images?

I personally would like to introduce new flag for OpenCL options in OpenCLExtensions.def which will indicate that feature/extension is header-only, and thus all of such options can be declared in OpenCLExtensions.def: if flag is set to true it can be stripped out from the parser. What do you think about this?

Hmm, I think the macros should either be declared in the headers or using a flag -D. I don't know why would adding them in OpenCLExtensions.def be beneficial if we can use conventional approaches? This allows avoiding the complexity and makes things more modular. If we look at the OpenCL vendor extensions for example - we probably don't want them all in one place?

svenvh updated this revision to Diff 348245.May 27 2021, 6:48 AM
svenvh edited the summary of this revision. (Show Details)

Update change to be header-only.

svenvh added inline comments.May 27 2021, 6:53 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

This feature is header only.

Good catch! I have updated the patch to define the feature macro in the header instead. Currently that definition is not optional, since we don't have finalized the solution for handling this yet (though the __undef proposal seems to be compatible with this change).

I personally would like to introduce new flag for OpenCL options in OpenCLExtensions.def which will indicate that feature/extension is header-only

If we still need to add header-only features to OpenCLExtensions.def, then they aren't really header-only anymore I'd argue (as @Anastasia pointed out above). So I'm not sure we need it either, or perhaps I missed something.

Anastasia added inline comments.May 27 2021, 7:02 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

FYI we have already added extended subgroups extension macros for SPIR in opencl-c-base.h without the __undef<...> trick.

#if defined(__SPIR__)
#define cl_khr_subgroup_extended_types 1
#define cl_khr_subgroup_non_uniform_vote 1
#define cl_khr_subgroup_ballot 1
#define cl_khr_subgroup_non_uniform_arithmetic 1
#define cl_khr_subgroup_shuffle 1
#define cl_khr_subgroup_shuffle_relative 1
#define cl_khr_subgroup_clustered_reduce 1
#endif // defined(__SPIR__)

But extra conditions can be added any time if we get the agreement on the route forward.

clang/lib/Headers/opencl-c-base.h
44

I think we should at least add a defined(__SPIR__) check though? Otherwise it won't be correct for other targets using the same header.

azabaznov added inline comments.May 27 2021, 7:22 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

Hmm, I think the macros should either be declared in the headers or using a flag -D. I don't know why would adding them in OpenCLExtensions.def be beneficial if we can use conventional approaches? This allows avoiding the complexity and makes things more modular. If we look at the OpenCL vendor extensions for example - we probably don't want them all in one place?

Well, IMO separating extensions/features into two classes of options exactly brings new complexities :) I'm not sure why do we need to have a separate interface for them if there already exists unified one. For example, Intel compute-runtime uses -cl-ext flag to forward options : https://github.com/intel/compute-runtime/blob/master/opencl/source/platform/extensions.cpp#L156. 

Can we use this header  mechanism  to define header-only features/extensions while -cl-extinterface is preserved?

svenvh updated this revision to Diff 348309.May 27 2021, 9:34 AM

Restrict feature macro definition to SPIR target only.

Anastasia added inline comments.Jun 1 2021, 5:07 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

I think if we can have a unified interface at no extra cost then sure we should do so. But adding everything into the compiler source code seems like a high cost to me.

Can we use this header mechanism to define header-only features/extensions while -cl-extinterface is preserved?

Perhaps I am missing something but why would using -cl-ext be better than -D? If you use -D you don't need to add anything to clang source code at all so it seems better. Of course, it means that you have to pass an extra flag to the compiler. But it still seems easier than modifying the compiler source code...

Anastasia accepted this revision.EditedJun 7 2021, 1:03 PM

LGTM! Thanks.

Considering that this patch doesn't add anything new to the design, I suggest we go ahead. But we should continue the discussion around -cl-ext etc elsewhere and then perhaps some follow up patches will be needed to fix this fully.

clang/test/Headers/opencl-c-header.cl
154–160

btw I think we are not testing that the macros are not defined for other targets than SPIR, but I guess this deserves a separate patch.

This revision is now accepted and ready to land.Jun 7 2021, 1:03 PM
azabaznov added inline comments.Jun 8 2021, 3:48 AM
clang/include/clang/Basic/OpenCLExtensions.def
113 ↗(On Diff #348222)

Perhaps I am missing something but why would using -cl-ext be better than -D? If you use -D you don't need to add anything to clang source code at all so it seems better. Of course, it means that you have to pass an extra flag to the compiler. But it still seems easier than modifying the compiler source code...

I see the main advantage is having the unified interface for all type of extensions/features: either it affects language semantics or it is a header-only. Also, I don't think it requires a lot of changes in clang: we only need to extend -cl-ext to add unknown extensions/features (which are not in OpenCLExtensions.def) and add a flag that extension/feature is header-only. Alternatively, we could add a new mode when compiling for SPIR target when not support all the features/extensions to not use the -D__undef trick. I think we can proceed with this and have further discussions.

This revision was landed with ongoing or failed builds.Jun 8 2021, 4:01 AM
This revision was automatically updated to reflect the committed changes.