Skip to content

[SYCL][DOC] Update docs to reflect devicelib change #1368

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

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

hiaselhans
Copy link
Contributor

in sycl/doc/GetStartedGuide.md

  • add 'libdevice' to external_projects and enable_projects
  • add 'libdevice_source_dir'
  • add cross reference to buildbot configuration script

romanovvlad
romanovvlad previously approved these changes Mar 22, 2020
@romanovvlad
Copy link
Contributor

@hiaselhans Please, sign-off you commit(git commit -s)

@romanovvlad romanovvlad linked an issue Mar 22, 2020 that may be closed by this pull request
describe the use of buildbot scripts in GetStartedGuide.md

 * use configure.py and compile.py
 * describe common flags
 * remove unacessible opencl-aot flags (->enabled by default)
 * add cross reference to buildbot configuration script
 * set DPCPP_HOME to ~/sycl_workspace in unix

Signed-off-by: hiaselhans <simon.klemenc@gmail.com>
@hiaselhans
Copy link
Contributor Author

okay, thanks @bader for your feedback, i updated the docs to use the buildbot scripts.

question i have:

  • should we maybe advice to use --no-ocl by default? It might be the path of least troubles for users (like me :) ) how is that on windows?
  • i haven't tested all this on windows, but expect it to work more less the same...?

@bader
Copy link
Contributor

bader commented Mar 23, 2020

  • should we maybe advice to use --no-ocl by default? It might be the path of least troubles for users (like me :) ) how is that on windows?

I think you laid it out perfectly. It might be useful to enable this option by default, but in this case I would not advice users to add this option, but rather change the script. I think it should be done in a separate PR. Does it sound reasonable?

  • i haven't tested all this on windows, but expect it to work more less the same...?

Looks good to me, although I haven't tried it neither. :-)

Thanks a lot for working on this!

@bader bader changed the title [SYCL][DOC] update docs to reflect devicelib change [SYCL][DOC] Update docs to reflect devicelib change Mar 23, 2020
bader
bader previously approved these changes Mar 23, 2020
Ruyk
Ruyk previously approved these changes Mar 23, 2020
mkdir %DPCPP_HOME%\build
cd %DPCPP_HOME%\build
python %DPCPP_HOME%\llvm\buildbot\configure.py -s %DPCPP_HOME%\llvm -o %DPCPP_HOME%\build -t release
python %DPCPP_HOME%\llvm\buildbot\compile.py -s %DPCPP_HOME%\llvm -o %DPCPP_HOME%\build
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a particular python version? Does it work with python3?

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 never tried on windows, but in my linux python3 is default and works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ruyk @hiaselhans I don't see anything version-specific in those scripts. I believe both versions are supported. And they definitely work with Python 3, as it's part of CI pipeline.

-DLLVM_EXTERNAL_SYCL_SOURCE_DIR=$DPCPP_HOME/llvm/sycl \
-DLLVM_EXTERNAL_LLVM_SPIRV_SOURCE_DIR=$DPCPP_HOME/llvm/llvm-spirv \
$DPCPP_HOME/llvm/llvm
In case you want to configure Cmake manually the up-to-date reference for variables is in these files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In case you want to configure Cmake manually the up-to-date reference for variables is in these files.
In case you want to configure CMake manually the up-to-date reference for variables is in these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -36,11 +36,16 @@ Throughout this document `DPCPP_HOME` denotes the path to the local directory
created as DPC++ workspace. It might be useful to create an environment variable
with the same name.


Copy link
Contributor

Choose a reason for hiding this comment

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

Why this space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accidential, fixed it :)

@hiaselhans hiaselhans dismissed stale reviews from Ruyk and bader via 3ab26d9 March 23, 2020 16:00
Signed-off-by: hiaselhans <simon.klemenc@gmail.com>
@hiaselhans
Copy link
Contributor Author

Well, thanks for the comments,
two questions that arise from this way to configure CMake:

  • how can we make --no-ocl default as suggested by @bader but still let it remain in buildbot
  • build failed with -DSYCL_ENABLE_WERROR=ON on my machine, should i change the script to include a --no-werror flag?

better to open a seperate issue and pr for this?

@romanovvlad romanovvlad requested a review from bader March 24, 2020 08:40
@bader
Copy link
Contributor

bader commented Mar 24, 2020

Well, thanks for the comments,
two questions that arise from this way to configure CMake:

  • how can we make --no-ocl default as suggested by @bader but still let it remain in buildbot
  • build failed with -DSYCL_ENABLE_WERROR=ON on my machine, should i change the script to include a --no-werror flag?

better to open a seperate issue and pr for this?

I suggest handling this issues separately.
Thanks!

@@ -55,55 +59,49 @@ Open a developer command prompt using one of two methods:
```bat
set DPCPP_HOME=%USERPROFILE%\sycl_workspace
mkdir %DPCPP_HOME%
cd %DPCPP_HOME%

git clone https://github.com/intel/llvm -b sycl
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 not sure if we really want to do this. But shallow clone would dramatically decrease download times for those who just want's to build the latest version (instead of working on new features/bugfixes).

Suggested change
git clone https://github.com/intel/llvm -b sycl
git clone --depth 1 https://github.com/intel/llvm -b sycl

Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit 75ee39b into intel:sycl Mar 24, 2020
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.

"libsycl-fallback-cassert.spv" not found
5 participants