Page MenuHomePhabricator

[OpenCL] Adding cl_intel_planar_yuv extension
ClosedPublic

Authored by sidorovd on Aug 29 2018, 12:08 AM.

Details

Reviewers
yaxunl
Anastasia
Summary

Just adding a preprocessor #define for the extension.

Patch by Alexey Sotkin

Diff Detail

Event Timeline

sidorovd created this revision.Aug 29 2018, 12:08 AM

Will this extension require any Clang changes that are not possible to add using this approach for vendor extensions:
https://clang.llvm.org/docs/UsersManual.html#opencl-extensions

If not I would suggest to add this to the header file instead.

sidorovd updated this revision to Diff 164157.Sep 6 2018, 1:08 AM

I'm not sure if I put pragmas in an appropriate place (I mean in the very beginning of the header).

Anastasia added inline comments.Sep 6 2018, 7:18 AM
lib/Headers/opencl-c.h
26 ↗(On Diff #164157)

@yaxunl, do you think we need to add some kind of architecture guard for such things? Like it should only be added if the architecture supports the extension? But I guess -cl-ext=+cl_intel_planar_yuv trick might not work here because it's not a Clang known extension?

So may be the right solution here is to introduce a target specific header? For now it can be explicitly included but we could think of a target hook to preload a target specific header...

Anastasia added inline comments.Sep 20 2018, 5:16 AM
lib/Headers/opencl-c.h
26 ↗(On Diff #164157)

@sidorovd, I am wondering if we could instead extend '-cl-ext=+cl_intel_planar_yuv' to work with non-builtin extensions? Would that be an acceptable solution for vendor extensions to be added to the common header?

sidorovd added inline comments.Sep 26 2018, 5:10 AM
lib/Headers/opencl-c.h
26 ↗(On Diff #164157)

Sorry for a long response.
I have covered a little study on how to enable extensions via adding them in clang, common header or without any changes (yeap, that is also a case) and how adding -cl-ext+ in the RUN string affects the test's (introduced in this patch, but with removed versioning) result. And found out that I don't understand how it works or it's supposed to work. Here is a result of my study:

Changescl-ext+cl-ext-no cl-ext
opencl-c.h ver >=1.2defined, known, supporteddefined, known, supporteddefined, known, supported
opencl-c.h ver < 1.2undefined, known, supportedundefined, unknownundefined, unknown
OpenCLExtensions.def ver >=1.2defined, known, supportedundefined, known, unsupporteddefined, known, supported
OpenCLExtensions.def ver < 1.2undefined, known, unsupportedundefined, known, unsupportedundefined, known, unsupported
No changes ver >=1.2undefined, known, supportedundefined, unknownundefined, unknown
No changes ver < 1.2undefined, known, supportedundefined, unknownundefined, unknown

So from my perspective there shouldn't be any difference between the result depending on an approach to use to enable an extension. But that is not how it's happening in reality, as an example see how values put in column #3 differs for changes in opencl-c.h and OpenCLExtensions.def.
Or am I wrong?

@Anastasia , since there is a problem I described, wouldn't you mind if I stay with the first iteration of the patch (adding the extension to OpenCLExtensions.def) and after we'll investigate what is wrong with -cl-ext approach?

@Anastasia , since there is a problem I described, wouldn't you mind if I stay with the first iteration of the patch (adding the extension to OpenCLExtensions.def) and after we'll investigate what is wrong with -cl-ext approach?

I would prefer not to add it to Clang directly unless absolutely necessary. I think it's ok to add this in the header for now as is, but then investigate how we can add an architecture guard later. Please, could you just move the testing into test/Headers/opencl-c-header.cl, because test/SemaOpenCL/extension-version.cl is for Clang built in extensions.

sidorovd updated this revision to Diff 169187.Oct 11 2018, 4:20 AM

Moved the test to test/Headers/opencl-c-header.cl

sidorovd updated this revision to Diff 169188.Oct 11 2018, 4:23 AM
  • Forgot to add clarification comments on conditional branching. Added.
This revision is now accepted and ready to land.Oct 19 2018, 11:13 AM
asavonic closed this revision.Oct 23 2018, 9:16 AM

Committed revision 345044