[SYCL] Modify configure script

[hiaselhans]

• use "--no-ocl" by default -> new "--system-ocl" flag (use system
  OpenCl headers and do not download)
• don't use "DSYCL_WERROR" by default, only on buildbot
• don't exclude buildbot dir in gitignore

Signed-off-by: hiaselhans simon.klemenc@gmail.com

[alexbatashev]

Overall, I like the idea of new options. But I doubt these changes will be propagated to buildbot pre-commit. If I'm not mistaken, some changes on the server side are also required. Contents of .github/workflows/linux_postcommit.yml only affects post-commit builds.

Tagging @DoyleLi and @bader to comment on this.

[bader]

I would prefer to keep this option ON by default, so contributors can't miss new warnings locally.

[hiaselhans]

no problem, i can swap this around, however i am noticing some -Werror=class-conversion:

FAILED: tools/sycl/source/CMakeFiles/sycl.dir/detail/builtins_math.cpp.o 
/usr/bin/c++  -DUSE_PI_CUDA -DXPTI_ENABLE_INSTRUMENTATION -DXPTI_STATIC_LIBRARY -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dsycl_EXPORTS -Itools/sycl/source -IXXX/llvm/sycl/source -I/usr/include/libxml2 -Iinclude -IXXX/llvm/llvm/include -IXXX/llvm/xpti/include -IXXX/llvm/sycl/include -Itools/sycl/OpenCL/inc -isystem /opt/cuda/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wextra -Wno-deprecated-declarations -Werror -O3  -fPIC   -UNDEBUG -std=c++11 -MD -MT tools/sycl/source/CMakeFiles/sycl.dir/detail/builtins_math.cpp.o -MF tools/sycl/source/CMakeFiles/sycl.dir/detail/builtins_math.cpp.o.d -o tools/sycl/source/CMakeFiles/sycl.dir/detail/builtins_math.cpp.o -c XXX/llvm/sycl/source/detail/builtins_math.cpp
In file included from XXX/llvm/sycl/include/CL/sycl/types.hpp:53,
                 from XXX/llvm/sycl/source/detail/builtins_helper.hpp:12,
                 from XXX/llvm/sycl/source/detail/builtins_math.cpp:18:
XXX/llvm/sycl/include/CL/sycl/multi_ptr.hpp:520:3: error: converting ‘cl::sycl::multi_ptr<const void, Space>’ to the same type will never use a type conversion operator [-Werror=class-conversion]
  520 |   operator multi_ptr<const void, Space>() const {
      |   ^~~~~~~~

this comes using c++ (Arch Linux 9.3.0-1) 9.3.0.
The idea was to ease the path for people wanting to compile intel/llvm using werror=OFF.
This reveals another one:

In function ‘void cl::sycl::detail::initValue(const char*, const char*)’,
    inlined from ‘void cl::sycl::detail::readConfig()’ at XXX/llvm/sycl/source/detail/config.cpp:96:16:
XXX/llvm/sycl/source/detail/config.cpp:42:12: warning: ‘char* strncpy(char*, const char*, size_t)’ output may be truncated copying 1 byte from a string of length 255 [-Wstringop-truncation]
   42 |     strncpy(SYCLConfigBase<Name>::MStorage, Value, MaxSize);                   \
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
XXX/llvm/sycl/source/detail/config.def:14:1: note: in expansion of macro ‘CONFIG’
   14 | CONFIG(SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP, 1, __SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP)
      | ^~~~~~

So, the most elegant would be to fix these and add a more recent version of g++ to buildbot.
Then make werror default but a flag to disable?

[bader]

I don't have a strong opinion on that. As long as CI prevents new warnings - I can configure my local builds as I want.

I'd like to hear what others think about this.

[hiaselhans]

So, i will modify the pr to do only the following instead:

add --no-werr in order to compile on gcc >= 8.2. Solving the Wclass-conversion and Wstringop-truncation is out of scope for this PR.

for the --no-ocl vs --system-ocl i proposed to advice users to use no-ocl in the docs with #1368 but back then the prefered option was to make it default in the script.
I see that this requires modifying the buildbot, but on the other hand I feel that we should - by default - guide the potential user to the smoothest way to get started with this great compiler :)

Is that okay?

[bader]

So, i will modify the pr to do only the following instead:

add --no-werr in order to compile on gcc >= 8.2. Solving the Wclass-conversion and Wstringop-truncation is out of scope for this PR.

for the --no-ocl vs --system-ocl i proposed to advice users to use no-ocl in the docs with #1368 but back then the prefered option was to make it default in the script.
I see that this requires modifying the buildbot, but on the other hand I feel that we should - by default - guide the potential user to the smoothest way to get started with this great compiler :)

Is that okay?

@hiaselhans, thanks!
--no-werr makes sense to me.

I would still proceed with --no-ocl -> --system-ocl change.
@alexbatashev, if I understand it correctly, buildbot "build" job will do unnecessary job with configuring OpenCL headers and ICD loader, but it still should continue to work.
Once this change is applied we can:

1. Remove separate buildbot jobs configuring OpenCL headers and ICD loader.
2. Pass --system-ocl option to skip configuring OpenCL headers and ICD loader during the "build" step.

@DoyleLi, does it make sense to you?

[hiaselhans]

perfect, thanks @bader!

I will wait for #1434 to be merged and rebase on that.

as for --no-ocl - if you want that - I can also just leave it there for now and comment as "Unused", while the inverse --system-ocl is in place. Then we can remove it once the buildbot command is changed?

[bader]

perfect, thanks @bader!

I will wait for #1434 to be merged and rebase on that.

#1434 is merged.

as for --no-ocl - if you want that - I can also just leave it there for now and comment as "Unused", while the inverse --system-ocl is in place. Then we can remove it once the buildbot command is changed?

AFAIK, BuildBot doesn't set --no-ocl option, so there is no reason to keep it.
@alexbatashev concerned that to keep the same behavior BuildBot must set a new option.
My argument is that it should be okay to change the behavior, but I'd great if @DoyleLi or @vladimirlaz to confirm.

[bader]

LGTM, but I'd like @vladimirlaz or @DoyleLi to approve before merging.

[hiaselhans]

thanks! :)