SYCL: Add a new header file with the reduction class definition by v-klochkov · Pull Request #1558 · intel/llvm

v-klochkov · 2020-04-21T04:09:52Z

The patch only adds the new header file, it does not implement
the reduction functionality in parallel_for() yet.

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

v-klochkov · 2020-04-21T05:36:44Z

Sorry, it took several attempts for me to make 'clang-format' happy.

romanovvlad

Could you please add tests?

Pennycook

Since the reducer and reduction classes are Intel extensions, they need to be declared in cl::sycl::intel.

The files should be moved to CL/sycl/intel/ as well.

v-klochkov · 2020-04-22T04:09:30Z

Since the reducer and reduction classes are Intel extensions, they need to be declared in cl::sycl::intel.

The files should be moved to CL/sycl/intel/ as well.

I moved the file to sycl/intel folder; put the 'reduction' function into namespace intel.
In my opinion, the aux classes 'reduction_impl' and 'reducer' should stay in 'detail' namespace as they are internals that are not supposed to be given to user, right?

@bader , Alexey what is your opinion?

v-klochkov · 2020-04-22T04:10:38Z

Could you please add tests?

Ok, I added a very basic test.

bader · 2020-04-22T12:10:31Z

Since the reducer and reduction classes are Intel extensions, they need to be declared in cl::sycl::intel.
The files should be moved to CL/sycl/intel/ as well.

I moved the file to sycl/intel folder; put the 'reduction' function into namespace intel.
In my opinion, the aux classes 'reduction_impl' and 'reducer' should stay in 'detail' namespace as they are internals that are not supposed to be given to user, right?

@bader , Alexey what is your opinion?

You can use sycl::intel::detail namespace for the symbols required for intel extensions by not supposed to be exposed to users.

sycl/test/reduction/reduction_ctor.cpp

v-klochkov · 2020-04-22T18:07:11Z

You can use sycl::intel::detail namespace for the symbols required for intel extensions by not supposed to be exposed to users.

Ok, I put aux reduction.hpp classes to cl::sycl::intel::detail namespace.
Please re-review.

The patch only adds the new header file, it does not implement the reduction functionality in parallel_for() yet. Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

romanovvlad

LGTM, but I would like that @Pennycook approve as well.

Pennycook

LGTM. Thanks for all your work on this, @v-klochkov!

v-klochkov · 2020-04-22T20:14:43Z

Thank you for the approvals.
I see some LIT tests (that seem unrelated to my changes) fail on Windows (buildbot/sycl-win-x64-pr).
Still need to check/confirm that.

…space Inside sycl::intel namespce for the expressions like detail::something, compiler looks for something in sycl::intel::detail instead of sycl::detail. Expressions detail::something were changed to sycl::detail::something Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

v-klochkov · 2020-04-23T05:17:39Z

The new cl::sycl::intel::detail namespace was added in the 1st commit.
That caused problems to compiler on Windows. For expressions like detail::something (used inside cl::sycl::intel) it looked for "something" only in cl::sycl::intel::detail and did not look for it in cl::sycl::detail.
The fix changes "detail::something" to "sycl::detail::something".

Pennycook

I'm a little surprised that this last change was necessary, but I think it makes sense. Previously there was only one detail:: namespace in this project, and now there are two.

We should consider whether the other Intel extensions should move implementation details to sycl::intel::detail as well, but I don't think you need to address that here.

AlexeySachkov

As second code-owner of sub-groups headers, I don't have objections against modifications performed in this patch

…_docs * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536)

…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)

v-klochkov requested a review from a team as a code owner April 21, 2020 04:09

v-klochkov requested review from Pennycook and sergey-semenov April 21, 2020 04:09

v-klochkov assigned Pennycook Apr 21, 2020

v-klochkov requested review from romanovvlad and removed request for sergey-semenov April 21, 2020 04:10

v-klochkov force-pushed the public_vklochkov_reduction_p1 branch 4 times, most recently from fddb644 to ba664c5 Compare April 21, 2020 05:36

romanovvlad suggested changes Apr 21, 2020

View reviewed changes

Pennycook requested changes Apr 21, 2020

View reviewed changes

v-klochkov force-pushed the public_vklochkov_reduction_p1 branch 2 times, most recently from af752ab to 47f34cd Compare April 22, 2020 04:04

v-klochkov requested review from Pennycook and romanovvlad April 22, 2020 04:10

romanovvlad suggested changes Apr 22, 2020

View reviewed changes

sycl/test/reduction/reduction_ctor.cpp Outdated Show resolved Hide resolved

v-klochkov force-pushed the public_vklochkov_reduction_p1 branch from 47f34cd to b37264d Compare April 22, 2020 18:05

v-klochkov requested a review from romanovvlad April 22, 2020 18:09

SYCL: Add the header file with the reduction class definition

ab84fce

The patch only adds the new header file, it does not implement the reduction functionality in parallel_for() yet. Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

v-klochkov force-pushed the public_vklochkov_reduction_p1 branch from b37264d to ab84fce Compare April 22, 2020 18:23

romanovvlad previously approved these changes Apr 22, 2020

View reviewed changes

Pennycook previously approved these changes Apr 22, 2020

View reviewed changes

v-klochkov dismissed stale reviews from Pennycook and romanovvlad via 7a01fdb April 23, 2020 05:12

v-klochkov requested a review from AlexeySachkov as a code owner April 23, 2020 05:12

v-klochkov requested a review from Pennycook April 23, 2020 05:17

Pennycook approved these changes Apr 23, 2020

View reviewed changes

AlexeySachkov approved these changes Apr 23, 2020

View reviewed changes

romanovvlad approved these changes Apr 23, 2020

View reviewed changes

bader merged commit 04a360a into intel:sycl Apr 23, 2020

v-klochkov deleted the public_vklochkov_reduction_p1 branch April 23, 2020 20:50

Comments

Conversation

v-klochkov commented Apr 21, 2020

Uh oh!

v-klochkov commented Apr 21, 2020

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-klochkov commented Apr 22, 2020

Uh oh!

bader commented Apr 22, 2020

Uh oh!

Uh oh!

v-klochkov commented Apr 22, 2020

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov commented Apr 22, 2020

Uh oh!

v-klochkov commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

v-klochkov commented Apr 22, 2020 •

edited

Loading

v-klochkov commented Apr 23, 2020 •

edited

Loading