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: Add a new header file with the reduction class definition #1558

Merged
merged 2 commits into from
Apr 23, 2020
Merged

SYCL: Add a new header file with the reduction class definition #1558

merged 2 commits into from
Apr 23, 2020

Conversation

v-klochkov
Copy link
Contributor

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 v-klochkov requested a review from a team as a code owner April 21, 2020 04:09
@v-klochkov v-klochkov requested review from romanovvlad and removed request for sergey-semenov April 21, 2020 04:10
@v-klochkov v-klochkov force-pushed the public_vklochkov_reduction_p1 branch 4 times, most recently from fddb644 to ba664c5 Compare April 21, 2020 05:36
@v-klochkov
Copy link
Contributor Author

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

romanovvlad
romanovvlad requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add tests?

Pennycook
Pennycook requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Contributor Author

v-klochkov commented Apr 22, 2020 •
edited
Loading

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
Copy link
Contributor Author

Could you please add tests?

Ok, I added a very basic test.

@bader
Copy link
Contributor

bader commented Apr 22, 2020

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.

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

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.

@v-klochkov v-klochkov requested a review from romanovvlad April 22, 2020 18:09
@v-klochkov
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 v-klochkov force-pushed the public_vklochkov_reduction_p1 branch from b37264d to ab84fce Compare April 22, 2020 18:23
romanovvlad
romanovvlad previously approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Pennycook
Pennycook previously approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@v-klochkov
Copy link
Contributor Author

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.

@v-klochkov
SYCL: fix errors on Windows caused by adding sycl::intel::detail name…
7a01fdb 
…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 v-klochkov dismissed stale reviews from Pennycook and romanovvlad via 7a01fdb April 23, 2020 05:12
@v-klochkov
Copy link
Contributor Author

v-klochkov commented Apr 23, 2020 •
edited
Loading

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

@v-klochkov v-klochkov requested a review from Pennycook April 23, 2020 05:17
Pennycook
Pennycook approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
AlexeySachkov approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bader bader merged commit 04a360a into intel:sycl Apr 23, 2020
@v-klochkov v-klochkov deleted the public_vklochkov_reduction_p1 branch April 23, 2020 20:50
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 26, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/user…
d120b47 
…_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)
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 29, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/lib_…
8092b9c 
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanovvlad romanovvlad romanovvlad approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@Pennycook Pennycook Pennycook approved these changes

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 @AlexeySachkov @romanovvlad