[PATCH] D51484: [OpenCL] Add support of cl_intel_device_side_avc_motion_estimation extension

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/AST/OperationKinds.def:325
 // Convert a zero value for OpenCL queue_t initialization.
 CAST_OPERATION(ZeroToOCLQueue)
 
----------------
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. 


================
Comment at: include/clang/AST/Type.h:6379
+inline bool Type::isOCLIntelSubgroupAVCType() const {
+#define EXT_OPAQUE_TYPE(Name, Id, Ext)
+#define INTEL_SUBGROUP_AVC_TYPE(ExtType, Id) \
----------------
I guess this define is not needed here.


================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:68
+    return llvm::PointerType::get( \
+        llvm::StructType::create(Ctx, "opencl." #ExtType), 0);
+#include "clang/Basic/OpenCLExtensionTypes.def"
----------------
I think more generic approach would be to pass AddrSpc and then targets can override getOpenCLTypeAddrSpace putting address space that is needed.


================
Comment at: lib/Headers/opencl-c.h:16196
 
+#pragma OPENCL EXTENSION cl_intel_device_side_avc_motion_estimation : begin
+
----------------
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.


================
Comment at: lib/Headers/opencl-c.h:16320
+
+#define CLK_AVC_IME_PAYLOAD_INITIALIZE_INTEL   { 0 }
+#define CLK_AVC_REF_PAYLOAD_INITIALIZE_INTEL   { 0 }
----------------
Could this just be regular integer literal like the ones above?


================
Comment at: test/SemaOpenCL/intel-subgroup-avc-ext-types.cl:1
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL1.2 -cl-ext=+cl_intel_device_side_avc_motion_estimation -fsyntax-only -verify -DNEGATIVE %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL1.2 -cl-ext=+cl_intel_device_side_avc_motion_estimation -fsyntax-only -verify %s
----------------
Any reasons to separate into 2 clang calls? Could we tests both in one invocation of parsing.


================
Comment at: test/SemaOpenCL/intel-subgroup-avc-ext-types.cl:13
+
+#define CLK_AVC_IME_PAYLOAD_INITIALIZE_INTEL   { 0 }
+#define CLK_AVC_REF_PAYLOAD_INITIALIZE_INTEL   { 0 }
----------------
Just 0 would work too?


================
Comment at: test/SemaOpenCL/intel-subgroup-avc-ext-types.cl:26
+         char4 c4, event_t e, struct st ss) {
+  intel_sub_group_avc_mce_payload_t payload_mce = 0; // No zero initializer for mce types
+  // expected-error at -1 {{initializing 'intel_sub_group_avc_mce_payload_t' with an expression of incompatible type 'int'}}
----------------
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?


================
Comment at: test/SemaOpenCL/intel-subgroup-avc-ext-types.cl:54
+  intel_sub_group_avc_ime_dual_reference_streamin_t dstreamin_list = {0x0, 0x1};
+  // expected-warning at -1 {{excess elements in struct initializer}}
+  intel_sub_group_avc_ime_dual_reference_streamin_t dstreamin_list2 = {};
----------------
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...


================
Comment at: test/SemaOpenCL/intel-subgroup-avc-ext-types.cl:57
+  // expected-error at -1 {{scalar initializer cannot be empty}}
+  intel_sub_group_avc_ime_dual_reference_streamin_t dstreamin_list3 = {c};
+  // expected-error at -1 {{initializing 'intel_sub_group_avc_ime_dual_reference_streamin_t' with an expression of incompatible type 'char'}}
----------------
Can you add something like:

   = {1}


https://reviews.llvm.org/D51484