Skip to content

[SYCL] Don't include level zero header explicitly to the interop header #2249

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

Merged
merged 1 commit into from
Aug 4, 2020
Merged

[SYCL] Don't include level zero header explicitly to the interop header #2249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sycl/include/CL/sycl/backend/level_zero.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
#pragma once

#include <CL/sycl.hpp>
#include <level_zero/ze_api.h>
// This header should be included by users.
//#include <level_zero/ze_api.h>
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any "standard" macro defined in level_zero/ze_api.h, which we can check?
It would be better to have something like:

#if !defined(LEVEL_ZERO_INCLUDED)
  #error "level_zero/ze_api.h must be included to use Level Zero interoperability with SYCL"
#endif

instead of these two lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is probably soemthing we could use, e.g. _ZE_API_H, but not sure how "standard" this is. Probably not (+@jandres742)
Also, since all the uses of L0 types are in templates, it is probably fine to have L0 included after this header

Copy link
Contributor

Choose a reason for hiding this comment

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

I recalled that we tried the same approach with OpenCL headers in the past and it caused some issues, so we include OpenCL headers from SYCL headers.
What is the motivation for this change? Will it be clear for user that includes of Level Zero headers are missing and how to fix this problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

The ze_api.h has this

#ifndef _ZE_API_H

but not quite sure what we are trying to solve here. Why we cannot include the ze_api.h in the interop header? We know the interop header is gonna need it anyway, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand it correctly, this patch is supposed to address the compile time issue with missing headers on the systems w/o Intel GPU (e.g. cpu-only system or a system with NVIDIA GPU). In these cases DPC++ is not supposed to use Level Zero plug-in and there might be no headers on the system.

I'm not sure that asking users to provide headers in the best option here, but it's one way to address this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bader You are right.


__SYCL_INLINE_NAMESPACE(cl) {
namespace sycl {
Expand Down