mantognini added a comment. Here are a few comments from me but keep in mind that English is not my primary language
================ Comment at: docs/LanguageExtensions.rst:1562-1565 +C style cast will follow OpenCL C v2.0 rules (s6.5.5). All cast operators will +permit implicit conversion to generic. However converting from named address +spaces to generic can only be done using ``addrspace_cast``. Note that +conversions between ``__constant`` and any other is still disallowed. ---------------- I would suggest using `__generic` or //generic address space//. The later is a bit more verbose, but makes the sentence more natural I think. ================ Comment at: docs/LanguageExtensions.rst:1582 + void foo() { + T m; // address space of m will be knowns at template instantiation time. + T * ptr; // ptr points to generic address space object. ---------------- s/knowns/known/ ================ Comment at: docs/LanguageExtensions.rst:1611-1615 +By default references will refer to generic address space objects (except for +dependent types that are not template specializations +(:ref:`<opencl_cpp_addrsp_deduction>`). When references are bound to values address +space compatibility check will be performed. The logic will largely follow the rules +from address space pointer conversion (OpenCL v2.0 s6.5.5). ---------------- The `ref` is broken. There's also one too many `(`. ================ Comment at: docs/LanguageExtensions.rst:1613-1614 +dependent types that are not template specializations +(:ref:`<opencl_cpp_addrsp_deduction>`). When references are bound to values address +space compatibility check will be performed. The logic will largely follow the rules +from address space pointer conversion (OpenCL v2.0 s6.5.5). ---------------- > When references are bound to values address space compatibility check will be > performed. This might just be me so this might actually be okay, but I feel the sentence is simpler when rewritten as "Address space compatibility checks are performed when references are bound to values." What do you think? > The logic will largely follow the rules from address space pointer conversion Present tense should probably be better. Are there actually any differences? If no, I would drop "largely" from the sentence. Otherwise it might be good to actually list the differences. ================ Comment at: docs/LanguageExtensions.rst:1617 + +**Default AS** + ---------------- AS might be better fully spelled out. ================ Comment at: docs/LanguageExtensions.rst:1619 + +All non-static methods take implicit object parameter that is a pointer type. By +default this pointer parameter is in generic address space. All concrete objects ---------------- take //an// implicit ================ Comment at: docs/LanguageExtensions.rst:1624 +address space won't be compiled unless address space is explicitly specified using +address space method qualifiers ((:ref:`<opencl_cpp_addrspace_method_qual>`) as the +conversion between ``__constant`` and generic is disallowed. Method qualifiers can ---------------- `ref` is broken and there's one too manu `(`. ================ Comment at: docs/LanguageExtensions.rst:1627 +also be useful in case conversion to generic address space is undesirable (even if +it's legal). For example if we need to take advantage of memory bank accesses. +Please note this not only applies to regular methods but to constructors and ---------------- > For example if we need to take advantage of memory bank accesses. Maybe "For example: to take advantage [...]"? ================ Comment at: docs/LanguageExtensions.rst:1662 + + class C{ + // Has the following implicit definition ---------------- nitpicking: missing space before `{`. ================ Comment at: docs/LanguageExtensions.rst:1671 + +**Built in operators** + ---------------- Typo: //Builtin// ================ Comment at: docs/LanguageExtensions.rst:1673 + +All builtin operators will be available in the specific address spaces, thus no conversion +to generic is performed. ---------------- "specific" seems to imply to the user will have to explicitly write down the AS. Maybe "requested" or "desired" would fit better? ================ Comment at: docs/LanguageExtensions.rst:1679 +There is no deduction of address spaces in non-pointer/non-reference template parameters and +dependent types (:ref:`<opencl_cpp_addrsp_deduction>`). The address space of template +parameter is deduced during the type deduction if it's not explicitly provided in ---------------- The reference seems broken as well. ================ Comment at: docs/LanguageExtensions.rst:1708 + void bar() { + foo3<__global int>(); // error: conflicting address space qualifiers are provided + } ---------------- s/foo3/foo/ ================ Comment at: docs/LanguageExtensions.rst:1717 + void foo(){ + T var; + } ---------------- nitpicking: indentation. Might as well rename it to `ii` to match the other example. ================ Comment at: docs/LanguageExtensions.rst:1753-1761 +Global objects are constructed before the first kernel using the global +objects is executed and destroyed just after the last kernel using the +program objects is executed. In OpenCL v2.0 drivers there is no specific +API for invoking global constructors. However, an easy workaround would be +to enqueue constructor initialization kernel that has a name +``@_GLOBAL__sub_I_<compled file name>``. This kernel is only present if there +are any global objects to be initialized in the compiled binary. One way to ---------------- Is it expected of the user to call this special kernel, or is it expected that the driver takes care of that? ================ Comment at: docs/LanguageExtensions.rst:1768 +.. code-block:: console + clang -cl-std=c++ test.cl + ---------------- As it stands, I'm not sure why this command is mentioned in this paragraph. Maybe it's a leftover of things that are now in the User's Manual? ================ Comment at: docs/LanguageExtensions.rst:1770 + +If there are any global objects to be initialized the end binary will contain +``@_GLOBAL__sub_I_test.cl`` kernel to be enqueued. ---------------- s/end/final/ ================ Comment at: docs/UsersManual.rst:2401 +Clang currently supports OpenCL C language standards up to v2.0. Starting from Clang9 +C++ mode is available for OpenCL (see :ref:`<opencl_cpp>`). ---------------- The reference is broken. ================ Comment at: docs/UsersManual.rst:2771-2772 + +There are only a few restrictions on allowed C++ features, for detailed information +please refer to documentation on Extensions (:doc:`LanguageExtensions`). + ---------------- I would make to sentences instead of using a `,`. ================ Comment at: docs/UsersManual.rst:2797 + + clang -cl-std=c++ test.cl + ---------------- I would remove this code sample; it is already explained above that `-cl-std=c++` should be used. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64418/new/ https://reviews.llvm.org/D64418 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits