This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add support of cl_intel_device_side_avc_motion_estimation extension
ClosedPublic

Authored by AlexeySachkov on Aug 30 2018, 3:58 AM.

Diff Detail

Event Timeline

AlexeySachkov created this revision.Aug 30 2018, 3:58 AM
Anastasia added inline comments.Sep 5 2018, 7:45 AM
include/clang/Basic/OpenCLExtensionTypes.def
1

I am confused about the purpose of this file. Is it supposed to contain intel extension specific types or generally OpenCL types?

27

From the specification of this extension I can't quite see if these types have to be in Clang instead of the header. Can you please elaborate on any example where it wouldn't be possible for this type to be declared in the header using the technique explained in:
https://clang.llvm.org/docs/UsersManual.html#opencl-extensions

lib/Headers/opencl-c.h
16197

Should we be using:

#pragma OPENCL EXTENSION my_ext : begin

then the user can get correct diagnostics that the functions can only be used once the extension is enabled.

AlexeySachkov added inline comments.Sep 6 2018, 1:42 AM
include/clang/Basic/OpenCLExtensionTypes.def
1

It is supposed to contain any opaque types from any OpenCL extension

27

We cannot define these types in header because their layout is not defined in specification, i.e. all of these types are opaque

Anastasia added inline comments.Sep 6 2018, 7:32 AM
include/clang/Basic/OpenCLExtensionTypes.def
27

This is not the reason to add functionality to Clang. You can easily sort such things with target specific headers or even general headers (see ndrange_t for example). Spec doesn't have to describe everything. The question is whether there is something about those types that can't be handled using standard include mechanisms. Usually it's prohibited behaviors that can't be represented with such mechanisms. Like if some operations have to be disallowed or allowed (since in OpenCL C you can't define user defined operators) with the types.

I think the trend is to avoid adding complexity into Clang, unless there is no other way to implement some feature correctly.

AlexeySotkin added inline comments.
include/clang/Basic/OpenCLExtensionTypes.def
27

Major part of these types must support initialization only by zero. intel_sub_group_avc_mce_payload_t and intel_sub_group_avc_mce_result_t must support initialization only via special builtins defined in the spec. Corresponding errors must be reported. I think we can't implement this behavior using standard include mechanism, can we?

Possible value of the additional complexity, except builtin declaration, is that the patch is quite generic. So next time anyone wants to implement an extension with a type restrictions which can't be handled with the include mechanism, all that they needs to do is to modify this single file.

Anastasia added inline comments.Sep 20 2018, 4:56 AM
include/clang/Basic/OpenCLExtensionTypes.def
27

Major part of these types must support initialization only by zero. intel_sub_group_avc_mce_payload_t and intel_sub_group_avc_mce_result_t must support initialization only via special builtins defined in the spec. Corresponding errors must be reported. I think we can't implement this behavior using standard include mechanism, can we?

Are these restrictions not mentioned in the specification document then? Or is it not the final version yet (not sure since it says First Draft). Do you plan to add the diagnostics for the restrictions afterwards? It doesn't have to be in the same patch, but just checking because if not I don't think it would make sense to go this route.

Possible value of the additional complexity, except builtin declaration, is that the patch is quite generic. So next time anyone wants to implement an extension with a type restrictions which can't be handled with the include mechanism, all that they needs to do is to modify this single file.

It seems reasonable to implement this extra mechanism, provided that there are more of similar use cases.

Btw, considering that there are some modifications to core language spec restriction sections in this document, does this extension invalidate or change any core language rules that might affect parsing/diagnostics?

AlexeySachkov added inline comments.Sep 21 2018, 12:50 AM
include/clang/Basic/OpenCLExtensionTypes.def
27

Are these restrictions not mentioned in the specification document then? Or is it not the final version yet (not sure since it says First Draft).

Current version of spec is not very clear regarding all these restrictions. I'm working with authors to get updated version published at Khronos registry.

Do you plan to add the diagnostics for the restrictions afterwards?

Yes, I'm going to update this patch and add some tests for Sema

Btw, considering that there are some modifications to core language spec restriction sections in this document, does this extension invalidate or change any core language rules that might affect parsing/diagnostics?

AFAIK extension describes a new one kind of sampler and initialization rules for it. This new sampler reusing existing sampler_t type, but value of initializer macro doesn't correspond to existing OpenCL spec: next version of patch will hide one warning message:

if (FilterMode != 1 && FilterMode != 2 &&
  !S.getOpenCLOptions().isEnabled(
      "cl_intel_device_side_avc_motion_estimation"))
S.Diag(Kind.getLocation(),
       diag::warn_sampler_initializer_invalid_bits)
       << "Filter Mode";

Updated patch with new functionality and tests

AlexeySachkov marked an inline comment as done.Sep 21 2018, 2:48 AM
Anastasia added inline comments.Sep 24 2018, 5:02 AM
include/clang/AST/OperationKinds.def
325 ↗(On Diff #166423)

I am wondering if we could potentially unify all of those ZeroToOCL* into one cast type... and also do similar for all of the zero init patterns.

include/clang/AST/Type.h
6377

I guess this define is not needed here.

lib/CodeGen/CGOpenCLRuntime.cpp
68

I think more generic approach would be to pass AddrSpc and then targets can override getOpenCLTypeAddrSpace putting address space that is needed.

lib/Headers/opencl-c.h
16196

I think we should guard this by

#ifdef cl_intel_device_side_avc_motion_estimation

so that it's not added for the targets that don't support this.

Also it might be worth adding a check for a function from this block into test/Headers/opencl-c-header.cl to verify that it's unavailable by default.

16320

Could this just be regular integer literal like the ones above?

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
1 ↗(On Diff #166423)

Any reasons to separate into 2 clang calls? Could we tests both in one invocation of parsing.

13 ↗(On Diff #166423)

Just 0 would work too?

26 ↗(On Diff #166423)

Would it make sense to add a check for non-zero constant?

Also can you assign variables of intel_sub_group_avc_mce_payload_t type from the same type? Any other restrictions on assignment (i.e. w integer literals) and operations over these types?

54 ↗(On Diff #166423)

This error message is probably not the best, but at least it's rejected. Some thing like
initializing ... with an expression of incompatible type would have been better. Not asking to do this change though...

57 ↗(On Diff #166423)

Can you add something like:

= {1}

Applied comments from Anastasia. Updated headers to conform with the latest spec

AlexeySachkov marked 7 inline comments as done.Oct 3 2018, 8:13 AM
AlexeySachkov added inline comments.
test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
13 ↗(On Diff #166423)

Yes, it would

26 ↗(On Diff #166423)

Also can you assign variables of intel_sub_group_avc_mce_payload_t type from the same type?

Yes, such assignment is allowed.

Any other restrictions on assignment (i.e. w integer literals)

All of these types can only be initialized using call to a special built-ins or using predefined macros like CLK_AVC_REF_RESULT_INITIALIZE_INTEL. Any other assignment should lead to an error.

I found that I'm able to assign variable of type intel_sub_group_avc_imc_payload_t to variable of type intel_sub_group_avc_mce_payload_t, so I will update the patch when I implement such diagnostic message.

and operations over these types?

Variables of these types can only be used as return values or arguments for built-in functions described in the specification. All other operations are restricted

It would be good to test your CIndex changes in test/Index/opencl-types.cl.

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
26 ↗(On Diff #166423)

W/o the spec change it's really difficult to review properly. So are you testing 2 groups of types:

  1. Init by zero in bar?
  2. Init by builtin function in foo?
34 ↗(On Diff #168115)

I am not sure it makes sense to iterate through all different types. You don't enumerate all of them and we don't do exhaustive testing in Clang tests anyway. I would just check integer literal and one other builtin type.

AlexeySachkov marked an inline comment as done.

Updated tests. Reverted usage of #pragma OPENCL EXTENSION my_ext : begin due to a failed LIT tests: test/Headers/opencl-c-header.cl

Test failed on the following RUN-line:

// Compile for OpenCL 2.0 for the first time. The module should change.                                                             
// RUN: %clang_cc1 -triple spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules -fimplicit-modu le-maps -fmodules-cache-path=%t -fdisable-module-hash -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 --check-prefix=CHECK- MOD %s

Clang crashes at the assertion in ASTReader::getGlobalSubmoduleID():

assert(I != M.SubmoduleRemap.end() && "Invalid index into submodule index remap");
AlexeySachkov added inline comments.Oct 12 2018, 4:21 AM
test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
26 ↗(On Diff #166423)

I would like to test:

  1. foo: init by literal or variable, negative tests
  2. far: init by other type or assignment between different types, negative tests
  3. bar: init by special initializers from spec, positive tests
34 ↗(On Diff #168115)

Simplified test a little bit.

Anastasia added inline comments.Oct 19 2018, 11:55 AM
lib/Headers/opencl-c.h
16197

Would it be better to use begin/end instead of enable/disable?

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
26 ↗(On Diff #166423)

Ok, I see. Considering that the restirctions are not in the spec yet. I feel adding a comment describing initialization/assignment/operation restrictions for different types would be helpful here.

16 ↗(On Diff #169368)

Is there any point to have all these defines if they are all mapped to 0? I think using constant literals are easier for testing.

Applied comments. Rebased to ToT

AlexeySachkov marked 2 inline comments as done.Oct 23 2018, 8:38 AM
AlexeySachkov added inline comments.
lib/Headers/opencl-c.h
16197

Done, but https://reviews.llvm.org/D53200 have to be landed first to avoid failures in LIT tests

Anastasia accepted this revision.Oct 26 2018, 8:15 AM

LGTM! Thanks!

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl
5 ↗(On Diff #170657)

Btw do you plan to implement this later:

All intel_sub_group_avc_* types can only be used as argument or return value of built-in functions defined in the extension.
This revision is now accepted and ready to land.Oct 26 2018, 8:15 AM
AlexeySotkin added inline comments.Nov 2 2018, 7:33 AM
lib/Sema/SemaInit.cpp
8073 ↗(On Diff #170657)

isOCLIntelSubgroupAVCType) -> isOCLIntelSubgroupAVCType())

Updated opencl-c.h header: fixed typos in built-in declarations. Applied comment from Alexey

Rebased to ToT.

This revision was automatically updated to reflect the committed changes.
AlexeySachkov changed the repository for this revision from rL LLVM to rC Clang.
AlexeySachkov removed a subscriber: llvm-commits.

Fixed issue in test/Index/opencl-types.cl

Committed in r346392.