Skip to content

Conversation

@lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented May 1, 2024

I've opened a PR in UR oneapi-src/unified-runtime#1569 which has been merged, that fixes an issue where USM allocation functions are returning non-null pointer values when called with alignment values that are not powers of 2 on L0 devices. In this PR, I've re-enabled gpu testing on an E2E test that was trying to validate this behavior.

@lbushi25 lbushi25 marked this pull request as ready for review May 1, 2024 19:04
@lbushi25 lbushi25 requested a review from a team as a code owner May 1, 2024 19:04
@lbushi25 lbushi25 requested a review from a team as a code owner May 1, 2024 19:34
@lbushi25 lbushi25 requested a review from uditagarwal97 May 1, 2024 19:34
@lbushi25 lbushi25 temporarily deployed to WindowsCILock May 1, 2024 20:27 — with GitHub Actions Inactive
@lbushi25 lbushi25 temporarily deployed to WindowsCILock May 1, 2024 21:38 — with GitHub Actions Inactive
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

//==----- alloc_with_invalid_alignment.cpp - SYCL USM allocation test-----==//
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is required to have a license boiler plate in tests, that's the case even with the upstream LLVM.


#include <sycl/sycl.hpp>

// Purpose of this test is to verify that when SYCL is backed by L0, the aligned
Copy link
Contributor

Choose a reason for hiding this comment

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

We could simplify the wording. Something like:
"This test verifies that for L0 backend, aligned USM alloc functions returns null_ptr, when called with alignment values that are not a power-of-2. "

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why just for L0 backend? What happens for opencl backend? If this test is supposed to run on a specific backend, it should have a "REQUIRES: opencl" or "REQUIRES:level_zero" statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a requires level_zero line in the first line of this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aahh.. I missed that. 👍🏻

@lbushi25 lbushi25 temporarily deployed to WindowsCILock May 2, 2024 21:15 — with GitHub Actions Inactive
@lbushi25
Copy link
Contributor Author

lbushi25 commented May 28, 2024

Reminder to not merge this until the UR changes go through so that I can update the tag.

@kbenzie kbenzie dismissed their stale review May 29, 2024 10:52

UR tag update required

@lbushi25 lbushi25 changed the title [SYCL][L0] Enable gpu testing for aligned allocation functions after L0 bug fix [SYCL] Enable gpu testing for aligned allocation functions after L0 bug fix May 29, 2024
@lbushi25 lbushi25 temporarily deployed to WindowsCILock May 29, 2024 17:20 — with GitHub Actions Inactive
@lbushi25 lbushi25 temporarily deployed to WindowsCILock May 29, 2024 18:38 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Nov 26, 2024
@github-actions
Copy link
Contributor

This pull request was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants