[PATCH] D33945: [OpenCL] Add support for missing sub_group functions.

[Alexey Bader via Phabricator via cfe-commits]

bader requested changes to this revision.
bader added a comment.
This revision now requires changes to proceed.

Please, split this patch into two parts:

1. Improve diagnostics on extension enabling.
2. Add missing `sub_group_*` built-in functions.



================
Comment at: Sema/SemaChecking.cpp:1091-1092
   case Builtin::BIwork_group_reserve_write_pipe:
+    if (SemaBuiltinReserveRWPipe(*this, TheCall, false))
+      return ExprError();
+    // Since return type of reserve_read/write_pipe built-in function is
----------------
I suggest leaving `SemaBuiltinReserveRWPipe` as is (i.e. two parameter) and modify the check for `sub_group_*` functions only:
```
if (checkOpenCLSubgroupExt(S, TheCall) || SemaBuiltinReserveRWPipe(*this, TheCall))
  return ExprError();
```

It think it's more readable than looking at SemaBuiltinReserveRWPipe declaration or leaving the comment like this:
```
if (SemaBuiltinReserveRWPipe(*this, TheCall, false /*isSubgroup*/))
```

Please, apply the same approach to `SemaBuiltinCommitRWPipe`.

In addition to that, it makes sense to set the `TheCall` type inside the `SemaBuiltinReserveRWPipe` to avoid code duplication.


================
Comment at: SemaOpenCL/cl20-device-side-enqueue.cl:219
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+}
----------------
Please, add a test case(s) on invalid block parameters to cover the checks you added for new `sub_group_` built-ins..


================
Comment at: SemaOpenCL/sub-group-bifs.cl:1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -cl-std=CL2.0
+
----------------
I don't think it makes sense to add this test. This test look like a duplicate of test cases added to SemaOpenCL/cl20-device-side-enqueue.cl.



================
Comment at: clang/Basic/Builtins.def:1402-1403
 LANGBUILTIN(get_kernel_preferred_work_group_size_multiple, "i.", "tn", OCLC20_LANG)
+LANGBUILTIN(get_kernel_max_sub_group_size_for_ndrange, "i.", "tn", OCLC20_LANG)
+LANGBUILTIN(get_kernel_sub_group_count_for_ndrange, "i.", "tn", OCLC20_LANG)
 
----------------
This built-in functions should return unsigned integers: "i." -> "Ui.".
Please, fix `get_kernel_work_group_size` and `get_kernel_preferred_work_group_size_multiple` too.


================
Comment at: clang/Basic/DiagnosticSemaKinds.td:8423-8424
   "illegal call to enqueue_kernel, incorrect argument types">;
 def err_opencl_enqueue_kernel_expected_type : Error<
-  "illegal call to enqueue_kernel, expected %0 argument type">;
+  "illegal call to %0, expected %1 argument type">;
 def err_opencl_enqueue_kernel_local_size_args : Error<
----------------
Since, this message is not specific to enqueue_kernel anymore, I suggest either rename it or better re-use existing diagnostics if possible. I think there are already message to report argument mismatch + probably additional note diagnostics that hints correct argument type.


https://reviews.llvm.org/D33945