Skip to content
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.

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

[SYCL] Fix failing ABI tests when LLVM_LIBDIR_SUFFIX is set #1605

Merged
merged 1 commit into from
May 2, 2020

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Apr 29, 2020

Make ABI tests look for libsycl in the correct directory.

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 29, 2020

I've removed XFAIL from all CUDA USM tests, since the tests pass on my machine.
I'd like to see what errors the CI finds.

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 29, 2020

I see all CUDA USM tests passing on

  • Tesla K40, CentOS 8, CUDA 10.1 / 10.2, NVIDIA drivers 440.64.00
  • Tesla V100, CentOS 7, CUDA 10.1, NVIDIA drivers 440.33.01
  • Tesla T4, CentOS 7, CUDA 10.1, NVIDIA drivers 440.33.01

@bader bader requested review from asavonic and vzakhari April 29, 2020 11:39
@@ -1,6 +1,5 @@
// REQUIRES: cpu,linux
// RUN: %clangxx -fsycl -c %s -o %t.o
// RUN: %clangxx -fsycl %t.o %sycl_libs_dir/libsycl-glibc.o -o %t.out
// RUN: %clangxx -fsycl %s -lsycl -o %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.

Link devicelib tests with libsycl instead of individual intermediate object files.

%sycl_libs_dir/libsycl-glibc.o is not an intermediate object file, see https://github.com/intel/llvm/blob/sycl/libdevice/cmake/modules/SYCLLibdevice.cmake#L33

These object files provide definitions for library functions used in device code. libsycl.so does not contain these definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that only for libsycl-glibc.o or also for the others ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That applies for all other object files used in sycl/test/devicelib directory:

  • libsycl-msvc.o
  • libsycl-glibc.o
  • libsycl-complex.o
  • libsycl-complex-fp64.o
  • libsycl-cmath.o
  • libsycl-cmath-fp64.o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way, how do I build them ?
make sycl-toolchain does not do it, at least using my (possibly outdated) build recipe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'd better move libdevice tests into libdevice project directory. Then, the libdevice tests that require libsycl may be enabled only if libsycl is actually being built. There may still be pure libdevice tests that do not require libsycl, although, I do not think there are any right now.

@asavonic, @fwyzard, if it sounds reasonable to you, I can make this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way should be fine: keeping the libdevice tests under sycl, and enabling them only if libdevice is built, or moving them to the libdevice directory, and enabling those that depend on SYCL only if it s being built.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't understand why I could get the tests to pass when libdevice was not built...

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'd better move libdevice tests into libdevice project directory. Then, the libdevice tests that require libsycl may be enabled only if libsycl is actually being built. There may still be pure libdevice tests that do not require libsycl, although, I do not think there are any right now.

@asavonic, @fwyzard, if it sounds reasonable to you, I can make this change.

I think it is easier to propagate a flag from CMake to LIT and disable these tests if the feature is missing.

If we move them to the toplevel directory, we probably need to copy some bits from sycl/lit.cfg.py as well (all those CPU_RUN_PLACEHOLDER hacks). Not sure how to make them run for check-sycl 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.

OK, it is probably not worth it.

asavonic
asavonic previously approved these changes Apr 29, 2020
Copy link
Contributor

@asavonic asavonic left a comment

Choose a reason for hiding this comment

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

Approved to start buildbot

@fwyzard fwyzard changed the title [SYCL] Fix failing tests for different build configurations [SYCL] Fix failing ABI tests when LLVM_LIBDIR_SUFFIX is set Apr 30, 2020
Make ABI tests look for libsycl in the correct directory.

Signed-off-by: Andrea Bocci <[email protected]>
@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 30, 2020

OK, I figured out why the CUDA USM tests were not failing for me (asserts were disabled), and will have a fix for those in a separate PR.

So this PR not is just about fixing the ABI tests when LLVM_LIBDIR_SUFFIX is set.

@bader bader merged commit 95ded99 into intel:sycl May 2, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request May 6, 2020
…_docs

* origin/sycl: (6482 commits)
  [SYCL][NFC] Clean formatting in Markdown documents (intel#1635)
  [SYCL][Doc] Remove obsolete parens from README (intel#1637)
  [SYCL] Fix failing ABI tests when LLVM_LIBDIR_SUFFIX is set (intel#1605)
  [SYCL] Fix warnings in libdevice (intel#1630)
  [SYCL][CUDA] Triage and clean LIT (intel#1620)
  [SYCL][NFC] Fix GCC 8 compilation warnings (intel#1631)
  [SYCL] Minor fixes in LowerWGScope
  [SYCL] PI: correct default interoperability plugin selection
  [SYCL] Add faster reduction implementations using atomic or/and intel::reduce() (intel#1615)
  [SYCL] Add sycl-ls utility for listing devices discovered/selected by SYCL RT (intel#1575)
  [SYCL] Fix getDeviceFromHandler declarations (intel#1626)
  [SPIR-V] Correct/improve declaration of SPIR-V builtins (intel#1519)
  [SYCL][USM] Improve USM allocator test and fix improper behavior. (intel#1538)
  [SYCL] Fix failing ABI LITs (intel#1622)
  [SYCL] Add support for MSVC internal math functions in device library (intel#1441)
  [SYCL] Add runtime library versioning (intel#1604)
  [SYCL] Check weak symbols in ABI dumps (intel#1609)
  [NFC][SYCL] Improve kernel metadata test (intel#1610)
  Revert "[SYCL] XFAIL LIT test due to duplicate diagnostic" (intel#1460)
  [SYCL] Move the reduction command group funcs out of handler.hpp (intel#1602)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants