[PATCH] D64418: [Docs][OpenCL] Documentation of C++ for OpenCL mode

Tue Jul 9 10:13:15 PDT 2019

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