Conversation
|
|
bashbaug
left a comment
bashbaug
left a comment
There was a problem hiding this comment.
Note, I did not review all of the CI/CD and CMake changes, but here are some comments about the sample updates themselves.
| install(TARGETS ${OPENCL_SAMPLE_TARGET} CONFIGURATIONS ${CONFIG} DESTINATION ${CMAKE_INSTALL_BINDIR}/${CONFIG}) | ||
| install(FILES ${OPENCL_SAMPLE_KERNELS} CONFIGURATIONS ${CONFIG} DESTINATION ${CMAKE_INSTALL_BINDIR}/${CONFIG}) | ||
| install(FILES ${OPENCL_SAMPLE_SHADERS} CONFIGURATIONS ${CONFIG} DESTINATION ${CMAKE_INSTALL_BINDIR}/${CONFIG}) |
There was a problem hiding this comment.
This means that instead of the samples being installed to <install_prefix>/install/bin they will be installed to <install_prefix>/install/bin/<config>, so something like /path/to/sdk/install/bin/Release. Was this intended? I'm not opposed to this change, but it is different than what we had previously.
There was a problem hiding this comment.
It happened when adding the tests jobs to CI that in some environments the execution of the tests failed when configuring and building for Debug and Release (so first we configured for Debug and Release, then we built for Debug and Release and then we executed), and separating the install of the executables was solving it. Currently I'm not able to reproduce the old buggy behaviour, so probably some of the other changes of this PR triggered the fix for it. We can undo this change then IMHO.
There was a problem hiding this comment.
If the older behavior still works I think I'd slightly lean towards keeping the same installation directories, but this is something the working group should decide.
There was a problem hiding this comment.
Okay, let's see it then in the next WG meeting
samples/core/blur/main.c
Outdated
| // cl_khr_subgroup_shuffle requires OpenCL 2.0 | ||
| strcat(kernel_op, " -cl-std=CL2.0 "); |
There was a problem hiding this comment.
See comment above: there is no dependency between cl_khr_subgroup_shuffle and OpenCL C 2.0.
There was a problem hiding this comment.
(IIRC) I think this was a consequence of one runtime which didn't behave well if it wasn't set. I'll verify tomorrow the reason for this.
|
Latest commits:
|
bashbaug
left a comment
bashbaug
left a comment
There was a problem hiding this comment.
Thanks, many issues are fixed, but a few new ones have cropped up (or I didn't notice them the first time).
I don't think anything is broken with these changes per se, but the checks for OpenCL 2.0 and cl_khr_subgroups do mean that these samples cannot run on as many devices as they should. So, if we want to merge these changes as-is and fix things up afterwards to make progress, I'd be OK with that, though ideally they'd be fixed before merging.
Beanavil
force-pushed
the
release-cd
branch
3 times, most recently
from
c954192 to
807a04a
Compare
EwanC
left a comment
EwanC
left a comment
There was a problem hiding this comment.
I'm not an expert in this SDK repo, so not clicked the big green approve button. But I have looked through this PR to try help unblock getting reviews on it and looks fine to me apart from the few minor comments I've made.
| // 5) Query OpenCL version to compile for. | ||
| // If no -cl-std option is specified then the highest 1.x version | ||
| // supported by each device is used to compile the program. Therefore, | ||
| // it's only necessary to add the -cl-std option for 2.0 and 3.0 OpenCL | ||
| // versions. | ||
| const std::string dev_version = device.getInfo<CL_DEVICE_VERSION>(); | ||
| cl::string compiler_options; | ||
| constexpr int max_major_version = 3; |
There was a problem hiding this comment.
I suspect other samples will need to do something similar, so this would be a great SDK utility function. This code from the OpenCL CTS may be a useful starting point:
Strictly speaking, this really ought to be checking for the OpenCL C versions supported by the device, not for the OpenCL version supported by the device, but given the current couplings between the OpenCL version and the OpenCL C versions I think this is OK.
|
@bashbaug so should we merge? |
|
Merging as discussed in the October 1st teleconference. We'll check that the install directories are working as we intend after merging. Specifically, with a non-multi-config generator (e.g. Makefiles), does the install go into per-config directories, or into a single |
This reverts the changes made on KhronosGroup#88 that modified the install location of the samples
* Revert install tree modifications This reverts the changes made on #88 that modified the install location of the samples * Make vendors path not depend on hard-coded POCL install path
7 participants
-Werror//WXis enabled in CI, however a list of warnings had to be enabled, since their resolution was deemed out-of-scope.clinfoas a dependency, that is built with the project.CMakeLists.txtto be able to produce binary Debian packages usingcpack.DebSourcePkg.cmakewhich is intended to run in CMake script mode. This script generates thedebian/control,debian/changeloganddebian/rulesfiles which are required to build a Debian source package.clinfois not part of the aforementioned Debian source package, since the Debian package build on Launchpad cannot access the Internet. A potential solution would be forkingclinfoand referencing it as a submodule, but this was decided to be out-of-scope for now.A ToDo item is to revert the referenced submodule commits to the
KhronosGrouprepos, once the respective PRs have been merged.This PR is considered to be complete, albeit review remarks and/or changes to related PRs might warrant minor updates.