Page MenuHomePhabricator

[OpenCL] Add macro definitions of OpenCL C 3.0 features
ClosedPublic

Authored by azabaznov on Feb 1 2021, 1:37 AM.

Details

Summary

This patch adds possibility to define OpenCL C 3.0 feature macros
via command line option or target setting.

Diff Detail

Event Timeline

azabaznov created this revision.Feb 1 2021, 1:37 AM
azabaznov requested review of this revision.Feb 1 2021, 1:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 1:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

azabaznov updated this revision to Diff 320710.Feb 2 2021, 1:26 AM

Rebased one more time

Anastasia added inline comments.
clang/lib/Basic/Targets.cpp
743

Btw we could add the other feature macros for earlier versions too but I guess it makes code more complicated?

clang/lib/Headers/opencl-c.h
17161 ↗(On Diff #320710)

Looping in @svenvh - I don't mind if we define those macros in headers for OpenCL 2.0. The only concern is that if we undef them here we will end up with different behavior between -finclude-default-header and -fdeclare-opencl-builtins. I would suggest not to undef them because it is better if we have coherency. Alternatively we could also add a third header with undefs that can be included at the end for both but it seems to make things even more complicated.

FYI __opencl_c_int64 is already added for all OpenCL versions.

clang/test/SemaOpenCL/features.cl
20

I think we should test all the macros that are being added.

svenvh added inline comments.Feb 2 2021, 4:58 AM
clang/lib/Headers/opencl-c.h
17161 ↗(On Diff #320710)

The only concern is that if we undef them here we will end up with different behavior between -finclude-default-header and -fdeclare-opencl-builtins

This is a valid point. Doing the undefs only in opencl-c.h will lead to a problem similar to https://reviews.llvm.org/D91429 .

A third header just for the undefs sounds like a bit of an overkill indeed, although having duplication isn't great either. Not sure what's best to be honest.

azabaznov added inline comments.Feb 2 2021, 7:16 AM
clang/lib/Basic/Targets.cpp
743

Yes, this will complicate things: we decided to generate a warning if any of core features is unsupported, right? So making OpenCL C 3.0 features as core in OpenCL C 2.0 will result in this kind of warning; distinguishing these features among the set of core functionality may require workarounds in clang, so let's keep them in headers only for OpenCL C 2.0.

clang/lib/Headers/opencl-c.h
17161 ↗(On Diff #320710)

My idea was just to temporary define features for built-ins and enums definitions as this is the only place where these macros may be useful for OpenCL C 2.0. However, I don't have any strong concerns if these macros will remain defined.

Anastasia added inline comments.Feb 2 2021, 9:12 AM
clang/lib/Basic/Targets.cpp
743

Ok, this is fine. If we find it useful they can be changed later on for OpenCL v2.0 too.

azabaznov updated this revision to Diff 321385.Feb 4 2021, 4:45 AM

Features remain defined in header for OpenCL C 2.0; adjusted the test

Anastasia added inline comments.Feb 4 2021, 5:11 AM
clang/test/SemaOpenCL/features.cl
2

since -cl-ext=-all doesn't affect the header functionality I suggest to drop it and use one one line with -finclude-default-header. Parsing time with this header is 300x slower so we should minimise its use in tests to avoid negative impact on clang testing.

Would it make sense to add a line without the header with -cl-std=CL2.0 or/and -cl-std=CL1.2 to check that there are indeed no macros defined?

15

Do you want to add -verify and expected-no-diagnostics directive for this or just change to FileCheck too?

azabaznov added inline comments.Feb 4 2021, 5:27 AM
clang/test/SemaOpenCL/features.cl
2

Would it make sense to add a line without the header with -cl-std=CL2.0 or/and -cl-std=CL1.2 to check that there are indeed no macros defined?

I think this makes no sense since we are planning to use these macros only internally for headers. Or if it's planned to include opencl-c-base.h by default, then this kind of lines can be eliminated for < 3.0 at all.

15

Ok, I think I'll use FileCheck to be consistent.

Anastasia added inline comments.Feb 4 2021, 5:43 AM
clang/test/SemaOpenCL/features.cl
2

I guess we still need to test the the macros are not added where they are not available which is not only OpenCL 3.0 or OpenCL 2.0 but also earlier standards?

For example for OpenCL 1.2 they shouldn't be defined with or without the header?

Btw I suggest to move the header functionality into clang/test/Headers/opencl-c-header.cl, where we already tests some extension macros for example cl_khr_subgroup_extended_types . In general it would be better to partition the tests but parsing that header has been so slow that we got shouted at for slowing down the clang testing.

azabaznov updated this revision to Diff 321425.Feb 4 2021, 7:20 AM

Adjusted test one more time: moved header functionality into separate test

azabaznov marked 2 inline comments as done.Feb 4 2021, 7:21 AM
Anastasia accepted this revision.Feb 5 2021, 2:18 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Feb 5 2021, 2:18 AM
This revision was landed with ongoing or failed builds.Feb 5 2021, 7:42 AM
This revision was automatically updated to reflect the committed changes.