Page MenuHomePhabricator

Add .rgba syntax extension to ext_vector_type types
ClosedPublic

Authored by pirama on May 24 2016, 3:29 PM.

Details

Summary

This patch enables .rgba accessors to ext_vector_type types and adds
tests for syntax validation and code generation.

'a' and 'b' can appear either in the point access mode or the numeric
access mode (for indices 10 and 11). To disambiguate between the two
usages, the accessor type is explicitly passed to relevant methods.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama updated this revision to Diff 58336.May 24 2016, 3:29 PM
pirama retitled this revision from to Add .rgba syntax extension to ext_vector_type types.
pirama updated this object.
pirama added subscribers: cfe-commits, srhines.
rsmith edited edge metadata.May 24 2016, 3:52 PM

Looks like this extension was added at some point between 1.1 and 2.1. It would make sense to produce an ExtWarn for it if the OpenCL standard version is less than the one that introduced it (whenever that was) -- that would match what we do for extensions in other languages.

Looks like this extension was added at some point between 1.1 and 2.1. It would make sense to produce an ExtWarn for it if the OpenCL standard version is less than the one that introduced it (whenever that was) -- that would match what we do for extensions in other languages.

This isn't just for OpenCL (actually we don't care about it in the context of OpenCL at all). We really are adding this as more of an extension for C99, where Android has used this in the past. In the case of OpenCL, I also don't know that it should be recognized as invalid, because it can be lowered to an appropriate construct on any implementation (since they all have to support regular 4-vectors).

Looks like this extension was added at some point between 1.1 and 2.1. It would make sense to produce an ExtWarn for it if the OpenCL standard version is less than the one that introduced it (whenever that was) -- that would match what we do for extensions in other languages.

This isn't just for OpenCL (actually we don't care about it in the context of OpenCL at all). We really are adding this as more of an extension for C99, where Android has used this in the past. In the case of OpenCL, I also don't know that it should be recognized as invalid, because it can be lowered to an appropriate construct on any implementation (since they all have to support regular 4-vectors).

I'm not suggesting it be treated as invalid. This extension is part of at least OpenCL 2.1, but it's not part of OpenCL 1.0. ext_vector_type is Clang's implementation of the OpenCL vector type. Therefore if the user asks us to support OpenCL 1.0, we should give them a warning if they use this syntax.

If we're not building an OpenCL source file, then yes, this is our extension and it makes sense for it to be available unconditionally (without a warning).

bader added a subscriber: bader.May 25 2016, 2:20 AM

I'm not suggesting it be treated as invalid. This extension is part of at least OpenCL 2.1, but it's not part of OpenCL 1.0. ext_vector_type is Clang's implementation of the OpenCL vector type. Therefore if the user asks us to support OpenCL 1.0, we should give them a warning if they use this syntax.

If we're not building an OpenCL source file, then yes, this is our extension and it makes sense for it to be available unconditionally (without a warning).

To avoid confusion, I'd like to note that rgba selector was first introduced in OpenCL C++ kernel language, which is currently in provisional state and will be part of OpenCL version 2.2.
https://www.khronos.org/registry/cl/specs/opencl-2.2-cplusplus.pdf (page 18).

OpenCL version 2.1 uses OpenCL C 2.0 kernel language, which doesn't support rgba selector.

https://www.khronos.org/registry/cl/ - latest versions of OpenCL specs.

To summarize, there are two scenarios where a warning is warranted when the source language is OpenCL:

I'm not suggesting it be treated as invalid. This extension is part of at least OpenCL 2.1, but it's not part of OpenCL 1.0. ext_vector_type is Clang's implementation of the OpenCL vector type. Therefore if the user asks us to support OpenCL 1.0, we should give them a warning if they use this syntax.

  • use of ext_vector_type in OpenCL 1.0 and earlier.

To avoid confusion, I'd like to note that rgba selector was first introduced in OpenCL C++ kernel language, which is currently in provisional state and will be part of OpenCL version 2.2.

  • use of rgba selector in OpenCL 2.1 and earlier.

I'll look at how to add the first warning. I'll most probably upload a separate CL for this.

I can easily add the second warning in this CL - to emit one on *any* OpenCL source. Alexey: can you take care of revising this warning when OpenCL2.2 is added as a langugage standard?

pirama updated this revision to Diff 58525.May 25 2016, 3:30 PM
pirama edited edge metadata.

Added warnings when rgba is used with OpenCL

I'm not suggesting it be treated as invalid. This extension is part of at least OpenCL 2.1, but it's not part of OpenCL 1.0. ext_vector_type is Clang's implementation of the OpenCL vector type. Therefore if the user asks us to support OpenCL 1.0, we should give them a warning if they use this syntax.

  • use of ext_vector_type in OpenCL 1.0 and earlier.

Looks like OpenCL 1.0 also has vector data types (Section 6.1.2 https://www.khronos.org/registry/cl/specs/opencl-1.0.pdf). So, I don't think it is necessary to emit warnings for ext_vector_types at all.

Looks good other than the recent information about the version of OpenCL that actually specifies this.

include/clang/Basic/DiagnosticSemaKinds.td
7899 ↗(On Diff #58525)

This should be an ExtWarn, not a Warning, and should be named ext_.... That way we'll reject it under -cl-std=CL2.0 -pedantic-errors.

7900 ↗(On Diff #58525)

I think the result of the discussion was that this is added in OpenCL 2.2. The way we phrase this in other diagnostics is:

"vector component name '%0' is an OpenCL 2.2 extension"
lib/Sema/SemaExprMember.cpp
334–336 ↗(On Diff #58525)

This should presumably be OpenCLVersion < 220.

pirama updated this revision to Diff 58535.May 25 2016, 4:20 PM

Switched to ExtWarn, updated warning's message and made the version check strict w.r.t. 2.2.

pirama updated this revision to Diff 58537.May 25 2016, 4:24 PM

Renamed diagnostic to use ext_ prefix.

Anastasia added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
7900 ↗(On Diff #58537)

Could it be an error instead for CL <2.2? It isn't a valid feature and we have already rejected similar cases with an error.

How about changing to something like:

"vector component name '%0' is not supported in earlier than OpenCL version 2.2"

See similar diagnostics above - err_opencl_invalid_read_write, err_opencl_unknown_type_specifier.

Using extension might be confusing because we have core spec and extension spec in OpenCL.

pirama added inline comments.May 26 2016, 11:02 AM
include/clang/Basic/DiagnosticSemaKinds.td
7900 ↗(On Diff #58537)

Since .rgba is just a syntactic feature in the frontend that requires no support from the runtime, should this be an error? Making this an ExtWarn, like Richard suggested, will reject these under -pedantic-errors. But I'll let you make this call.

I'll update the diagnostic message to the following, if that's ok:

"vector component name '%0' cannot be used earlier than OpenCL version 2.2.

pirama updated this revision to Diff 58813.May 27 2016, 11:05 AM

Switched to emitting errors when rgba is used in OpenCL < 2.2. Also, fixed the
diagnostic's name mismatch at its point of issue.

Friendly ping...

rsmith added inline comments.Jun 10 2016, 2:52 PM
include/clang/Basic/DiagnosticSemaKinds.td
7900 ↗(On Diff #58813)

I don't see any good reason to reject this in earlier versions of OpenCL. Clang has its own extensions and is not beholden to the OpenCL spec's notion of extensions in this regard. Generally, wherever reasonable, we support features from later versions of a language as extensions in earlier versions.

However, since "extension" means something in the OpenCL specification, we could say "OpenCL 2.2 feature" rather than "OpenCL 2.2 extension" in the ExtWarn message.

lib/Sema/SemaExprMember.cpp
337–339 ↗(On Diff #58813)

diagBegin -> DiagBegin

pirama updated this revision to Diff 60425.Jun 10 2016, 4:17 PM

Convert error to warning, update tests, and rename variable name.

Anastasia added inline comments.Jun 13 2016, 9:29 AM
include/clang/Basic/DiagnosticSemaKinds.td
7907 ↗(On Diff #60425)

I am not too bothered with this specific case and I agree compiler implementation can deviate from the spec in certain beneficial cases (though I hope not too many of them).

Just for the record: we have diagnosed with an errors all other similar situations. Also my understanding is that compilation should largely work compliant to the passed language version. This is something we have been doing for OpenCL mainly - making sure to accept or reject code according to the language version requirement. This way if the code is compiled with features that aren't supported by either hardware or a driver user will be given diagnostics about it and spec compliance acts as a contract guarantee between compiler and other components involved.

pirama updated this revision to Diff 60718.Jun 14 2016, 12:19 PM
  • Add TargetInfo for 32-bit and 64-bit RenderScript
pirama updated this revision to Diff 60723.Jun 14 2016, 12:24 PM
  • Undo bad merge from different patch
rsmith added inline comments.Jun 29 2016, 1:13 PM
include/clang/Basic/DiagnosticSemaKinds.td
7907 ↗(On Diff #60723)

As mentioned, normal Clang policy is to issue an ExtWarn in such cases unless there is some actual problem with accepting the feature as an extension. This *does* cause us to produce a diagnostic, which, as you say, is important.

The C and C++ standards are both clear that producing a diagnostic is a sufficient response to code that is ill-formed, and that we are still allowed to accept the program after producing the diagnostic; if the OpenCL specification says something else, it is simply broken and we should get it fixed.

test/Misc/warning-flags.c
15–19 ↗(On Diff #60723)

Please follow these instructions.

pirama updated this revision to Diff 62305.Jun 29 2016, 4:25 PM

Added GroupInfo to warning.

rsmith accepted this revision.Jul 22 2016, 11:27 AM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Jul 22 2016, 11:27 AM
This revision was automatically updated to reflect the committed changes.