[PATCH] D115640: [OpenCL] Add support of __opencl_c_device_enqueue feature macro.

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.


================
Comment at: clang/test/Misc/opencl-c-3.0.incorrect_options.cl:21
+
 // CHECK-FP64: error: options cl_khr_fp64 and __opencl_c_fp64 are set to different values
 
----------------
Anastasia wrote:
> Anastasia wrote:
> > I can't remember if we have discussed this already, but could we use `-verify` for these errors?
> We should be able to remove `FileCheck` and replace `CHECK` directives with something like:
> `//expected-error@*{{options cl_khr_fp64 and __opencl_c_fp64 are set to different values}}`
I don't think we can test it like this because there is different output for each invalid combination, so we need to check them with label.


================
Comment at: clang/test/SemaOpenCL/invalid-device-enqueue-types-cl3.0.cl:5
+void f() {
+  clk_event_t e;
+  queue_t q;
----------------
Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > I know that many test have prefix "invalid" but I feel we have failed to establish the meaning for it because most of the tests in Sema are testing some sort of invalid behavior. But also here I feel that we should test that `enqueue_kernel` is not supported?
> > > 
> > > Do you think we chould merge this testing together with `SemaOpenCL/cl20-device-side-enqueue.cl` with some filename renaming?
> > > 
> > > Technically we should do the same testing even for CL1.x versions...
> > I think it can. But `enqueu_kernel` is a `LangBuiltin` and still not supported yet. Support for `LangBuiltins`  is going to be added in a separate patch: with this patch all of the features that affect language built-ins are supported.
> Ok, then would it still work if we merge this functionality `SemaOpenCL/cl20-device-side-enqueue.cl` and the rest can be added later on...
It's difficult to refactor that test, since it relies on the fact that device enqueue feature is supported and checks for incorrect constructs. We can't enable it for 3.0 now since language built-ins  (`enqueue_kernel` etc.) are not supported yet.


================
Comment at: clang/test/SemaOpenCL/storageclass.cl:2
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL3.0 -cl-ext=-__opencl_c_program_scope_global_variables,-__opencl_c_generic_address_space,-__opencl_c_pipes
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL3.0 -cl-ext=+__opencl_c_program_scope_global_variables,-__opencl_c_generic_address_space,-__opencl_c_pipes
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL3.0 -cl-ext=-__opencl_c_program_scope_global_variables,+__opencl_c_generic_address_space
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL3.0 -cl-ext=-__opencl_c_program_scope_global_variables,-__opencl_c_generic_address_space,-__opencl_c_pipes,-__opencl_c_device_enqueue
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL3.0 -cl-ext=+__opencl_c_program_scope_global_variables,-__opencl_c_generic_address_space,-__opencl_c_pipes,-__opencl_c_device_enqueue
----------------
Anastasia wrote:
> Anastasia wrote:
> > These lines are getting a bit longer. Do we actually need to set `-__opencl_c_device_enqueue` for this test? Same for some other tests...
> ping
Yeah, we need that here because we are turning off generic AS and PSV in this test. Note that `__opencl_c_device_enqueue` is turned off with `-cl-ext=-all`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115640/new/

https://reviews.llvm.org/D115640