Skip to content

[SYCL][ESIMD] Fix an error when a scalar offset is provided as a parameter to the API #8075

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
Merged
merged 10 commits into from
Jan 31, 2023
Merged

[SYCL][ESIMD] Fix an error when a scalar offset is provided as a parameter to the API #8075

merged 10 commits into from
Jan 31, 2023

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Jan 21, 2023

No description provided.

@fineg74 fineg74 requested a review from a team as a code owner January 21, 2023 02:31
@fineg74 fineg74 mentioned this pull request Jan 21, 2023
Merged
@fineg74
Copy link
Contributor Author

fineg74 commented Jan 21, 2023

Complementary test PR: intel/llvm-test-suite#1534

@fineg74 fineg74 temporarily deployed to aws January 21, 2023 02:56 — with GitHub Actions Inactive
@fineg74
Copy link
Contributor Author

fineg74 commented Jan 21, 2023

/verify with intel/llvm-test-suite#1534

@fineg74
Copy link
Contributor Author

fineg74 commented Jan 21, 2023

Emulator failure at dword_atomic_smoke.cpp is fixed in the test PR

@fineg74 fineg74 temporarily deployed to aws January 21, 2023 03:26 — with GitHub Actions Inactive
@fineg74
Copy link
Contributor Author

fineg74 commented Jan 21, 2023

/verify with intel/llvm-test-suite#1534

@v-klochkov
Copy link
Contributor

Looks good to me. The only question that stops me from approving it is this one:

If I understand it correctly, this PR is motivated by some regression,
i.e. we had a test that worked correctly some time ago and eventually started giving compilation error.
How was it possible to have that test working (compiling, running and passing) without these additional variation-functions being added here, and what happened that introduced that regression?

@fineg74
Copy link
Contributor Author

fineg74 commented Jan 26, 2023

Looks good to me. The only question that stops me from approving it is this one:

If I understand it correctly, this PR is motivated by some regression, i.e. we had a test that worked correctly some time ago and eventually started giving compilation error. How was it possible to have that test working (compiling, running and passing) without these additional variation-functions being added here, and what happened that introduced that regression?

The test that failed is a benchmark test that seems not to run regularly and that is why the issue wasn't discovered earlier.
The root cause is the change to introduce templated offset type that broke implicit conversions that used to work before i.e. conversions from simd_view to simd (fixed in original PR) or conversions from a scalar to simd vector (this PR). I believe it is pretty rare corner case to use scalar instead of vector for offsets and I am not sure if this was intended use in the failing test, but to preserve the compatibility with old behavior this PR was introduced.

v-klochkov
v-klochkov reviewed Jan 26, 2023
View reviewed changes
v-klochkov
v-klochkov previously approved these changes Jan 27, 2023
View reviewed changes
@v-klochkov v-klochkov dismissed their stale review January 27, 2023 16:21

Temporarily cancel my approval. I want to ensure that multiple/vector stores to same offset and atomic vector updates are safe from GPU/hardware point of view and is not undefined behavior

v-klochkov
v-klochkov reviewed Jan 27, 2023
View reviewed changes
@fineg74
Merge remote-tracking branch 'origin/sycl' into scalarOffset
f01cc09 
# Conflicts:
#	sycl/include/sycl/ext/intel/esimd/memory.hpp
@fineg74
Merge branch 'scalarOffset' of https://github.com/fineg74/llvm into s…
93c0417 
…calarOffset

# Conflicts:
#	sycl/include/sycl/ext/intel/esimd/memory.hpp
@fineg74
x clang-format issue
c2c223e
@fineg74 fineg74 temporarily deployed to aws January 27, 2023 21:40 — with GitHub Actions Inactive
@fineg74 fineg74 temporarily deployed to aws January 27, 2023 22:11 — with GitHub Actions Inactive
@fineg74 fineg74 temporarily deployed to aws January 27, 2023 22:47 — with GitHub Actions Inactive
@fineg74
Copy link
Contributor Author

fineg74 commented Jan 28, 2023

Test failure OptionalKernelFeatures/is_compatible.cpp is not related to the change

@fineg74 fineg74 temporarily deployed to aws January 31, 2023 05:53 — with GitHub Actions Inactive
@fineg74 fineg74 temporarily deployed to aws January 31, 2023 06:25 — with GitHub Actions Inactive
@fineg74 fineg74 temporarily deployed to aws January 31, 2023 18:59 — with GitHub Actions Inactive
@fineg74 fineg74 temporarily deployed to aws January 31, 2023 20:46 — with GitHub Actions Inactive
@v-klochkov v-klochkov merged commit 035ef2b into intel:sycl Jan 31, 2023
@fineg74 fineg74 deleted the scalarOffset branch February 1, 2023 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v-klochkov v-klochkov v-klochkov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fineg74 @v-klochkov @vmustya