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

Improvements to setup.py #426

Merged
merged 7 commits into from
May 21, 2021
Merged

Improvements to setup.py #426

merged 7 commits into from
May 21, 2021

Conversation

diptorupd
Copy link
Contributor

@diptorupd diptorupd commented May 4, 2021

Closes #361

  • The build_backend.py is not invoked using a sub-process. We now use importlib to properly import the file as a module.
  • build_backend.py now provides a function to run the build steps to compile DPCTLSyclInteface.
  • Initial changes to build_backend.py to make it possible to pass in different build options. Presently, the only new option that was added is l0_support.
  • Minor clean ups and fixes to setup.py.

@diptorupd diptorupd requested a review from oleksandr-pavlyk May 4, 2021 19:07
setup.py Outdated Show resolved Hide resolved
@oleksandr-pavlyk
Copy link
Collaborator

The failure on Windows is due to %ONEAPI_ROOT%\setvars.bat setting %DPCPP_ROOT% environment variable, which does not play well with us using DPCPP module, so execution of FindDPCPP.cmake is entirely skipped, and variables DPCPP_OPENCL_LIBRARY and DPCPP_SYCL_LIBRARY end up being not set, causing linkage problems.

The suggested solution is to renamed to CMake module.

@diptorupd
Copy link
Contributor Author

The suggested solution is to renamed to CMake module.

#319 has the change. Let us try once more after #319 and maybe #386 is merged.

The logic to build the backend is moved inside a function, allowing
to pass argument controlling the build.

Currently this is used to specify whether to use L0 support (hardcoded to True),
but opens a door to specify custom compiler to use to build SyclInterface library.

setup.py no longer uses subprocess to invoke build_backed, but uses
importlib to import the function defined there and calls the function with
appropriate arguments.

scipts/build_backend.py retains the function of the script, calling the build_backend
function with default arguments (no L0 support)
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the setup_pie branch 2 times, most recently from 49b3902 to f04a81b Compare May 20, 2021 19:59
@oleksandr-pavlyk
Copy link
Collaborator

The following worked file for me:

# here $LDH is path to local-disk-home
python setup.py develop --sycl-compiler-prefix=${LDH}/sycl_workspace/llvm/build

setup.py Outdated Show resolved Hide resolved
python setup.py develop ---help displays them, with use messaes

Options are

   --level-zerop-support=(true|false)
   --coverage=(true|false)
   --sycl-compiler-path=(path to prefix, optional)

If path is not provided backend builder looks up ONEAPI_ROOT variable.

In the path, builder looks for bin/dpcpp (Linux), or bin/dpcpp.exe (Windows).
If found, cmake does see -DDPCTL_DPCPP_ROOT variable set.

Otherwise the variable is set, and clang++ is used instead of dpcpp
@oleksandr-pavlyk
Copy link
Collaborator

README and Quickstart documents were modified.

@diptorupd diptorupd mentioned this pull request May 21, 2021
CODE_COVERAGE was used to influence whether the backend SyclInterface library
was built in debug mode with coverage over dpctp-capi/ test suite collected or not.

It was also influencing how Cython extensions were built. It would pass linetrace=true
option to cythonize, and define CYTHON_TRACE preprocessor variable when compiling
Cython-generated C++ source files with compiler.

This change removes use CODE_COVERAGE all together. Instead:

1. Custom setuptools command 'build_ext' is implemented which auto-adds
   'CYTHON_TRACE' to the list of defines received by downstream build_ext
   command if develop command received --coverage=True

   cythonize call was removed from extensions() function, allowing
   ``python setup.py develop --help` execute cleanly without running
   cythonize.

   Consequentially, linetrace Cython directive will need to be added to
   each .pyx file in a separate commit.  This is safe to do per

   https://cython.readthedocs.io/en/latest/src/tutorial/profiling_tutorial.html#enabling-line-tracing

   since it is a no-op unless CYTHON_TRACE preprocessor variable is also set,
   which is only done when --coverage=True is used

2. install setuptools command removed support for `--coverage` option, and always
   builds backend without coverage.

   This is to avoid inadvertent installation of debug build of library in Python prefix.
This is safe to do per

https://cython.readthedocs.io/en/latest/src/tutorial/profiling_tutorial.html#enabling-line-tracing

Its use allows improved coverage testing of Cython modules when Cython
sources are compiled with CYTHON_TRACE/CYTHON_TRACE_NOGIL preprocessor
variables defined.
@diptorupd diptorupd merged commit 32bf0a6 into master May 21, 2021
@diptorupd diptorupd deleted the setup_pie branch May 21, 2021 22:26
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.

Optional Level Zero
2 participants