Skip to content

[SYCL] Enable LIT testing with CUDA BE #1458

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 1 commit into from
Apr 7, 2020
Merged

[SYCL] Enable LIT testing with CUDA BE #1458

merged 1 commit into from
Apr 7, 2020

Conversation

vladimirlaz
Copy link
Contributor

Signed-off-by: Vladimir Lazarev vladimir.lazarev@intel.com

@vladimirlaz vladimirlaz requested a review from Ruyk April 2, 2020 14:05
@bader bader added the cuda CUDA back-end label Apr 2, 2020
bader
bader requested changes Apr 2, 2020
View reviewed changes
@romanovvlad romanovvlad requested a review from alexbatashev April 3, 2020 15:01
@bader bader mentioned this pull request Apr 3, 2020
Merged
@vladimirlaz vladimirlaz force-pushed the private/vlazarev/enable_lit_for_cuda_be branch from 0a713cf to b896df5 Compare April 3, 2020 17:45
@vladimirlaz vladimirlaz requested a review from romanovvlad as a code owner April 3, 2020 17:45
@vladimirlaz vladimirlaz force-pushed the private/vlazarev/enable_lit_for_cuda_be branch 2 times, most recently from 54bfc66 to 295680b Compare April 3, 2020 18:00
bader
bader reviewed Apr 3, 2020
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.

Looks much better. Thanks!

I think we already have similar changes proposed by #1288, #1300, #1303, #1304. Do you suggest closing them?

bader
bader reviewed Apr 3, 2020
View reviewed changes
sycl/test/scheduler/DataMovement.cpp
// RUN: %t.out
// RUN: %CPU_RUN_PLACEHOLDER %t.out
// RUN: %GPU_RUN_PLACEHOLDER %t.out
// RUN: %ACC_RUN_PLACEHOLDER %t.out
Copy link
Contributor

Choose a reason for hiding this comment

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

@romanovvlad, could you take a look this change, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Host device is now affects only default_selector. So we can test all available devices in this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with this change.

bader
bader reviewed Apr 3, 2020
View reviewed changes
sycl/unittests/pi/cuda/test_primary_context.cpp
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a TODO here, please?
It looks like we need to do something to enable these tests. Right?

Copy link
Contributor Author

@vladimirlaz vladimirlaz Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

All PI/cuda tests are disabled. I do expect that they will be enabled in one shot. An it is out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, we should do that after we have stable LIT results.

@vladimirlaz vladimirlaz force-pushed the private/vlazarev/enable_lit_for_cuda_be branch from 295680b to b626ba0 Compare April 3, 2020 18:12
@vladimirlaz
[SYCL] Enable LIT testing with CUDA BE
ea89d42 
Signed-off-by: Vladimir Lazarev <vladimir.lazarev@intel.com>
@vladimirlaz vladimirlaz force-pushed the private/vlazarev/enable_lit_for_cuda_be branch from b626ba0 to ea89d42 Compare April 3, 2020 18:26
@vladimirlaz
Copy link
Contributor Author

vladimirlaz commented Apr 4, 2020 •
edited
Loading

Save build results without CI change: http://ci.llvm.intel.com:8010/#/builders/37/builds/259
The latest result with CI changes are available in buildbot/Lit_With_Cuda (run on CUDA device)

@vladimirlaz
Copy link
Contributor Author

milar changes propos

I think we can close those PRs...

@Ruyk
Copy link
Contributor

Ruyk commented Apr 6, 2020

@bjoernknafla

romanovvlad
romanovvlad approved these changes Apr 6, 2020
View reviewed changes
sycl/test/scheduler/DataMovement.cpp
// RUN: %t.out
// RUN: %CPU_RUN_PLACEHOLDER %t.out
// RUN: %GPU_RUN_PLACEHOLDER %t.out
// RUN: %ACC_RUN_PLACEHOLDER %t.out
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with this change.

@romanovvlad
Copy link
Contributor

@bader are you OK with the change?

Ruyk
Ruyk reviewed Apr 7, 2020
View reviewed changes
Copy link
Contributor

@Ruyk Ruyk left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me. @bjoernknafla may have further comments as he has worked on similar patches

sycl/tools/CMakeLists.txt
@@ -4,6 +4,31 @@ set(CMAKE_CXX_EXTENSIONS OFF)

add_executable(get_device_count_by_type get_device_count_by_type.cpp)
add_dependencies(get_device_count_by_type ocl-headers ocl-icd)

if( SYCL_BUILD_PI_CUDA )
find_package(CUDA 10.0 REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CUDA 10 the version running in your buildbots? we have only tested 10.1 minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are using 10.1 as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the script?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on a follow up PR for LIT fixes (reworking an old one based on this PR) and can introduce the change to 10.1 in that if you are ok with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am introducing the change to 10.1 in the following PR: #1303

sycl/unittests/pi/cuda/test_primary_context.cpp
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, we should do that after we have stable LIT results.

@bjoernknafla
Copy link
Contributor

This looks very good to me. I will re-base my LIT fixes on top.

@bader bader merged commit 1cca686 into intel:sycl Apr 7, 2020
@vladimirlaz vladimirlaz deleted the private/vlazarev/enable_lit_for_cuda_be branch April 8, 2020 06:26
@bjoernknafla bjoernknafla mentioned this pull request Apr 8, 2020
Merged
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 9, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/sche…
0032e53 
…duler_docs

* origin/sycl: (26 commits)
  [Driver][SYCL] Move include/sycl header before other system header locations (intel#1492)
  [BuildBot] Improve usability of buildbot scripts (intel#1472)
  [NFC] Add GitHub actions badges to README file (intel#1496)
  [SYCL] Improve error handling for kernel invocation (intel#1209)
  [SYCL][Driver] Fix SYCL standards' handling for '-fsycl -fsycl-device-only' invocations (intel#1371)
  [SYCL] Move type checks to later in Semantic Analysis lifecycle (intel#1465)
  [CI] Download fixed versions of Python tools (intel#1485)
  [SYCL] Fix sub_group::broadcast (intel#1482)
  [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488)
  [SYCL] Only export public API (intel#1456)
  [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475)
  [SYCL] Enable LIT testing with CUDA BE (intel#1458)
  [SYCL] Fix float to half-type conversion (intel#1395)
  [NFC] Cleanup unneded macro from builtins implementation (intel#1445)
  Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479)
  [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  ...
@bader bader mentioned this pull request Apr 10, 2020
Closed
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 15, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/basi…
961afa4 
…c_abi_checks

* origin/sycl: (625 commits)
  [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488)
  [SYCL] Only export public API (intel#1456)
  [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475)
  [SYCL] Enable LIT testing with CUDA BE (intel#1458)
  [SYCL] Fix float to half-type conversion (intel#1395)
  [NFC] Cleanup unneded macro from builtins implementation (intel#1445)
  Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479)
  [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454)
  [SYCL] Change priority of devices in default_selector (intel#1264)
  [CI] Update CODEOWNERS matching rules order (intel#1468)
  [SYCL] Share PFWG lambda object through shared memory (intel#1455)
  [CI] Fix CODEOWNERS file syntax (intel#1464)
  [SYCL][CUDA] Fix active context when creating base event (intel#1447)
  [SYCL] Diagnose implicit declaration of kernel function type (intel#1450)
  [BuildBot] Modify configure script (intel#1421)
  ...
@vladimirlaz vladimirlaz mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjoernknafla bjoernknafla bjoernknafla left review comments

@bader bader bader approved these changes

@Ruyk Ruyk Ruyk left review comments

@romanovvlad romanovvlad romanovvlad approved these changes

@alexbatashev alexbatashev Awaiting requested review from alexbatashev

Assignees

@romanovvlad romanovvlad

Labels
cuda CUDA back-end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vladimirlaz @Ruyk @romanovvlad @bjoernknafla @bader