Skip to content

[SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI #10681

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 4 commits into from
Aug 10, 2023
Merged

[SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI #10681

merged 4 commits into from
Aug 10, 2023

Conversation

jandres742
Copy link
Contributor

L0 already provides an interface to access PCI information without needing to set an environment variable to access SYSMAN interfaces.

@jandres742 jandres742 requested review from a team as code owners August 3, 2023 18:07
smaslov-intel
smaslov-intel reviewed Aug 3, 2023
View reviewed changes
sycl/doc/EnvironmentVariables.md
@@ -152,7 +152,6 @@ Note that conflicting configuration tuples in the same list will favor the last

| Environment variable | Values | Description |
| -------------------- | ------ | ----------- |
| `SYCL_ENABLE_PCI` | Integer | When set to 1, enables obtaining the GPU PCI address when using the Level Zero backend. The default is 0. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is described not in experimental env vars section, I think that we keep it for now, say it is deprecated, and is ignored, and will be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, changing @smaslov-intel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaslov-intel please check latest commit, 5937848, to confirm statement added is correct.

smaslov-intel
smaslov-intel reviewed Aug 3, 2023
View reviewed changes
sycl/plugins/unified_runtime/ur/adapters/level_zero/device.cpp
return UR_RESULT_ERROR_INVALID_VALUE;
}
ZesStruct<zes_pci_properties_t> ZeDevicePciProperties;
ZE2UR_CALL(zesDevicePciGetProperties, (ZeDevice, &ZeDevicePciProperties));
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need SYSMAN for anything? perhaps some header includes can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaslov-intel : yes, still for free memory.

@jandres742 jandres742 temporarily deployed to aws August 3, 2023 18:25 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws August 3, 2023 18:55 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws August 3, 2023 20:12 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

Error on CPU is unrelated. here we are touching only L0 GPU code:

$ ":" "RUN: at line 3"
note: command had no output on stdout or stderr
$ "env" "ONEAPI_DEVICE_SELECTOR=opencl:cpu" "/__w/llvm/llvm/build-e2e/HostInteropTask/Output/host-task-dependency4.cpp.tmp.out"
note: command had no output on stdout or stderr
error: command failed with exit status: -9
error: command reached timeout: True

error on HIP is unrelated for the same reason

default_selector()      : No device of requested type available. -1 (PI_ERRO...
accelerator_selector()  : No device of requested type available. -1 (PI_ERRO...
cpu_selector()          : No device of requested type available. -1 (PI_ERRO...
gpu_selector()          : No device of requested type available. -1 (PI_ERRO...
custom_selector(gpu)    : No device of requested type available. -1 (PI_ERRO...
custom_selector(cpu)    : No device of requested type available. -1 (PI_ERRO...
custom_selector(acc)    : No device of requested type available. -1 (PI_ERRO...

bader
bader reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Please, update the PR title "Remove" -> "Deprecate".

bader
bader reviewed Aug 7, 2023
View reviewed changes
@jandres742 jandres742 temporarily deployed to aws August 7, 2023 19:31 — with GitHub Actions Inactive
bader
bader approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Thanks!
The documentation part looks good to me. Don't forget to update the PR title before the merge (see this comment - #10681 (review)).

@jandres742 jandres742 temporarily deployed to aws August 7, 2023 19:51 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

Thanks! The documentation part looks good to me. Don't forget to update the PR title before the merge (see this comment - #10681 (review)).

oh yes, thanks @bader . Changed.

@jandres742 jandres742 changed the title [SYCL][UR][L0] Remove SYCL_ENABLE_PCI [SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI Aug 7, 2023
Jaime Arteaga added 4 commits August 7, 2023 13:35
[SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI
d17852b 
L0 already provides an interface to access PCI information
without needing to set an environment variable to access
SYSMAN interfaces.

Signed-off-by: Jaime Arteaga <[email protected]>
Add deprecate note for SYCL_ENABLE_PCI
8f7ae2f 
Signed-off-by: Jaime Arteaga <[email protected]>
Add warning on deprecated variable
a78ab04 
Signed-off-by: Jaime Arteaga <[email protected]>
Address comment on documentation
c64c5b9 
Signed-off-by: Jaime Arteaga <[email protected]>
@jandres742 jandres742 temporarily deployed to aws August 7, 2023 20:37 — with GitHub Actions Inactive
@jandres742 jandres742 temporarily deployed to aws August 7, 2023 21:32 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor Author

@sergey-semenov , @intel/llvm-reviewers-runtime : friendly reminder on review pending above.

@jandres742
Copy link
Contributor Author

@intel/llvm-gatekeepers : please merge when possible.

@aelovikov-intel
Copy link
Contributor

HIP:

Timed Out Tests (1):
  SYCL :: Basic/built-ins/marray_geometric.cpp

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

$ "env" "ONEAPI_DEVICE_SELECTOR=ext_oneapi_hip:gpu" "/__w/llvm/llvm/build-e2e/AtomicRef/Output/or.cpp.tmp.out"
# command stderr:
Memory access fault by GPU node-1 (Agent handle: 0x24eb2f0) on address 0x7fc581600000. Reason: Page not present or supervisor privilege.

@aelovikov-intel aelovikov-intel merged commit 2d12863 into intel:sycl Aug 10, 2023
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 21, 2023
[SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI (intel#10681)
ac3acb1 
L0 already provides an interface to access PCI information without
needing to set an environment variable to access SYSMAN interfaces.

---------

Signed-off-by: Jaime Arteaga <[email protected]>
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
@mdtoguchi
[SYCL][UR][L0] Deprecate SYCL_ENABLE_PCI (intel#10681)
1eea38e 
L0 already provides an interface to access PCI information without
needing to set an environment variable to access SYSMAN interfaces.

---------

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

@bader bader bader approved these changes

@sergey-semenov sergey-semenov sergey-semenov approved these changes

@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 @aelovikov-intel @bader @sergey-semenov @smaslov-intel