[OpenCL] Add support of cl_intel_device_side_avc_motion_estimation extension

[AlexeySachkov]

Summary

Documentation can be found at https://www.khronos.org/registry/OpenCL/extensions/intel/cl_intel_device_side_avc_motion_estimation.txt

Patch by Kristina Bessonova

[Anastasia]

include/clang/Basic/OpenCLExtensionTypes.def

2

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

28

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

16204

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]

include/clang/Basic/OpenCLExtensionTypes.def

2

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

28

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

[Anastasia]

include/clang/Basic/OpenCLExtensionTypes.def

28

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]

include/clang/Basic/OpenCLExtensionTypes.def

28

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]

include/clang/Basic/OpenCLExtensionTypes.def

28

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]

include/clang/Basic/OpenCLExtensionTypes.def

28

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";

[AlexeySachkov]

Updated patch with new functionality and tests

[Anastasia]

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

6474

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

16203

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.

16327

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

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

2

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

14

Just 0 would work too?

27

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?

55

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...

58

Can you add something like:

= {1}

[AlexeySachkov]

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

[AlexeySachkov]

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

14

Yes, it would

27

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

[Anastasia]

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

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

27

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?

35

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]

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]

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

27

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

35

Simplified test a little bit.

[AlexeySotkin]

Ping

[Anastasia]

lib/Headers/opencl-c.h

16204

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

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

17

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.

27

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.

[AlexeySachkov]

Applied comments. Rebased to ToT

[AlexeySachkov]

lib/Headers/opencl-c.h

16204

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

[Anastasia]

LGTM! Thanks!

test/SemaOpenCL/intel-subgroup-avc-ext-types.cl

6

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.

[AlexeySotkin]

lib/Sema/SemaInit.cpp

8073

isOCLIntelSubgroupAVCType) -> isOCLIntelSubgroupAVCType())

[AlexeySachkov]

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

[AlexeySachkov]

Rebased to ToT.

[asavonic]

I reverted this change in r346338, as it breaks Index/opencl-types.cl LIT test. See http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/21503/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Aopencl-types.cl

[AlexeySachkov]

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

[asavonic]

Committed in r346392.