Skip to content

[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

Merged
merged 11 commits into from
Dec 19, 2023

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Dec 6, 2023

This PR fixes incorrect declarations of API that uses simd_view

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

@v-klochkov v-klochkov Dec 6, 2023

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.

Copy link
Contributor Author

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

Comment on lines 4566 to 4567
typename OffsetRegionTy = region1d_t<Toffset, N, 1>,
typename RegionTy = region1d_t<T, N, 1>,
typename RegionTy = region1d_t<ViewedTy, N, 1>,
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

@fineg74 fineg74 changed the title [ESIMD] Fix incorrect declarations of simd_view based API [NFC][ESIMD] Fix incorrect declarations of simd_view based API Dec 7, 2023
Copy link
Contributor

@v-klochkov v-klochkov left a 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

Copy link
Contributor

@v-klochkov v-klochkov left a 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.

@v-klochkov v-klochkov requested a review from sarnex December 12, 2023 18:30
PropertyListT props = {}) {
block_store<Toffset, N>(acc, vals.read(), props);
block_store<OffsetObjT, N>(acc, vals.read(), props);
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

@sarnex sarnex Dec 12, 2023

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

Copy link
Contributor Author

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);
Copy link
Contributor

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?

Copy link
Contributor

@v-klochkov v-klochkov left a 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.

Copy link
Contributor

@v-klochkov v-klochkov left a 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.

@fineg74
Copy link
Contributor Author

fineg74 commented Dec 14, 2023

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.

We have some at memory_properties.cpp. I will add some more

@fineg74
Copy link
Contributor Author

fineg74 commented Dec 14, 2023

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.

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);
Copy link
Contributor

@v-klochkov v-klochkov Dec 15, 2023

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?

Copy link
Contributor Author

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.

@v-klochkov v-klochkov merged commit 331bd5d into intel:sycl Dec 19, 2023
@fineg74 fineg74 deleted the FixViewAPI branch December 19, 2023 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants