Page MenuHomePhabricator

[Docs][OpenCL] Release 9.0 notes for OpenCL
ClosedPublic

Authored by Anastasia on Aug 15 2019, 7:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Anastasia created this revision.Aug 15 2019, 7:31 AM
mantognini requested changes to this revision.Aug 15 2019, 8:43 AM

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.

docs/ReleaseNotes.rst
49 ↗(On Diff #215397)

"Full experimental" feels like an oxymoron. I would drop the "full".

49 ↗(On Diff #215397)

"support of" -> "support for"

138 ↗(On Diff #215397)

Support for address space attributes [...] improved (refer [...]

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

142 ↗(On Diff #215397)

Inconsistent sentence capitalization between the two bullet points.

173 ↗(On Diff #215397)

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.

175 ↗(On Diff #215397)

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?

183 ↗(On Diff #215397)

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?)

194 ↗(On Diff #215397)

Maybe this would be better?

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

201 ↗(On Diff #215397)

Ditto, I would drop "full" here.

202 ↗(On Diff #215397)

compatible with

209 ↗(On Diff #215397)

Did you meant to indent this bullet point as well?

215 ↗(On Diff #215397)

Missing comma: "functions, including"

220 ↗(On Diff #215397)

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.

222 ↗(On Diff #215397)

This seems to be already included in previous point.

226 ↗(On Diff #215397)

Which "cast" operators?

226 ↗(On Diff #215397)

[...] are now prevented from converting [...]

229 ↗(On Diff #215397)

missing comma: "C, including"

232–233 ↗(On Diff #215397)

I would suggest this:

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

237–238 ↗(On Diff #215397)

Stab? Stub?

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

240–241 ↗(On Diff #215397)

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

This revision now requires changes to proceed.Aug 15 2019, 8:43 AM
svenvh added inline comments.Aug 15 2019, 8:51 AM
docs/ReleaseNotes.rst
173 ↗(On Diff #215397)

BIFs -> built-in functions

175 ↗(On Diff #215397)

The flag is called -fdeclare-opencl-builtins (not -fadd...).

176 ↗(On Diff #215397)

The option does not only "enable a trie" during parsing. I'd suggest to just drop "to enable trie during parsing".

179 ↗(On Diff #215397)

Refactored the opencl-c.h header file ...

TableGen trie -> -fdeclare-opencl-builtins.

Anastasia updated this revision to Diff 215591.Aug 16 2019, 6:57 AM
Anastasia marked 5 inline comments as done.
Anastasia added inline comments.
docs/ReleaseNotes.rst
173 ↗(On Diff #215397)

builtin is a more common spelling in Clang docs.

176 ↗(On Diff #215397)

We don't have to describe all details here btw, but just the main ones. I would like to say what the flag is used for explicitly rather than leaving the room for interpretations.

Btw, would it make sense to document this flag in UserManual? https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options

220 ↗(On Diff #215397)

I prefer the second one, as the first one can be read as parameter address space.

222 ↗(On Diff #215397)

Do you mean method qualifiers? I don't see the connection?

232–233 ↗(On Diff #215397)

I mean with the exception of blocks here.

Anastasia updated this revision to Diff 215598.Aug 16 2019, 7:27 AM
mantognini added inline comments.Aug 19 2019, 3:41 AM
docs/ReleaseNotes.rst
209 ↗(On Diff #215598)

I think Sphinx/RST wants an empty line here.

Nitpicking for consistency, could you have a ; at then end of each bullet point (except the last one, obviously)? Thanks.

Anastasia updated this revision to Diff 215894.Aug 19 2019, 7:34 AM
  • Added empty line and ; as requested on the review.
mantognini accepted this revision.Aug 19 2019, 9:16 AM

Thanks for addressing my comments.

This revision is now accepted and ready to land.Aug 19 2019, 9:16 AM
hans accepted this revision.Aug 20 2019, 2:16 AM

lgtm, and many thanks for writing release notes!

Go ahead and commit to the branch directly with SVN or let me know if you'd like me to do it.

Closed by commit rL369758: [Docs][OpenCL] Release 9.0 notes for OpenCL (authored by stulova, committed by ). · Explain WhyAug 23 2019, 6:55 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2019, 6:55 AM