SYCL: support 0-dim acc in handler::copy(accessor, accessor) by v-klochkov · Pull Request #1551 · intel/llvm

v-klochkov · 2020-04-20T10:39:48Z

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

sycl/include/CL/sycl/handler.hpp

s-kanaev · 2020-04-20T17:39:33Z

@v-klochkov Please apply clang-format to the patch. You can find usefule artifacts in deails of clang-format-check job details.

v-klochkov · 2020-04-20T19:50:56Z

@s-kanaev I gave some honor to clang-format and added few additional/recommended changes.
I though want to ignore few places
a) 2 places in handler.hpp: the code without clang-format is much more readable and
b) 2 places in LIT test: I just follow the more compact style of all other similar funcs in LIT test.

v-klochkov · 2020-04-20T19:51:55Z

@romanovvlad Thank you for the quick initial review.
I updated the changes in handler.hpp and also added 3 more cases to LIT test to check atomics.

s-kanaev · 2020-04-21T06:59:46Z

sycl/include/CL/sycl/handler.hpp

Should we check here if DimSrc == DimDst?

I don't know if it is correct to copy from 1D accessor to 3D. It is probably correct. So, that check is not needed.
In my patch I do not change any constraints.
My patch only separates the existing code to this routine to make it possible to copy to/from 0-dim accessor.

s-kanaev · 2020-04-21T07:01:41Z

sycl/include/CL/sycl/handler.hpp

Could this conversion be converted to this?

Suggested change

auto AtomicSrc = (atomic<T, access::address_space::global_space>)Src;

atomic<T, access::address_space::global_space> AtomicSrc = Src;

Ok, I changed it that way.

s-kanaev · 2020-04-21T07:01:57Z

sycl/include/CL/sycl/handler.hpp

Same thing here

s-kanaev · 2020-04-21T07:16:17Z

@s-kanaev I gave some honor to clang-format and added few additional/recommended changes.
I though want to ignore few places
a) 2 places in handler.hpp: the code without clang-format is much more readable and
b) 2 places in LIT test: I just follow the more compact style of all other similar funcs in LIT test.

I believe you have to massage the code to make clang-format happy.

v-klochkov · 2020-04-21T17:24:25Z

@s-kanaev

I believe you have to massage the code to make clang-format happy.

I already applied clang-format per your request, and ignored format's recommendation in only 4 places in sake of better code readability. Did you read my reasoning above?
Sure, clang-format is good in 95%, but let's not turn it into dogma, please.

romanovvlad

LGTM, but I would like @s-kanaev to approve as well.

s-kanaev · 2020-04-21T18:04:54Z

I already applied clang-format per your request, and ignored format's recommendation in only 4 places in sake of better code readability. Did you read my reasoning above?
Sure, clang-format is good in 95%, but let's not turn it into dogma, please.

I understand readability as a pretty good reason. The justification for my argument is that I'm not really sure that "Merge" button will be available with clang-format job failed.

s-kanaev · 2020-04-21T18:14:53Z

sycl/include/CL/sycl/handler.hpp

Shouldn't it be a reference to atomic?

The accessor operator[] and operator() return a value, not a reference.

93 /* Available only when: accessMode == access::mode::atomic && dimensions ==
94 0 /
95 operator atomic<dataT, access::address_space::global_space> () const;
96
97 / Available only when: accessMode == access::mode::atomic && dimensions >
98 0 */
99 atomic<dataT, access::address_space::global_space> operator[](
100 id index) const;

Please clarify if I understood your comment incorrectly.
Did you think about adding & sign right before AtomicDst?
atomic<T, access::address_space::global_space> &AtomicDst = Dst;
If so it would be incorrect code, I think.

romanovvlad · 2020-04-21T18:33:58Z

I already applied clang-format per your request, and ignored format's recommendation in only 4 places in sake of better code readability. Did you read my reasoning above?
Sure, clang-format is good in 95%, but let's not turn it into dogma, please.

I understand readability as a pretty good reason. The justification for my argument is that I'm not really sure that "Merge" button will be available with clang-format job failed.

clang-format-check is not required so we should be able to merge even if it fails

bader · 2020-04-21T19:01:51Z

I already applied clang-format per your request, and ignored format's recommendation in only 4 places in sake of better code readability. Did you read my reasoning above?
Sure, clang-format is good in 95%, but let's not turn it into dogma, please.

I understand readability as a pretty good reason. The justification for my argument is that I'm not really sure that "Merge" button will be available with clang-format job failed.

The reason to make this change is to make consistent formatting across the project, not to make "merge button available".

@v-klochkov, if you think that our project should not accept changes proposed by clang-format, please suggest clang-format configuration file changes. It's much better approach than just break the guidelines followed by the community.

s-kanaev · 2020-04-21T19:30:22Z

The reason to make this change is to make consistent formatting across the project, not to make "merge button available".

I understand it pretty much. I have only translated mechanics or sequence of actions into plain English.

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

v-klochkov · 2020-04-21T21:46:40Z

I understand readability as a pretty good reason. The justification for my argument is that I'm not really sure that "Merge" button will be available with clang-format job failed.

The reason to make this change is to make consistent formatting across the project, not to make "merge button available".

@v-klochkov, if you think that our project should not accept changes proposed by clang-format, please suggest clang-format configuration file changes. It's much better approach than just break the guidelines followed by the community.

Teaching clang-format to recognize the existing laconic/1-line style/format already used in file,
or teaching it to recognize pattern in arg names does not seem easy doable and/or deserving time.

'clang-format' check did not have "Required" status, thus I thought there was some flexibility here.
The clang-format issue is fixed now.

s-kanaev

The changes LGTM.

Might need some changes in config of clang-format, though.

…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)

Accessors with atomic access mode were supported in handler::copy() by mistake in the patch (intel#1551). Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

Accessors with atomic access mode were supported in handler::copy() by mistake in the patch (#1551). Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

v-klochkov requested a review from a team as a code owner April 20, 2020 10:39

v-klochkov requested review from romanovvlad and s-kanaev April 20, 2020 10:39

romanovvlad suggested changes Apr 20, 2020

View reviewed changes

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved

v-klochkov force-pushed the public_vklochkov_copy_0dim branch from c69dac4 to 8b33aaa Compare April 20, 2020 19:46

v-klochkov requested a review from romanovvlad April 20, 2020 19:47

v-klochkov force-pushed the public_vklochkov_copy_0dim branch from 8b33aaa to 8bfe7eb Compare April 20, 2020 20:01

s-kanaev reviewed Apr 21, 2020

View reviewed changes

v-klochkov force-pushed the public_vklochkov_copy_0dim branch from 8bfe7eb to f82691c Compare April 21, 2020 17:16

v-klochkov requested a review from s-kanaev April 21, 2020 17:25

romanovvlad previously approved these changes Apr 21, 2020

View reviewed changes

s-kanaev reviewed Apr 21, 2020

View reviewed changes

v-klochkov dismissed romanovvlad’s stale review via 6faf521 April 21, 2020 21:22

v-klochkov force-pushed the public_vklochkov_copy_0dim branch from f82691c to 6faf521 Compare April 21, 2020 21:22

SYCL: support 0-dim acc in handler::copy(accessor, accessor)

bc90862

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

v-klochkov force-pushed the public_vklochkov_copy_0dim branch from 6faf521 to bc90862 Compare April 21, 2020 21:36

s-kanaev approved these changes Apr 22, 2020

View reviewed changes

bader merged commit 5666107 into intel:sycl Apr 22, 2020

v-klochkov deleted the public_vklochkov_copy_0dim branch April 22, 2020 17:01

v-klochkov mentioned this pull request May 2, 2020

[SYCL] Support scalar accessor in handler::copy(acc,ptr) and copy(ptr… #1634

Merged

v-klochkov mentioned this pull request Jan 28, 2021

[SYCL] Ensure correct access mode in handler::copy (#3109) #3111

Merged

v-klochkov mentioned this pull request Jan 28, 2021

[SYCL] Remove handler::copy() support of atomic accessors. #3118

Merged

	auto AtomicSrc = (atomic<T, access::address_space::global_space>)Src;
	atomic<T, access::address_space::global_space> AtomicSrc = Src;

Conversation

v-klochkov commented Apr 20, 2020

Uh oh!

Uh oh!

Uh oh!

s-kanaev commented Apr 20, 2020

Uh oh!

v-klochkov commented Apr 20, 2020

Uh oh!

v-klochkov commented Apr 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kanaev 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!

s-kanaev commented Apr 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad commented Apr 21, 2020

Uh oh!

bader commented Apr 21, 2020

Uh oh!

s-kanaev commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-klochkov commented Apr 21, 2020

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

s-kanaev commented Apr 21, 2020 •

edited

Loading