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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits