Glow: OpenCL backend as an opt-in rather than an opt-out ?
Today, the OpenCL backend is compiled by default, what generates error when compiling glow 'out of the box' on platforms that don't support OpenCL or that don't have OpenCL installed.
It is then required to disable explicitly OpenCL in the cmake command.
-DGLOW_WITH_OPENCL=OFF
Since on the contrary to the CPU, OpenCL is not available on all platforms, I think it would be relevant to have the OpenCL option OFF by default in the cmake.
Any opinion ?
tlepley-cadence
All 7 comments
@artemrakhov @opti-mix @ZchiPitt
nadavrot
on 3 Aug 2018
@tlepley-cadence I've no objections. Please submit a PR.
My only concern is that if OpenCL support is disabled, a person who submits a PR does not test if the OpenCL support code builds at all. And our CI currently also does not enable the OpenCL support. Thus, it is rather easy to submit and merge something that would break the OpenCL support and we would notice it only much later. Ideally, our CI should be able to at least build with the OpenCL support enabled. But this is another story ...
opti-mix
on 3 Aug 2018
@opti-mix It's a good point. Please decide what the default should me.
nadavrot
on 3 Aug 2018
@opti-mix My assumption was that the automatic non-regression engine (that is triggered when we submit a new PR) would check Glow with all options ON, independently from the default option value in cmake. From what you said, I understand that currently the non-regression engine builds and checks with OpenCL disabled. Is it correct ?
tlepley-cadence
on 3 Aug 2018
@tlepley-cadence Yes, this is correct. The Travis CI machines do not have GPUs and there is no OpenCL support of the box. There is #1181 with a proposal how to solve this, but so far it is not implemented. But at some point we should definitely enable the OpenCL support on CI builds.
opti-mix
on 3 Aug 2018
So, may be we should wait a bit until we can check that our OpenCL support builds on CI.
opti-mix
on 3 Aug 2018
I'm fine with this. I suggest to change the default option as soon as the Travis CI supports OpenCL.
tlepley-cadence
on 6 Aug 2018
Related issues
speryt
路
3Comments
gcatron
路
4Comments
alannnna
路
3Comments
stoklund
路
5Comments
georgeokelly
路
4Comments
Most helpful comment
@tlepley-cadence I've no objections. Please submit a PR.
My only concern is that if OpenCL support is disabled, a person who submits a PR does not test if the OpenCL support code builds at all. And our CI currently also does not enable the OpenCL support. Thus, it is rather easy to submit and merge something that would break the OpenCL support and we would notice it only much later. Ideally, our CI should be able to at least build with the OpenCL support enabled. But this is another story ...