Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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 [...] |
| 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. |
| 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. |
| 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. |
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.