Skip to content

[SYCL] Do additional mostly NFC changes for reduction patch(1585)#1602

Merged
v-klochkov merged 2 commits intointel:syclfrom
v-klochkov:public_vklochkov_reduction_p2fix
Apr 30, 2020
Merged

[SYCL] Do additional mostly NFC changes for reduction patch(1585)#1602
v-klochkov merged 2 commits intointel:syclfrom
v-klochkov:public_vklochkov_reduction_p2fix

Conversation

@v-klochkov
Copy link
Contributor

@v-klochkov v-klochkov commented Apr 29, 2020

  • 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.

@v-klochkov
[SYCL][NFC] Move the reduction command group funcs out of handler.hpp
47db644 
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 <vyacheslav.n.klochkov@intel.com>
@v-klochkov v-klochkov requested a review from romanovvlad April 29, 2020 00:23
@v-klochkov v-klochkov requested a review from a team as a code owner April 29, 2020 00:23
@v-klochkov
Copy link
Contributor Author

v-klochkov commented Apr 29, 2020 •
edited
Loading

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.

@v-klochkov
[SYCL] Additional mostly NFC changes for reduction patch(1585)
f241d2b 
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 <vyacheslav.n.klochkov@intel.com>
@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

v-klochkov commented Apr 29, 2020

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

romanovvlad commented Apr 29, 2020

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
Merge remote-tracking branch 'origin/sycl' into private/abatashe/user…
b952a76 
…_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

Reviewers

1 more reviewer

@romanovvlad romanovvlad romanovvlad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@romanovvlad romanovvlad

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@v-klochkov @romanovvlad