-
Notifications
You must be signed in to change notification settings - Fork 130
[SYCL] E2E test for interop_task with Level-Zero and OpenCL #252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, apply the same comments to OpenCL test as well.
// clang-format off | ||
#include <level_zero/ze_api.h> | ||
#include <CL/sycl/backend/level_zero.hpp> | ||
// clang-format on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why clang-format comments are needed here, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format puts <level_zero/ze_api.h> after <CL/sycl/backend/level_zero.hpp> and compilation fails because backend/level_zero.hpp
uses Level-zero types and expects ze_api.h
included before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Alternative way to handle this is to put an empty line between ze_api.h
and level_zero.hpp
includes:
// Native API header
#include <level_zero/ze_api.h>
// SYCL API headers
#include <CL/sycl.hpp>
#include <CL/sycl/backend/level_zero.hpp>
// clang-format off | ||
#include <level_zero/ze_api.h> | ||
#include <CL/sycl/backend/level_zero.hpp> | ||
// clang-format on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Alternative way to handle this is to put an empty line between ze_api.h
and level_zero.hpp
includes:
// Native API header
#include <level_zero/ze_api.h>
// SYCL API headers
#include <CL/sycl.hpp>
#include <CL/sycl/backend/level_zero.hpp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except one minor comment regarding disabling clang-format.
Co-authored-by: vladimirlaz <[email protected]>
@mikhail-nikolskiy, could you please resolve conflicts to let us follow up with the change? |
@mikhail-nikolskiy does the PR is still needed? |
Needed if we want tests for this small change merged into llvm repo https://github.com/intel/llvm/pull/3606/files, not needed otherwise |
@mikhail-nikolskiy could you please resolve the conflict? |
@vladimirlaz, BTW, I believe that test in |
nothing to be done. It is known regression and we are working on the fix. |
Similar to #168 but extended for
to test llvm#3606
WIP because stable failure withconst void*
exception observed (on Release build only) on interop-opencl-interop-task-mem.cpp line 46.The instability issue with
const void*
exception fixed in test code by replacing '&' with '=' in interop_task lambda - copy accessor objects by value, not by reference.