Skip to content
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

[SYCL][UR][L0] Add UR_L0 environment variables #8732

Merged
merged 3 commits into from
Mar 30, 2023
Merged

[SYCL][UR][L0] Add UR_L0 environment variables #8732

merged 3 commits into from
Mar 30, 2023

Conversation

jandres742
Copy link
Contributor

@jandres742 jandres742 commented Mar 22, 2023 •
edited
Loading

Define UR_LO_DEBUG and UR_L0_SERIALIZE, and keep
their ZE_ counterparts for now.

And do some reorganization

@jandres742 jandres742 temporarily deployed to aws March 22, 2023 10:55 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws March 22, 2023 12:57 — with GitHub Actions Inactive
@jandres742 jandres742 marked this pull request as ready for review March 22, 2023 13:43
@jandres742 jandres742 requested a review from a team as a code owner March 22, 2023 13:43
@jandres742
Copy link
Contributor Author

@smaslov-intel : please review.

bmyates
bmyates reviewed Mar 22, 2023
View reviewed changes
sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_common.cpp Outdated
int Res = setenv(name, value, 1);
#endif
if (Res != 0) {
zePrint(
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we need a way to determine if this code is being executed as part of L0 PI or L0 UR adapter. One way to do it would be to add a compile define into cmake that is only set when compiling the adapater library, check it in this file and route calls accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmyates: we could, but that would be short-termed since idea is that there's no PI+L0 anymore but PI+URL0, if I'm not mistaken. This is how the prints look like now with this change:

$ UR_L0_DEBUG=1 SYCL_PI_TRACE=2 <app>
...
---> piKernelRelease(
        <unknown> : 0x2a78f10
ZE ---> zeKernelDestroy(Kernel->ZeKernel)
UR ---> urProgramRelease(KernelProgram)
) --->  pi_result : PI_SUCCESS
...

smaslov-intel
smaslov-intel reviewed Mar 23, 2023
View reviewed changes
sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_common.hpp Outdated
uint32_t SerializeModeValue = 0;
if (ZeSerializeMode) {
SerializeModeValue = std::atoi(ZeSerializeMode);
} else if (UrL0SerializeMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, have UR_L0_SERIALIZE take precedence

@jandres742 jandres742 temporarily deployed to aws March 25, 2023 01:05 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws March 25, 2023 01:36 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

@smaslov-intel Rebased and introduced some of the suggestions.

@smaslov-intel
Copy link
Contributor

@smaslov-intel Rebased and introduced some of the suggestions.

LGTM. Please avoid rebase if not strictly necessary. It allows for incremental reviews.

@jandres742 jandres742 force-pushed the updateurenvvars branch 2 times, most recently from 6734945 to 69fd501 Compare March 29, 2023 16:40
@jandres742
Copy link
Contributor Author

@smaslov-intel : incremental changes on third commit: 69fd501

@jandres742 jandres742 temporarily deployed to aws March 29, 2023 17:42 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

Failing on CUDA with

********************
Failed Tests (1):
  SYCL :: GroupAlgorithm/reduce_sycl2020.cpp
failing also with CUDA. do we have a known issue for this also?

Seems like it is being fixed in #8860

@jandres742 jandres742 temporarily deployed to aws March 29, 2023 20:09 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

Rebased needed to include #8860

@jandres742 jandres742 temporarily deployed to aws March 30, 2023 02:06 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented Mar 30, 2023

@jandres742, could you resolve merge conflicts, please?

[SYCL][UR][L0] Add UR_L0 environment variables
4ec687d 
Define UR_LO_DEBUG and UR_L0_SERIALIZE, and keep
their ZE_ counterparts for now.

And do some reorganization

Signed-off-by: Jaime Arteaga <[email protected]>
@jandres742 jandres742 temporarily deployed to aws March 30, 2023 05:12 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws March 30, 2023 05:58 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

@jandres742, could you resolve merge conflicts, please?

Thanks @bader . Rebased.

@jandres742 jandres742 temporarily deployed to aws March 30, 2023 11:34 — with GitHub Actions Inactive
@jandres742 jandres742 mentioned this pull request Mar 30, 2023
Closed
@jandres742
Copy link
Contributor Author

@bader : this is failing with

********************
Failed Tests (1):
  SYCL :: AtomicRef/atomic_memory_order_acq_rel.cpp

it is flaky here #8847. is someone working on it? I will rerun to see if it works.

@jandres742 jandres742 closed this Mar 30, 2023
@jandres742 jandres742 reopened this Mar 30, 2023
@jandres742 jandres742 temporarily deployed to aws March 30, 2023 16:24 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws March 30, 2023 18:20 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

jandres742 commented Mar 30, 2023 •
edited
Loading

only failure is still

********************
Failed Tests (1):
  SYCL :: AtomicRef/atomic_memory_order_acq_rel.cpp

Reported in #8847

@intel/llvm-gatekeepers : could we merge this, to avoid further delays and rebases?

@againull againull merged commit 2b19458 into intel:sycl Mar 30, 2023
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 21, 2023
[SYCL][UR][L0] Add UR_L0 environment variables (intel#8732)
33eabf4 
Define UR_LO_DEBUG and UR_L0_SERIALIZE, and keep
their ZE_ counterparts for now.

And do some reorganization

---------

Signed-off-by: Jaime Arteaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmyates bmyates bmyates left review comments

@smaslov-intel smaslov-intel smaslov-intel 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.
5 participants
@jandres742 @smaslov-intel @bader @bmyates @againull