Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SYCL] Implement USM vars and placeholder accessors passed to reduction #1657

Merged
merged 6 commits into from
May 18, 2020
Merged

[SYCL] Implement USM vars and placeholder accessors passed to reduction #1657

merged 6 commits into from
May 18, 2020

Conversation

v-klochkov
Copy link
Contributor

In order to support USM vars/pointers passed to reduction
the placeholder accessors to global buffers were used.

Signed-off-by: Vyacheslav N Klochkov vyacheslav.n.klochkov@intel.com

@v-klochkov v-klochkov requested a review from Pennycook May 7, 2020 18:26
@v-klochkov v-klochkov requested a review from a team as a code owner May 7, 2020 18:26
@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p4usm branch from 0e7d16c to e4cbcec Compare May 7, 2020 18:31
@v-klochkov
Copy link
Contributor Author

v-klochkov commented May 8, 2020 •
edited
Loading

The 2nd commit in this PR caused a new fail on the new LIT test reduction_usm.cpp.
I don't see why it fails on GPU for sycl-ubu-x64-pr buildbot. I cannot reproduce that fail locally.

One of changes in the 2nd patch that potentially could break something is the explicit memory allocation/deallocation.
So, I add the 3rd commit with a proactive fix for one of places that looked suspicious to me. Perhaps, it fixes the issue.

@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p4usm branch from 5fb7e42 to 82408aa Compare May 14, 2020 00:43
vladimirlaz
vladimirlaz suggested changes May 14, 2020
View reviewed changes
sycl/test/reduction/reduction_usm.cpp Show resolved Hide resolved
sycl/test/reduction/reduction_usm.cpp Outdated Show resolved Hide resolved
sycl/test/reduction/reduction_placeholder.cpp Show resolved Hide resolved
@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p4usm branch from 5773668 to 92ce8bb Compare May 15, 2020 04:18
@v-klochkov
Copy link
Contributor Author

@vladimirlaz , please see the changes in LIT tests. I removed 'UNSUPPORTED: linux' line because it blocked the test on Linux even for CPU where the test passed.
Also, I did not turn off the test on Linux-GPU because the cause of the fail is old GPU driver used in CI and which is going to be updated very soon.
Thank you.

@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p4usm branch 2 times, most recently from 90089a4 to 4ccfba9 Compare May 15, 2020 20:46
Pennycook
Pennycook previously approved these changes May 15, 2020
View reviewed changes
v-klochkov added 6 commits May 17, 2020 01:16
@v-klochkov
[SYCL] Implement USM vars and placeholder accessors passed to reduction
4176ce7 
In order to support USM vars/pointers passed to reduction
the placeholder accessors to global buffers were used.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov
[SYCL] Fix reduction constructors accepting USM pointer
ff0bdc6 
Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov
[SYCL] do proactive fix for USM implementation
2ce0fe6 
The idea of the fix is to guarantee that the temporary buffer created
for USM memory does not write to that USM memory at the buffer descturtor.

The write to USM buffer happens explicitly using handler::copy(TmpAcc,USMPtr).
Another write to USMPtr inside temporary buffer destructor could/might happen
even after USMPtr is deallocated, which would cause Seg Fault, which is fixed
in this patch.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov
[SYCL] do addifitional fixes per reviewer comments
4d8dc85 
This fix also disables the LIT test reduction_usm.cpp for Linux
until the GPU drivers used by CI is updated to 20.06.15619 or newer.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov
[SYCL] Additional fixes to LIT tests per reviewers' coments
5f8b75e 
Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov
[SYCL][LIT] Check device allocated USM pointer passed to reduction
afd31d5 
Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p4usm branch from 4ccfba9 to afd31d5 Compare May 17, 2020 08:16
@bader bader requested review from romanovvlad and Pennycook May 17, 2020 09:32
@v-klochkov
Copy link
Contributor Author

After John's approval I simply re-based the patch to catch the new HEAD with fixed/updated CI. I did not do additional changes.

@bader
Copy link
Contributor

bader commented May 18, 2020

After John's approval I simply re-based the patch to catch the new HEAD with fixed/updated CI. I did not do additional changes.

Please, use "merge" instead of "rebase".

@bader
Copy link
Contributor

bader commented May 18, 2020

I still see unresolved comments from Vlad and John. What is the plan to resolve them?

@v-klochkov
Copy link
Contributor Author

I still see unresolved comments from Vlad and John. What is the plan to resolve them?

All comments are resolved now.

@bader bader merged commit 94cb022 into intel:sycl May 18, 2020
@v-klochkov v-klochkov deleted the public_vklochkov_reduction_p4usm branch May 18, 2020 17:18
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request May 18, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/user…
8007710 
…_docs

* origin/sycl: (1867 commits)
  [SYCL] Implement USM vars and placeholder accessors passed to reduction (intel#1657)
  [SYCL] Fix post-commit testing (intel#1705)
  [SYCL][NFC] Remove multi_ptr<const void, Space>::operator multi_ptr<const void, Space> (intel#1702)
  [Doc] Readme update. (intel#1701)
  Support function pointers in cast instructions
  Adjust FPGA IVDep translation for embedded loops
  [SYCL] Fix host-to-host copy during copyback (intel#1692)
  [SYCL] Add information about dependencies used in CI
  [SYCL] Add runtime support for fsycl-id-queries-fit-in-int (intel#1685)
  Revert "Revert "[llvm][NFC] Cleanup uses of std::function in Inlining-related APIs""
  StoreInst should store Align, not MaybeAlign
  [clang][slh] Add test for SLH feature checking macro
  [NFC] Deduplicate comment in PromoteMemoryToRegister.cpp
  [WebAssembly] Optimize splats of bitcasted vectors
  [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members
  [AArch64][SVE] Implement AArch64ISD::SETCC_PRED
  [SVE] Restore broken LLVM-C ABI compatability
  IR: Remove extra name mangling from llvm.ptrmask
  [NFC] Whitespace fix inside OptParserEmitter
  [compiler-rt][CMAKE] Only add cmake link flags in standalone build
  ...
@v-klochkov v-klochkov mentioned this pull request May 19, 2020
Merged
Fznamznon pushed a commit to Fznamznon/llvm that referenced this pull request Dec 14, 2022
@MrSidims @Fznamznon
Remove Intel Op codes that are being upstreamed (intel#1657)
c65849b 
Unstreamed in KhronosGroup/SPIRV-Headers#298

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@1d56946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimirlaz vladimirlaz vladimirlaz approved these changes

@Pennycook Pennycook Pennycook approved these changes

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

Assignees

@Pennycook Pennycook

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@v-klochkov @bader @Pennycook @romanovvlad @vladimirlaz