[PATCH] D66294: [Docs][OpenCL] Release 9.0 notes for OpenCL

Thu Aug 15 08:43:31 PDT 2019

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

The overall structure seems alright. I'll let people more familiar with the code-base judge whether other important features should be added there as well.

================
Comment at: docs/ReleaseNotes.rst:49

+- Full experimental support of :ref:`C++ for OpenCL <openclcpp>` has
+  been added.
----------------
"Full experimental" feels like an oxymoron. I would drop the "full".

================
Comment at: docs/ReleaseNotes.rst:49

+- Full experimental support of :ref:`C++ for OpenCL <openclcpp>` has
+  been added.
----------------
mantognini wrote:
> "Full experimental" feels like an oxymoron. I would drop the "full".
"support of" -> "support for"

================
Comment at: docs/ReleaseNotes.rst:138

+- Support of address space attribute in various C++ features was improved, refer
+  to :ref:`C++ for OpenCL <openclcpp>` for more details). The following features
----------------
Support //for// address space attribute//s// [...] improved //(refer// [...]

Alternatively, if you don't want to use parenthesis, drop the closing parenthesis on the next line.

================
Comment at: docs/ReleaseNotes.rst:142
+
+  (1) address spaces as method qualifiers are not accepted yet;
+
----------------
Inconsistent sentence capitalization between the two bullet points.

================
Comment at: docs/ReleaseNotes.rst:173
+
+- Added initial support for implicitly including OpenCL BIFs using
+  efficient trie lookup generated by TableGen. A corresponding
----------------
If the BIF acronym wasn't introduced before, it should be replaced with "builtin functions". It seems we don't have more file context in this review so I cannot tell.

================
Comment at: docs/ReleaseNotes.rst:175
+  efficient trie lookup generated by TableGen. A corresponding
+  frontend only flag ``-fadd-opencl-builtins`` has been added to
+  enable trie during parsing.
----------------
I'm not 100% sure about the grammar rule in English, but shouldn't there be a "-" between "frontend" and "only" here to make it an adjective-ish?

================
Comment at: docs/ReleaseNotes.rst:183
+
+- Simplified representation of blocks including their generation in
+  IR i.e. indirect calls to block function has been changed to
----------------
Simplified //the// representation of blocks//**,**// including their generation //into// IR. //Furthermore,// indirect calls [...]

(I'm assuming here that the indirect calls to block function is not the only improvement. And even if it is, "i.e." is less impressive, isn't it?)

================
Comment at: docs/ReleaseNotes.rst:194
+
+- Improved math builtin function of long long for x86.
+
----------------
Maybe this would be better?

Improved math builtin functions with parameters of type `long long` for x86.

================
Comment at: docs/ReleaseNotes.rst:201
+
+Full experimental support for C++17 features in OpenCL has been
+added and backwards compatibility to OpenCL C v2.0 was enabled.
----------------
Ditto, I would drop "full" here.

================
Comment at: docs/ReleaseNotes.rst:202
+Full experimental support for C++17 features in OpenCL has been
+added and backwards compatibility to OpenCL C v2.0 was enabled.
+The documentation has been added for supported language features
----------------
compatible //with//

================
Comment at: docs/ReleaseNotes.rst:209
+
+- Templates parameters and arguments
+
----------------
Did you meant to indent this bullet point as well?

================
Comment at: docs/ReleaseNotes.rst:215
+
+  - Objects and member functions including special member
+    functions;
----------------
Missing comma: "functions, including"

================
Comment at: docs/ReleaseNotes.rst:220
+
+  - Method qualifiers are allowed with address spaces;
+
----------------
Maybe something along these line would be better?

Methods can be overloaded for different address spaces.

Or, if you want to emphasis the qualifiers,

Method qualifiers now include address space.

================
Comment at: docs/ReleaseNotes.rst:222
+
+  - Address space deduction has been extended for C++ use cases;
+
----------------
This seems to be already included in previous point.

================
Comment at: docs/ReleaseNotes.rst:226
+
+  - Cast operators are now preventing to converting address
+    spaces. They can still be cast using C style cast.
----------------
Which "cast" operators?

================
Comment at: docs/ReleaseNotes.rst:226
+
+  - Cast operators are now preventing to converting address
+    spaces. They can still be cast using C style cast.
----------------
mantognini wrote:
> Which "cast" operators?
[...] are now prevent//ed from// converting [...]

================
Comment at: docs/ReleaseNotes.rst:229
+
+- Vector types as in OpenCL C including compound vector
+  initialization.
----------------
missing comma: "C, including"

================
Comment at: docs/ReleaseNotes.rst:232-233
+
+- OpenCL specific type: images, samplers, events, pipes, except
+  for blocks.
+
----------------
I would suggest this:

OpenCL-specific types, except within blocks: [...]

================
Comment at: docs/ReleaseNotes.rst:237-238
+
+- Global constructor stab is made an executable kernel to allow
+  invoking it from the host side.
+
----------------
Stab? Stub?

Global constructors can be invoked from the host side using a specific, compiler-generated kernel.

================
Comment at: docs/ReleaseNotes.rst:240-241
+
+- Overloads with generic address space are added to all atomics
+  including the ones from prior to OpenCL v2.0.

----------------
[...] to all //atomic builtins//(?)**//,//** including the ones prior to [...]

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

https://reviews.llvm.org/D66294