This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

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

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

49

"support of" -> "support for"

138

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

Inconsistent sentence capitalization between the two bullet points.

173

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

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

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

Maybe this would be better?

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

201

Ditto, I would drop "full" here.

202

compatible with

209

Did you meant to indent this bullet point as well?

215

Missing comma: "functions, including"

220

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

This seems to be already included in previous point.

226

Which "cast" operators?

226

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

229

missing comma: "C, including"

232–233

I would suggest this:

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

237–238

Stab? Stub?

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

240–241

[...] 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

BIFs -> built-in functions

175

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

176

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

179

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

builtin is a more common spelling in Clang docs.

176

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

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

222

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

232–233

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
287

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2019, 6:55 AM