-
Notifications
You must be signed in to change notification settings - Fork 736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL][Doc] Release Notes for Jul'24 release #15138
[SYCL][Doc] Release Notes for Jul'24 release #15138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment of posting this comment this PR is still a draft, but I will tag some folks in advance so that they are aware the their review is needed here.
The PR will be moved out of draft state once I clean up UR commits. My assumption is that some of them may be related to customer-reported bugs and therefore they should be mentioned in our release notes. However, it is pretty hard to understand if that's really the case. I will tag @intel/unified-runtime-reviewers here for their feedback, but my plan is to just drop those for now unless someone objects.
Tagging @sarnex to review ESIMD-specific stuff.
Tagging @mdtoguchi to look at SYCL compiler sections which are related to clang driver changes
Tagging @intel/llvm-reviewers-runtime, @aelovikov-intel, @againull, @uditagarwal97 here to review the doc in general and specifically SYCL library sections
Tagging @intel/bindless-images-reviewers to review corresponding items
Tagging @intel/sycl-graphs-reviewers to review corresponding items
Tagging @intel/sycl-matrix-reviewers to review corresponding items
Tagging @intel/llvm-reviewers-cuda to review CUDA & HIP related things
Some general comments/questions from my side:
- I switched from using commit hashes to
owner/project#id
syntax, i.e. direct links to PRs. I think that's easier for both reviewers and curious readers, but let me know if you disagree - Distribution between "New features" and "Improvements" is probably questionable and some items could be re-categorized, so leave a comment if you spot something weird
- There are commits which update both an extension spec and implementation and I mentioned those twice: once in "Documentation" section, another time in "SYCL Library" (or some other) section. Not sure if that was the right approach. If you spot a place which we can simplify, let me know
sycl/ReleaseNotes.md
Outdated
Please note that the implementation is naive and does not expose any special | ||
HW capabilities, it won't provide any performance benefit over how a group | ||
load/store could be done without this extension using simple `for` loop and | ||
group barriers. intel/llvm#13043 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @aelovikov-intel here. As I understand it, the initial implementation has been improved by #13734 and #13673. I have both those PRs listed as separate entry, but I assume that they should be squashed into this single entry. Any suggestions about the combined wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation uses optimized SPIR-V block/write operations for sub-group load/stores if the operation can be directly mapped onto a single block operation. In other cases, it defers to the naive implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the wording and merged the two lines in 4e12678
sycl/ReleaseNotes.md
Outdated
- Fixed a bug where querying a kernel by name from a kernel bundle would crash | ||
a program. intel/llvm#13155 | ||
TODO: ask @cperkinsintel for feedback about the wording here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @cperkinsintel to help write good message here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though TODO
comment was dropped in 79f9295, feedback is still welcome here
sycl/ReleaseNotes.md
Outdated
- Fixed a compilation issue occurring when `printf` is used on CUDA backend | ||
on Windows. intel/llvm#13784 | ||
TODO: was it really a compilation issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @mmoadeli for feedback here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though TODO
comment was dropped in 79f9295, feedback is still welcome here
sycl/ReleaseNotes.md
Outdated
commit https://github.com/intel/llvm/commit/29b4d855fa1a378e89182795e0d368304c40c3f6 | ||
[SYCL][CUDA] Enable support of msvc math functions for nvptx target. (#14007) | ||
|
||
commit https://github.com/intel/llvm/commit/7a9d3b1e9483b69baa0b8c6f1097016efd52854c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Naghasan, I understand that this commit is some kind of performance improvement for passing kernel arguments on CUDA. Is my understanding correct? Do you have any suggestions about how we should word this in release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sycl/ReleaseNotes.md
Outdated
commit https://github.com/intel/llvm/commit/7a9d3b1e9483b69baa0b8c6f1097016efd52854c | ||
[SYCL][NVPTX] Do not decompose SYCL functor unless necessary (#14434) | ||
|
||
commit https://github.com/intel/llvm/commit/38e663ecd37de513d8e31afdfdf245cf8c9d17f0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@againull, I found that this patch helped to resolve some weird issue, but do you have any more meaningful insights into it in context of mentioning it in release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to completely drop this one in 82e5641. Let me know if there are objections to that
sycl/ReleaseNotes.md
Outdated
[SYCL] Declare __devicelib_assert_read only when fallback assert is enabled (#13241) | ||
Is there any particular user-visible bug associated with this? | ||
|
||
commit https://github.com/intel/llvm/commit/4b993a7b32f7743980bce646765a1b427b0996b6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@againull, @mdtoguchi, could you please suggest a message that should be used to describe this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some wording was added in 82e5641, but any feedback is still welcome
sycl/ReleaseNotes.md
Outdated
commit https://github.com/intel/llvm/commit/4b14d706d93891cdb5b0e6a8d4b0b027c1d54ab8 | ||
[SYCL][DeviceSanitizer] Use -asan-constructor-kind=none to disable ctor/dtor (#13259) | ||
was the bug really user-visible? | ||
commit https://github.com/intel/llvm/commit/2442ef047a4e9e9c135beed18a92029e1aad6cad | ||
[DeviceSanitizer] Disable handling no return calls (#14652) | ||
// bugfix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenju-he, @zhaomaosu, could you please help understand if those commits should be mentioned in release notes?
Are they separate bugfixes, or should they be squashed into some other notes we have here for the device sanitizer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed both TODOs in c77b5d3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm besides caps thing, i just did control+f esimd and reviewed those, hopefully that's enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I hope my comments are generally useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Most comments are minor suggestions to typos. I've also left a question I'd like to address regarding the inclusion of highly experimental APIs.
There are still UR commits to handle and some TODOs to resolve, but I'm opening the PR for review to increase its visibility and get more feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYCLcompat notes look good
@intel/dpcpp-doc-reviewers, I've resolved all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small misspellings. LGTM!
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
No description provided.