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

Wed Aug 14 06:49:18 PDT 2019

Anastasia marked 12 inline comments as done and 2 inline comments as done.
Anastasia added inline comments.

================
Comment at: docs/LanguageExtensions.rst:327

+.. _languageextensions-builtin-macros:
+
----------------
FYI, this is not part of my change. It was just hard to produce combined diff.

================
Comment at: docs/LanguageExtensions.rst:1541
+- Standard C++ libraries. Currently there is no solution for alternative C++
+  libraries provided. Future release will feature library support.
+
----------------
kpet wrote:
> I would remove this last sentence. Only a very small portion of the libray can be supported anyway.
I think it's valuable especially for metaprogramming. I was being asked about it.

================
Comment at: docs/LanguageExtensions.rst:1553
+extension ISO/IEC JTC1 SC22 WG14 N1021 s3.1. Note that since the address space
+behavior in C++ is not documented formally yet, Clang extends existing concept
+from C and OpenCL. For example conversion rules are extended from qualification
----------------
kpet wrote:
> kpet wrote:
> > I would remove the "yet", unless we can refer to an on-going and visible effort.
> "Clang extends existing concept" feels like it should be re-worded.
I need to understand what needs re-wording

================
Comment at: docs/LanguageExtensions.rst:1557
+Embedded C (ISO/IEC JTC1 SC22 WG14 N1021 s3.1.3). For OpenCL it means that
+implicit conversions are allowed from named to ``__generic`` but not vice versa
+(OpenCL C v2.0 s6.5.5) except for ``__constant`` address space. Most of the
----------------
kpet wrote:
> Why use `__generic` when `generic` is equally valid and easier on the eyes? The same comment applies to other mentions of address space keywords.
I prefer to use `__` for two reasons (1) it's more official in OpenCL docs and tutorials (2) I sometimes omit the noun and just use it as a short term in the text.

================
Comment at: docs/LanguageExtensions.rst:1564
+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
----------------
kpet wrote:
> Isn't a conversion explicit if a cast operator is used?
It's complicated to explain but basically some cast operators perform implicit conversions of what they are not expected to do because implicit conversions are allowed everywhere even if the operator isn't intended to do the conversion explicitly.

Similar example in C++, most of operators accept converting to `const` objects.

================
Comment at: docs/LanguageExtensions.rst:1565
+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.
----------------
kpet wrote:
> kpet wrote:
> > At the time of writing `addrspace_cast` doesn't seem to exist in trunk and C-style casts seem to work.
> Didn't you mean "from `__generic` to named address spaces"?
> At the time of writing addrspace_cast doesn't seem to exist in trunk and C-style casts seem to work.

I think it's ok to document full extension even if it's WIP.

================
Comment at: docs/LanguageExtensions.rst:1577
+- non-pointer/non-reference class members except for static data members that are
+  deduced to ``__global`` address space.
+- non-pointer/non-reference alias declarations.
----------------
kpet wrote:
> Makes me wonder whether const members couldn't be in the `constant` address space.
No `constant` specify memory region but `const` indicates that object is not changed during lifetime. You can cast away `const` but you can't convert `constant` object to any other address space.

================
Comment at: docs/LanguageExtensions.rst:1761
+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
----------------
kpet wrote:
> OpenCL doesn't have an API for this per-se, regardless of the version. The only mechanism currently specified is the SPIR-V `Initializer` and `Finalizer` execution modes for entrypoints, either of which currently require an OpenCL 2.2 implementation.
Not sure whether you imply any action here.

================
Comment at: docs/LanguageExtensions.rst:1779
+
+Global destructors can not be invoked in OpenCL v2.0 drivers. However, all
+memory used for program scope objects is released on ``clReleaseProgram``.
----------------
kpet wrote:
> This could be managed by the application with a scheme similar to the one you're proposing for constructor functions.
The Itanium C++ ABIs for destructors are different and we haven't got it working yet for OpenCL. I would leave the description as is for now.

================
Comment at: docs/UsersManual.rst:2771
+
+There are only a few restrictions on allowed C++ features. For detailed information
+please refer to documentation on Extensions (:doc:`LanguageExtensions`).
----------------
kpet wrote:
> Everything is relative. I would keep this 100% factual: "there are restrictions". This is not a sales pitch :).
I think it's fair to say few restrictions because the majority of C++ just works in OpenCL.

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

https://reviews.llvm.org/D64418