- User Since
- Mar 5 2015, 8:05 AM (307 w, 23 h)
Added suggestions from Sven.
LGTM, great! Thanks! Perhaps the test can be simplified before committing...
LGTM! Thanks! Great work! The comment can be addressed before committing.
Wed, Jan 20
Tue, Jan 19
Mon, Jan 18
Fri, Jan 15
This patch contains very useful improvements to the design along with the behavioral fixes. I see that it opens more opportunities to bring the implementation in line with the conventional flow and therefore simplify the further development of new functionality. More improvements can be easily built on top of this work. I, therefore, suggest we drive towards finalizing this patch asap and I have made a couple of small comments that should hopefully be quick enough to address.
Thu, Jan 14
Fri, Jan 8
Added the correction from Sven.
Thu, Jan 7
Addressed review comments
Wed, Jan 6
This patch is uploaded on top of https://reviews.llvm.org/D93942
Added missing testing in extension-version.cl
Another fix from Marco.
Tue, Jan 5
(I only discovered the "Suggest Edit" feature halfway through the review; I didn't update the comments I already made as I'm not sure in which format you prefer them?)
Further change suggested by Marco
Addressed comments from Sven.
Mon, Jan 4
Thu, Dec 31
Addressed comments from Marco.
LGTM, the change seems reasonable. Thanks!
Wed, Dec 30
Fixed some build warning/formating issues.
Dec 18 2020
Dec 11 2020
Dec 10 2020
This looks much cleaner than the current flow! Thanks! We should just figure out a better place for defining the macros (see detailed comment in Targets.cpp).
Dec 8 2020
- Set all macros to value 1
Dec 3 2020
Dec 2 2020
Dec 1 2020
Nov 30 2020
FYI I just noticed this review is public, but not linked to cfe-commits. Not sure it is intentional though.
Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow. I also suggest removing the dependence on LangOpts. See detailed comments.
The testing can be improved before committing!
Nov 27 2020
Thanks! This looks much cleaner.
Nov 26 2020
After a recent discussion with Khronos on https://github.com/KhronosGroup/OpenCL-Docs/issues/500 and more thinking I start to realize that there is absolutely nothing new in OpenCL 3.0 features as it isn't a new concept at all. They use to be explained as optional core features in the extension spec from early spec versions. So I think what have changed mainly in OpenCL 3.0 is that the features that were core have now become optional while the current design in clang assumes that core features can't become optional. I feel this is what we should reflect better in the design right now. So the categories we have are:
- extension (conditionally supported, can have pragma to alter certain functionality)
- optional feature (conditionally supported, pragma has no effect)
- core feature (unconditionally supported, pragma has no effect)
Great! LGTM! Thanks!
Nov 25 2020
Nov 24 2020
It will need some decent review and maybe some testing by other CL users to make sure I've haven't messed up when used with a fully featured CL 3.0 stack.
I assume your change depends on some other changes that add the feature macro definitions. So I think we should link the other changes in at some point. CCing to @azabaznov here who is working on the features too.
Btw how about making some checks simpler. We could always define feature macros __opencl_c_atomic_scope_device, __opencl_c_generic_address_space for OpenCL 2.0 or C++ for OpenCL . Then everywhere we would only need to check feature macros instead of language versions and feature macros together.
Nov 23 2020
Nov 19 2020
LGTM for the fix! Do you think we could improve testing? I presume there is something that triggers a failure without your change...
Nov 17 2020
Added full diffs.
Added full diff.
- Added full diffs.