UoB-HPC / BabelStream

STREAM, for lots of devices written in many programming models
Other
323 stars 110 forks source link

ComputeCpp does not compile with signed array sizes #92

Closed tom91136 closed 3 years ago

tom91136 commented 3 years ago

This was caught by #91 after trying to enable ComputeCpp (still not possible yet due to authentication requirements). I've added better filtering in #91 as well so all the compiler warnings show up in the CI log.

> /opt/ComputeCpp-CE-2.3.0-x86_64-linux-gnu/bin/compute++ -sycl -O2 -mllvm -inline-threshold=1000 -intelspirmetadata -sycl-target spir64 -std=c++1z -I"/opt/computecpp_archive/ComputeCpp-CE-2.3.0-x86_64-linux-gnu/include" -I"/home/tom/babelstream-upstream/CL" -DSYCL       -DCL_TARGET_OPENCL_VERSION=220  -D_GLIBCXX_USE_CXX11_ABI=0 SYCLStream.cpp

SYCLStream.cpp:94:44: error: non-constant-expression cannot be narrowed from type 'int' to 'size_t' (aka 'unsigned long') in initializer list [-Wc++11-narrowing]
    cgh.parallel_for<copy_kernel>(range<1>{array_size}, [=](id<1> idx)
                                           ^~~~~~~~~~
SYCLStream.cpp:313:16: note: in instantiation of member function 'SYCLStream<float>::copy' requested here
template class SYCLStream<float>;
               ^
SYCLStream.cpp:94:44: note: insert an explicit cast to silence this issue
    cgh.parallel_for<copy_kernel>(range<1>{array_size}, [=](id<1> idx)
                                           ^~~~~~~~~~

... reports the same thing for all range<1>{array_size} calls ...

This is without any extra warning flags, we also got the same thing but as warnings in hipSYCL:

/opt/hipsycl/cff515c/lib/cmake/hipSYCL/syclcc-launcher --launcher-cxx-compiler=/usr/lib64/ccache/c++ --launcher-syclcc=/opt/hipsycl/cff515c/bin/syclcc-clang  --hipsycl-platform=omp /usr/lib64/ccache/c++  -DNDEBUG CMakeFiles/babelstream.dir/SYCLStream.cpp.o CMakeFiles/babelstream.dir/main.cpp.o -o babelstream  -Wl,-rpath,/opt/hipsycl/cff515c/lib /opt/hipsycl/cff515c/lib/libhipSYCL-rt.so
SYCLStream.cpp:94:44: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:126:43: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:110:43: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:143:45: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:160:47: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:221:44: warning: narrowing conversion of ‘(int)((SYCLStream<float>*)this)->SYCLStream<float>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:94:44: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:126:43: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:110:43: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:143:45: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:160:47: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
SYCLStream.cpp:221:44: warning: narrowing conversion of ‘(int)((SYCLStream<double>*)this)->SYCLStream<double>::array_size’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]

DPC++ compiled fine and reported nothing (!)

We got several options here:

  1. Append -Wno-narrowing, ComputeCpp compiles after this but probably not a good thing
  2. static_cast to the correct type at warning/error site, or don't use initialiser lists
  3. use size_t for array_size

The only place array_size is used for SYCL are:

Not part of this issue but the N in the dot kernel might need to be int as per 9a69d3d5d982f0983974d16ace54a017cb330f4e.

Other than that, we aren't using it in any benchmark kernels directly; I vote option 3.

tomdeakin commented 3 years ago

Yes, option 3 is good: make array_size a size_t in the header file. Good catch!