-
Notifications
You must be signed in to change notification settings - Fork 787
[NFC][ESIMD] Fix incorrect declarations of simd_view based API #12093
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
Conversation
atomic_update(T *p, simd<Toffset, N> byte_offset, simd_view<T, RegionTy> src0, | ||
simd_mask<N> mask, PropertyListT props = {}) { | ||
atomic_update(T *p, simd<Toffset, N> byte_offset, | ||
simd_view<ViewedTy, RegionTy> src0, simd_mask<N> mask, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently we agreed not to have simd_view
overloads for src0 and src1. They seemed redundant because, of implicit conversion from simd_view to simd.
but we need to keep simd_view
overloads for byte_offset
as the implicit conversion doesn't work for byte_offset
when it is simd_view
- cannot match simd_view to simd error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should I remove it here and in the last atomic_update PR or someone else also looking to this. Asking in order to avoid merge conflicts
typename OffsetRegionTy = region1d_t<Toffset, N, 1>, | ||
typename RegionTy = region1d_t<T, N, 1>, | ||
typename RegionTy = region1d_t<ViewedTy, N, 1>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All such places do not require the default value for template parameter. That default value is never used and is always set by the type of byte_offset
and src
operands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 atomic_update() PR in code-review - 1 from Nick (2 operands atomic_update(acc, ...) and 1 from Evgeniy (atomic_update(local_accessor, ...) - #11924
This clean-up fix should wait for those 2 PRs to be merged.
BTW, if your PR does not really change the functionality, then it should have [NFC]
in the title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see both of these PR are dealing with accessors API and therefore it is logical to deal with the changes at #11683 (it causes merge conflicts there). This one deals primarily with USM and other non accessor based atomic_update API and therefore non related to these 2 PRs
# Conflicts: # sycl/include/sycl/ext/intel/experimental/esimd/memory.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have just 1 comment for naming of Toffset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just 2 comments need minor fix.
@sarnex - This is non-experimental API. Please take a quick look at this fix too, I could miss something.
PropertyListT props = {}) { | ||
block_store<Toffset, N>(acc, vals.read(), props); | ||
block_store<OffsetObjT, N>(acc, vals.read(), props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original code was wrong, the first template parameter is the accessor type and it would never work. I think none of these are getting called. I made an internal tracker to check if that's true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be removed as usinf simd_view to pass values or I misread one of the previous comments ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I mean probably we can remove this entire function because there's no way it was every getting called because the first template argument in the called function was always totally wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
atomic_update(T *p, simd_view<Toffset, OffsetRegionTy> offsets, | ||
simd_view<T, RegionTy> src0, simd_mask<N> mask, | ||
PropertyListT props = {}) { | ||
return atomic_update<Op, T, N>(p, offsets.read(), src0.read(), mask, props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests for every combination right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please explicitly check first what is the effect of calling:
accessor acc;
auto vals_view = vals.select<T, N>();
block_store(acc, vals_view);
- same for USM:
block_store(ptr, vals_view);
Are they compiled successfully?
If existing tests don't have such cases, please add them to LIT tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily blocking PR to get answers to questions and avoid accidental merge.
We have some at memory_properties.cpp. I will add some more |
Added tests that use block_store API and simd_view as an argument |
@@ -63,17 +63,17 @@ kernel7(accessor<int, 1, access::mode::read_write, access::target::device> &buf) | |||
SYCL_ESIMD_FUNCTION { | |||
simd<int, 32> v1(0, 1); | |||
auto vals_view = v1.select<8, 1>(0); | |||
block_store(buf, 0, vals_view); | |||
block_store<int, 8>(buf, 0, vals_view); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If restore the removed simd_view vals
prototypes and have a call block_store(buf, 0, vals_view) without specifying <T, N>
would the test pass or fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I leave the prototypes as they were the tests fail.
This PR fixes incorrect declarations of API that uses simd_view