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.

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] Do additional mostly NFC changes for reduction patch(1585) #1602

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

v-klochkov
Copy link
Contributor

  • Moved 2 big functions implementing command group functions
    for reductions from handler.hpp to reduction.hpp
  • Removed handler::dissociateWithHandler()
  • Removed handler::addEventToQueue() and made queue_impl::addEvent() private again;
  • Minor changes in comments.
  • Replaced 'auto' with 'size_t' in couple places.

This patch only moves he functions has been moved from handler.hpp
to reduction.hpp. No any other changes done.

Signed-off-by: Vyacheslav N Klochkov <[email protected]>
@v-klochkov
Copy link
Contributor Author

v-klochkov commented Apr 29, 2020

Vlad, please review this patch.
It was done for your comments in #1585

This patch did not yet include changes for your comment: #1585 (comment)

The main idea of that comment is to make the code look more graceful, in particular in finalize() method, if I understood it right.

Making the handler for which parallel_for(nd_range, reduction, func) called) empty and aggregative will also require fixing some other places that now require that each command-group-function must have at least one action/task (like single_task/parallel_for/copy). The alternative to that is to change the MCGType for 'this' handler from detail::CG::KERNEL to some new type.
Taking that into account makes that approach less graceful and thus compromises the whole idea.

I think this deserves a separate discussion and a patch sometime a bit later.

Removed handler::dissociateWithHandler()
Removed handler::addEventToQueue() and made queue_impl::addEvent() private again;
Minor changes in comments.
Replaced 'auto' with 'size_t' in couple places.

Signed-off-by: Vyacheslav N Klochkov <[email protected]>
@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p2fix branch from b586eb2 to f241d2b Compare April 29, 2020 00:41
@v-klochkov
Copy link
Contributor Author

Currently, there are 2 patches to simplify the review process.
1st patch simply moves 2 functions from handler.hpp to reduction.hpp
2nd patch addresses other reviewer's comments in #1585

@romanovvlad
Copy link
Contributor

patch did not yet include changes for your com

Vlad, please review this patch.
It was done for your comments in #1585

This patch did not yet include changes for your comment: #1585 (comment)

The main idea of that comment is to make the code look more graceful, in particular in finalize() method, if I understood it right.

Making the handler for which parallel_for(nd_range, reduction, func) called) empty and aggregative will also require fixing some other places that now require that each command-group-function must have at least one action/task (like single_task/parallel_for/copy). The alternative to that is to change the MCGType for 'this' handler from detail::CG::KERNEL to some new type.
Taking that into account makes that approach less graceful and thus compromises the whole idea.

I think this deserves a separate discussion and a patch sometime a bit later.

Yes, my idea was to add a new type for MCGType.

@v-klochkov v-klochkov merged commit abe533c into intel:sycl Apr 30, 2020
@v-klochkov v-klochkov deleted the public_vklochkov_reduction_p2fix branch April 30, 2020 04:34
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request May 6, 2020
…_docs

* origin/sycl: (6482 commits)
  [SYCL][NFC] Clean formatting in Markdown documents (intel#1635)
  [SYCL][Doc] Remove obsolete parens from README (intel#1637)
  [SYCL] Fix failing ABI tests when LLVM_LIBDIR_SUFFIX is set (intel#1605)
  [SYCL] Fix warnings in libdevice (intel#1630)
  [SYCL][CUDA] Triage and clean LIT (intel#1620)
  [SYCL][NFC] Fix GCC 8 compilation warnings (intel#1631)
  [SYCL] Minor fixes in LowerWGScope
  [SYCL] PI: correct default interoperability plugin selection
  [SYCL] Add faster reduction implementations using atomic or/and intel::reduce() (intel#1615)
  [SYCL] Add sycl-ls utility for listing devices discovered/selected by SYCL RT (intel#1575)
  [SYCL] Fix getDeviceFromHandler declarations (intel#1626)
  [SPIR-V] Correct/improve declaration of SPIR-V builtins (intel#1519)
  [SYCL][USM] Improve USM allocator test and fix improper behavior. (intel#1538)
  [SYCL] Fix failing ABI LITs (intel#1622)
  [SYCL] Add support for MSVC internal math functions in device library (intel#1441)
  [SYCL] Add runtime library versioning (intel#1604)
  [SYCL] Check weak symbols in ABI dumps (intel#1609)
  [NFC][SYCL] Improve kernel metadata test (intel#1610)
  Revert "[SYCL] XFAIL LIT test due to duplicate diagnostic" (intel#1460)
  [SYCL] Move the reduction command group funcs out of handler.hpp (intel#1602)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants