[SYCL][NFC] Get started guide clean-up #590
Conversation
bader
requested review from
romanovvlad,
vladimirlaz,
v-klochkov and
pvchupin
bader
force-pushed
the
get-started-guide
branch
from
498acaa to
e19528e
Compare
|
@romanovvlad, I think I applied all your comments. |
| https://gcc.gnu.org/install/ | ||
| * `python` - https://www.python.org/downloads/release/python-2716/ | ||
| * `Visual Studio 2017` or later (Windows only) - | ||
| https://visualstudio.microsoft.com/downloads/ |
There was a problem hiding this comment.
Please specify exact version (MSVC 19.15.26730.0 and higher). We are observing build failures on earlier versions.
There was a problem hiding this comment.
Could you provide Visual Studio version, please?
https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes
There was a problem hiding this comment.
We had no problems with MSVC2017 before. If It does not work we could fix it.
CLANG requires MSVC2017 or newer, why we would raise the bar?
BTW, The current words "16 or later" look good. I would only add .0 after 16: i.e. "16.0 or later"
| * Intel GPU [Intel® Graphics Compute Runtime for | ||
| OpenCL™](https://github.com/intel/compute-runtime/releases) | ||
| * Intel CPU - [Experimental Intel® CPU Runtime for OpenCL™ Applications | ||
| with SYCL support](https://github.com/intel/llvm/releases/tag/2019-08) |
There was a problem hiding this comment.
for GPU you passed the general location for CPU RT you passed exact version.
Also different approach is used on Windows and Linux. Can we have common was of specifying RTs.
There was a problem hiding this comment.
I think that is okay to put link to a general location here. Certain recommended version is changed from time to time and mentioned in release notes. So, probably it worth say that exact recommended version can be found in release notes
There was a problem hiding this comment.
Also different approach is used on Windows and Linux. Can we have common was of specifying RTs.
What do you mean by "different approach"? Could you clarify, please?
| * Intel GPU [Intel® Graphics Compute Runtime for | ||
| OpenCL™](https://github.com/intel/compute-runtime/releases) | ||
| * Intel CPU - [Experimental Intel® CPU Runtime for OpenCL™ Applications | ||
| with SYCL support](https://github.com/intel/llvm/releases/tag/2019-08) |
There was a problem hiding this comment.
I think that is okay to put link to a general location here. Certain recommended version is changed from time to time and mentioned in release notes. So, probably it worth say that exact recommended version can be found in release notes
|
Might be worth fixing the GPU scheduler example while you're here so we can close this issue: #22 |
|
@vladimirlaz, @romanovvlad, please, review again to confirm that your concerns are resolved. |
bader
dismissed
romanovvlad’s stale review
Consider "LGTM" as "no changes required".
- "Table of contents" is added - A few unnecessary sections are removed - Information for testing Intel SYCL implementation with Khronos SYCL conformance test suite is added - Minor style fixes (80-char lines, markdown style, etc.) - Link to CPU runtime is updated - Link to Intel FPGA selector is added - NEOGPUDeviceSelector example is fixed - Compiler invocation example is updated - no need to link OpenCL library - Visual Studio version requirement is updated Signed-off-by: Alexey Bader <[email protected]>
bader
force-pushed
the
get-started-guide
branch
from
e828183 to
ef03437
Compare
| ### Linux: | ||
| ```bash | ||
| cd $SYCL_HOME | ||
| git clone https://github.com/KhronosGroup/OpenCL-ICD-Loader |
There was a problem hiding this comment.
Alexey, I missed the fact that you removed the build steps for ICD-Loader.
I believe that was not good move. make and nmake generators are stable, I also had some success with ninja during the main SYCL compiler build, but when ICD-Loader and Headers are not pre-built before the main SYCL build, ninja again gives this error:
D:\iusers\vklochko\ws\gs_public\build_ninja2>ninja sycl-toolchain
ninja: error: 'lib/OpenCL.lib', needed by 'bin/sycl.dll', missing and no known rule to make it
There was a problem hiding this comment.
@v-klochkov, I encountered similar error (unfortunately, I've lost build logs) while using MSVC generator: we definitely have something misconfigured/missing
There was a problem hiding this comment.
This looks like a bug in the cmake script, feel free to file a bug report!
* [SYCL] Add gpu_aot_target_opts parameter The gpu_aot_target_opts LIT parameter and the GPU_AOT_TARGET_OPTS CMake variable is introduced to allow customization of the target device for AOT compilation on GPU. Co-authored-by: Alexey Bader <[email protected]>
SYCL conformance test suite is added
library