Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] E2E test for interop_task with Level-Zero and OpenCL #252

Merged
merged 12 commits into from
Jul 12, 2021

Conversation

mikhail-nikolskiy
Copy link

@mikhail-nikolskiy mikhail-nikolskiy commented Apr 23, 2021

Similar to #168 but extended for

  • buffers and images
  • Level-Zero and OpenCL

to test llvm#3606

WIP because stable failure with const 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.

@mikhail-nikolskiy mikhail-nikolskiy changed the title [WIP] [SYCL] E2E test fot interop_task with Level-Zero and OpenCL [SYCL] E2E test for interop_task with Level-Zero and OpenCL Apr 26, 2021
Copy link

@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, apply the same comments to OpenCL test as well.

Comment on lines 8 to 11
// clang-format off
#include <level_zero/ze_api.h>
#include <CL/sycl/backend/level_zero.hpp>
// clang-format on
Copy link

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?

Copy link
Author

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

Copy link

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>

Comment on lines 8 to 11
// clang-format off
#include <level_zero/ze_api.h>
#include <CL/sycl/backend/level_zero.hpp>
// clang-format on
Copy link

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>

bader
bader previously approved these changes Apr 28, 2021
Copy link

@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.

LGTM, except one minor comment regarding disabling clang-format.

@bader bader requested a review from vladimirlaz April 29, 2021 16:50
bader
bader previously approved these changes Apr 29, 2021
@vladimirlaz
Copy link

@mikhail-nikolskiy, could you please resolve conflicts to let us follow up with the change?

@vladimirlaz
Copy link

@mikhail-nikolskiy does the PR is still needed?

@mikhail-nikolskiy
Copy link
Author

@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

@vladimirlaz
Copy link

@mikhail-nikolskiy could you please resolve the conflict?

@mikhail-nikolskiy
Copy link
Author

@vladimirlaz,
I resolved the conflict, but CI failed on unrelated cuda test, how to solve that?

BTW, I believe that test in interop-level-zero-interop-task-mem.cpp was failing under CI (but not locally) because of problem passing params to interop_task lambda discussed here. It shouldn't fail anymore after changing [&] to [=] in interop_task lambda.

@vladimirlaz
Copy link

@vladimirlaz,
I resolved the conflict, but CI failed on unrelated cuda test, how to solve that?

nothing to be done. It is known regression and we are working on the fix.

@vladimirlaz vladimirlaz merged commit 0d30e9f into intel:intel Jul 12, 2021
smaslov-intel pushed a commit to smaslov-intel/llvm-test-suite that referenced this pull request Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants