User Details
- User Since
- Sep 23 2020, 3:53 AM (28 w, 6 d)
Fri, Apr 9
Remove unrelated change, fixed error in test to check only cl_khr_fp64 for OpenCL C < 1.2
Addressed comments, rebased after refactoring: https://reviews.llvm.org/D97058
Wed, Apr 7
Thanks for working on this! Looks good to me in general. I have a small comment.
Thu, Apr 1
Thanks for feedback!
Tue, Mar 30
Looks good to me. Thanks!
Thanks for the patch! Sorry for the delay
Tue, Mar 16
I have one more though.
Mon, Mar 15
@Anastasia, thanks for landing this! Sorry for delay, I though that this can wait until Monday
Do you also get fatal error: 'opencl-c-base.h' file not found? If so, we might need at least to file a clang bug that we should look at before the next release.
Mar 12 2021
Yes, I was able to reproduce the failure with -target arm64-apple-macosx flag, I provided the fix to set explicit target: https://reviews.llvm.org/D98539.
Thanks guys,
Yeah, I'm looking into it, but I can't reproduce it locally: the test passes at x86_64 linux system. I'll revert the change if it takes too much time to investigate what's going on. Thanks.
Mar 10 2021
Ok, addressing in a separate patch is reasonable, but why do you think that we will break backward compatibility? My current worry is that the implementation is so messy and inconsistent that it will take us longer time if we do the incremental steps. Also, we risk introducing the regressions since it is so hard to make sense out of it.
Replaced atomic_double implicit definition
Corrected some mistakes, added a test for diagnosing undeclared identifiers when a extension is unsupported. Generally leaving the change as it is as completely removing pragma may break backward compatibility now: let's do it in a separate patch and notify everyone before doing that. Also, we should wait before spec will be finalized.
Mar 4 2021
That's awesome!
Mar 3 2021
Check 'isEnabled' is now private: it is used only for non-core or non-optional core features;
creation of implicit type definitions is guarder with extension support check; minor refactoring
Mar 2 2021
Thanks for working on this. LGTM in general. I'll let Marco comment on the latest changes.
Feb 25 2021
I see the update regaring this functionality on github issue: https://github.com/KhronosGroup/OpenCL-Docs/issues/82#issuecomment-785496025. Is it a right way to reflect this information in`OpenCLExtensions.def`?
FYI, I would even be ok if we remove the need for enabling non-core
Feb 20 2021
Feb 19 2021
Thanks! I review is shortly. As for now, I also was doing some refactoring: https://reviews.llvm.org/D97058. Check-clang passes, as I see there are no conflicts for now.
Feb 17 2021
Feb 16 2021
Thanks for review. Last change is a small fix for if clang-tidy warnings
Fix clang-tidy warnings
Feb 15 2021
Feb 12 2021
Rewrote 'RemoveAddressSpaceFromPtr' to return canonical type
Feb 11 2021
Feb 10 2021
Thanks for review. Just to be clear: can I proceed with this? Or we should discuss it first? As I see possible this change can be abandoned after discussion.
Feb 5 2021
Feb 4 2021
Adjusted test one more time: moved header functionality into separate test
Features remain defined in header for OpenCL C 2.0; adjusted the test
Renamed language options and adjusted comment.
Feb 2 2021
Rebased one more time
Rebased
Rebased
Feb 1 2021
I suggest that renaming 'extension' to 'option' (extensions + features) concept to be done in separate commit. There are a lot of places to change all over the place in clang.
Due to refactoring in https://reviews.llvm.org/D92277 this flow is no longer valid. New patch for this functionality is here: https://reviews.llvm.org/D95776.
Jan 25 2021
Jan 22 2021
Removed getCoreFeatures() as it's not used in this change.
Jan 21 2021
Thanks for feedback. I agree that providing diagnostics for unsupported core features is enough for now (I guess I'll try to come up with something for OpenCL C 3.0 features to set them unconditionally for 2.0, or at least these macros can be defined in header only). So I removed setting of core features (and also addressed all cosmetic concerns).
Jan 20 2021
Jan 18 2021
Jan 14 2021
Changes in the latest patch:
Dec 13 2020
Dec 9 2020
Changes in the latest patch:
Dec 1 2020
Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow
Nov 29 2020
I didn't want to end up with a big refactoring from this patch but now I start to think that considering the situation it might be unavoidable. :(
@Anastasia, there is common functionality with your approach in https://reviews.llvm.org/D91531. I guess these two patches can be unified at some point once we discuss it.
Nov 24 2020
Addressed almost all technical and cosmetic concerns. Except putting reference of OpenCLOptions in Sema due to const of TargetInfo. I am going to think about that later. Also:
Hi! Great to see the interest in OpenCL C 3.0!
Nov 23 2020
So if I understand it well you are suggesting to perform a check for every parsed macro definition that would identify whether the macro has a name matching what is provided in -cl-ext=? Then if the name matches it would check whether the macro should be defined or not?
Perhaps if you give me an example it would help to understand
Nov 19 2020
Yes, in general this approach looks good to me conceptually. I have two suggestions:
Nov 18 2020
Nov 17 2020
Nov 11 2020
What did you think in mind regarding activation of features for SPIR?
Addressed all concerns except replacing __opencl_c_int64 definition into header. The reason for this as follows: this macro should be predefined regardless if clang includes default header or not.
Nov 6 2020
I guess targets like SPIR will be supporting all features by default?
Oct 21 2020
Oct 14 2020
Looks good to me, thanks for the note.
Oct 9 2020
Anastasia, Sven, thanks for the review! Anastasia, it seems that I don't have write access yet. Would you mind landing this change on behalf of me? Thanks!