Add .rgba syntax extension to ext_vector_type types

[pirama]

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.

[rsmith]

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.

[srhines]

In D20602#438528, @rsmith wrote:

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).

[rsmith]

In D20602#438643, @srhines wrote:

In D20602#438528, @rsmith wrote:

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]

In D20602#438667, @rsmith wrote:

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.

[pirama]

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]

Added warnings when rgba is used with OpenCL

[pirama]

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.

[rsmith]

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]

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

[pirama]

Renamed diagnostic to use ext_ prefix.

[Anastasia]

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]

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]

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

[pirama]

Friendly ping...

[rsmith]

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]

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

[Anastasia]

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]

• Add TargetInfo for 32-bit and 64-bit RenderScript

[pirama]

• Undo bad merge from different patch

[pirama]

Ping...

[rsmith]

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]

Added GroupInfo to warning.

[pirama]

Ping...

cfe/trunk/include/clang/AST/Type.h

static bool classof(const Type *T) {
     return T->getTypeClass() == Vector || T->getTypeClass() == ExtVector;
   }
 };
 
 /// ExtVectorType - Extended vector type. This type is created using
 /// __attribute__((ext_vector_type(n)), where "n" is the number of elements.
 /// Unlike vector_size, ext_vector_type is only allowed on typedef's. This
 /// class enables syntactic extensions, like Vector Components for accessing
-/// points, colors, and textures (modeled after OpenGL Shading Language).
+/// points (as .xyzw), colors (as .rgba), and textures (modeled after OpenGL
+/// Shading Language).
 class ExtVectorType : public VectorType {
   ExtVectorType(QualType vecType, unsigned nElements, QualType canonType) :
     VectorType(ExtVector, vecType, nElements, canonType, GenericVector) {}
   friend class ASTContext;  // ASTContext creates these.
 public:
   static int getPointAccessorIdx(char c) {
     switch (c) {
     default: return -1;
-    case 'x': return 0;
-    case 'y': return 1;
-    case 'z': return 2;
-    case 'w': return 3;
+    case 'x': case 'r': return 0;
+    case 'y': case 'g': return 1;
+    case 'z': case 'b': return 2;
+    case 'w': case 'a': return 3;
     }
   }
   static int getNumericAccessorIdx(char c) {
     switch (c) {
       default: return -1;
       case '0': return 0;
       case '1': return 1;
       case '2': return 2;
switch (c) {
       case 'd': return 13;
       case 'E':
       case 'e': return 14;
       case 'F':
       case 'f': return 15;
     }
   }
 
-  static int getAccessorIdx(char c) {
-    if (int idx = getPointAccessorIdx(c)+1) return idx-1;
+  static int getAccessorIdx(char c, bool isNumericAccessor) {
+    if (isNumericAccessor)
       return getNumericAccessorIdx(c);
+    else
+      return getPointAccessorIdx(c);
   }
 
-  bool isAccessorWithinNumElements(char c) const {
-    if (int idx = getAccessorIdx(c)+1)
+  bool isAccessorWithinNumElements(char c, bool isNumericAccessor) const {
+    if (int idx = getAccessorIdx(c, isNumericAccessor)+1)
       return unsigned(idx-1) < getNumElements();
     return false;
   }
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
   static bool classof(const Type *T) {
     return T->getTypeClass() == ExtVector;

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
 def ObjCRetainBlockProperty : DiagGroup<"objc-noncopy-retain-block-property">;
 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;
 def ObjCInvalidIBOutletProperty : DiagGroup<"invalid-iboutlet">;
 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;
 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
+def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def PointerArith : DiagGroup<"pointer-arith">;
 def PoundWarning : DiagGroup<"#warnings">;
 def PoundPragmaMessage : DiagGroup<"#pragma-messages">,

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_opencl_enqueue_kernel_local_size_args : Error<
   "mismatch in number of block parameters and local size arguments passed">;
 def err_opencl_enqueue_kernel_invalid_local_size_type : Error<
   "local memory sizes need to be specified as uint">;
 def err_opencl_enqueue_kernel_blocks_non_local_void_args : Error<
   "blocks used in device side enqueue are expected to have parameters of type 'local void*'">;
 def err_opencl_enqueue_kernel_blocks_no_args : Error<
   "blocks in this form of device side enqueue call are expected to have have no parameters">;
+
+// OpenCL v2.2 s2.1.2.3 - Vector Component Access
+def ext_opencl_ext_vector_type_rgba_selector: ExtWarn<
+  "vector component name '%0' is an OpenCL version 2.2 feature">,
+  InGroup<OpenCLUnsupportedRGBA>;
 } // end of sema category
 
 let CategoryName = "OpenMP Issue" in {
 // OpenMP support.
 def err_omp_expected_var_arg : Error<
   "%0 is not a global variable, static local variable or static data member">;
 def err_omp_expected_var_arg_suggest : Error<
   "%0 is not a global variable, static local variable or static data member; "

cfe/trunk/lib/AST/Expr.cpp

bool ExtVectorElementExpr::containsDuplicateElements() const {
 
   return false;
 }
 
 /// getEncodedElementAccess - We encode the fields as a llvm ConstantArray.
 void ExtVectorElementExpr::getEncodedElementAccess(
     SmallVectorImpl<uint32_t> &Elts) const {
   StringRef Comp = Accessor->getName();
-  if (Comp[0] == 's' || Comp[0] == 'S')
+  bool isNumericAccessor = false;
+  if (Comp[0] == 's' || Comp[0] == 'S') {
     Comp = Comp.substr(1);
+    isNumericAccessor = true;
+  }
 
   bool isHi =   Comp == "hi";
   bool isLo =   Comp == "lo";
   bool isEven = Comp == "even";
   bool isOdd  = Comp == "odd";
 
   for (unsigned i = 0, e = getNumElements(); i != e; ++i) {
     uint64_t Index;
 
     if (isHi)
       Index = e + i;
     else if (isLo)
       Index = i;
     else if (isEven)
       Index = 2 * i;
     else if (isOdd)
       Index = 2 * i + 1;
     else
-      Index = ExtVectorType::getAccessorIdx(Comp[i]);
+      Index = ExtVectorType::getAccessorIdx(Comp[i], isNumericAccessor);
 
     Elts.push_back(Index);
   }
 }
 
 ShuffleVectorExpr::ShuffleVectorExpr(const ASTContext &C, ArrayRef<Expr*> args,
                                      QualType Type, SourceLocation BLoc,
                                      SourceLocation RP) 

cfe/trunk/lib/Sema/SemaExprMember.cpp

case IMA_Error_Unrelated:
     diagnoseInstanceReference(*this, SS, R.getRepresentativeDecl(),
                               R.getLookupNameInfo());
     return ExprError();
   }
 
   llvm_unreachable("unexpected instance member access kind");
 }
 
+/// Determine whether input char is from rgba component set.
+static bool
+IsRGBA(char c) {
+  switch (c) {
+  case 'r':
+  case 'g':
+  case 'b':
+  case 'a':
+    return true;
+  default:
+    return false;
+  }
+}
+
 /// Check an ext-vector component access expression.
 ///
 /// VK should be set in advance to the value kind of the base
 /// expression.
 static QualType
 CheckExtVectorComponent(Sema &S, QualType baseType, ExprValueKind &VK,
                         SourceLocation OpLoc, const IdentifierInfo *CompName,
                         SourceLocation CompLoc) {
CheckExtVectorComponent(Sema &S, QualType baseType, ExprValueKind &VK,
 
   // Check that we've found one of the special components, or that the component
   // names must come from the same set.
   if (!strcmp(compStr, "hi") || !strcmp(compStr, "lo") ||
       !strcmp(compStr, "even") || !strcmp(compStr, "odd")) {
     HalvingSwizzle = true;
   } else if (!HexSwizzle &&
              (Idx = vecType->getPointAccessorIdx(*compStr)) != -1) {
+    bool HasRGBA = IsRGBA(*compStr);
     do {
+      // Ensure that xyzw and rgba components don't intermingle.
+      if (HasRGBA != IsRGBA(*compStr))
+        break;
       if (HasIndex[Idx]) HasRepeated = true;
       HasIndex[Idx] = true;
       compStr++;
     } while (*compStr && (Idx = vecType->getPointAccessorIdx(*compStr)) != -1);
+
+    // Emit a warning if an rgba selector is used earlier than OpenCL 2.2
+    if (HasRGBA || (*compStr && IsRGBA(*compStr))) {
+      if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion < 220) {
+        const char *DiagBegin = HasRGBA ? CompName->getNameStart() : compStr;
+        S.Diag(OpLoc, diag::ext_opencl_ext_vector_type_rgba_selector)
+          << StringRef(DiagBegin, 1)
+          << S.getLangOpts().OpenCLVersion << SourceRange(CompLoc);
+      }
+    }
   } else {
     if (HexSwizzle) compStr++;
     while ((Idx = vecType->getNumericAccessorIdx(*compStr)) != -1) {
       if (HasIndex[Idx]) HasRepeated = true;
       HasIndex[Idx] = true;
       compStr++;
     }
   }
CheckExtVectorComponent(Sema &S, QualType baseType, ExprValueKind &VK,
   // operates on.
   if (!HalvingSwizzle) {
     compStr = CompName->getNameStart();
 
     if (HexSwizzle)
       compStr++;
 
     while (*compStr) {
-      if (!vecType->isAccessorWithinNumElements(*compStr++)) {
+      if (!vecType->isAccessorWithinNumElements(*compStr++, HexSwizzle)) {
         S.Diag(OpLoc, diag::err_ext_vector_component_exceeds_length)
           << baseType << SourceRange(CompLoc);
         return QualType();
       }
     }
   }
 
   // The component accessor looks fine - now we need to compute the actual type.

cfe/trunk/test/CodeGen/ext-vector.c

 
 // CHECK: @test17
 void test17(void) {
   char16 valA;
   char valB;
   char valC;
   char16 destVal = valC ? valA : valB;
 }
+
+typedef __attribute__(( ext_vector_type(16) )) float float16;
+
+float16 vec16, vec16_2;
+
+// CHECK: @test_rgba
+void test_rgba() {
+  // CHECK: fadd <4 x float>
+  vec4_2 = vec4.abgr + vec4;
+
+  // CHECK: shufflevector {{.*}} <i32 0, i32 1>
+  vec2 = vec4.rg;
+  // CHECK: shufflevector {{.*}} <i32 2, i32 3>
+  vec2_2 = vec4.ba;
+  // CHECK: extractelement {{.*}} 2
+  f = vec4.b;
+  // CHECK: shufflevector {{.*}} <i32 2, i32 2, i32 2, i32 2>
+  vec4_2 = vec4_2.bbbb;
+
+  // CHECK: insertelement {{.*}} 0
+  vec2.r = f;
+  // CHECK: shufflevector {{.*}} <i32 1, i32 0>
+  vec2.gr = vec2;
+
+  // CHECK: extractelement {{.*}} 0
+  f = vec4_2.rg.r;
+  // CHECK: shufflevector {{.*}} <i32 2, i32 1, i32 0>
+  // CHECK: shufflevector {{.*}} <i32 0, i32 1, i32 2, i32 undef>
+  // CHECK: shufflevector {{.*}} <i32 4, i32 5, i32 6, i32 3>
+  vec4.rgb = vec4.bgr;
+
+  // CHECK: extractelement {{.*}} 11
+  // CHECK: insertelement {{.*}} 2
+  vec4.b = vec16.sb;
+  // CHECK: shufflevector {{.*}} <i32 10, i32 11, i32 12, i32 13>
+  vec4_2 = vec16.sabcd;
+}

cfe/trunk/test/Sema/ext_vector_components.c

static void test() {
     vec2.yx = vec2_2.xy;
     vec4 = (float4){ 1,2,3,4 };
     vec4.xy.w; // expected-error {{vector component access exceeds type 'float2'}}
     vec4.s06; // expected-error {{vector component access exceeds type 'float4'}}
     vec4.x = vec16.sf;
     vec4.x = vec16.sF;
   
     vec4p->yz = vec4p->xy;
+
+    vec2.a; // expected-error {{vector component access exceeds type 'float2'}}
+    vec2.rgba; // expected-error {{vector component access exceeds type 'float2'}}
+    vec4.rgba; // expected-warning {{expression result unused}}
+    vec4.rgbz; // expected-error {{illegal vector component name 'z'}}
+    vec4.rgbc; // expected-error {{illegal vector component name 'c'}}
+    vec4.xyzr; // expected-error {{illegal vector component name 'r'}}
+    vec4.s01b; // expected-error {{vector component access exceeds type 'float4'}}
+
+    vec3 = vec4.rgb; // legal, shorten
+    f = vec2.r; // legal, shorten
+    f = vec4.rg.r; // legal, shorten
+    vec4_2.rgba = vec4.xyzw; // legal, no intermingling
+
+    vec4_2.rgbr = vec4.rgba; // expected-error {{vector is not assignable (contains duplicate components)}}
+    vec4_2.rgbb = vec4.rgba; // expected-error {{vector is not assignable (contains duplicate components)}}
+    vec4_2.rgga = vec4.rgba; // expected-error {{vector is not assignable (contains duplicate components)}}
+    vec2.x = f;
+    vec2.rr = vec2_2.rg; // expected-error {{vector is not assignable (contains duplicate components)}}
+    vec2.gr = vec2_2.rg;
+    vec2.gr.g = vec2_2.r;
+    vec4 = (float4){ 1,2,3,4 };
+    vec4.rg.b; // expected-error {{vector component access exceeds type 'float2'}}
+    vec4.r = vec16.sf;
+    vec4.g = vec16.sF;
+
+    vec4p->gb = vec4p->rg;
 }
 
 float2 lo(float3 x) { return x.lo; }
 float2 hi(float3 x) { return x.hi; }
 float2 ev(float3 x) { return x.even; }
 float2 od(float3 x) { return x.odd; }

cfe/trunk/test/SemaOpenCL/ext_vectors.cl

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.1
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
+
+typedef float float4 __attribute__((ext_vector_type(4)));
+
+void test_ext_vector_accessors(float4 V) {
+  V = V.wzyx;
+  V = V.abgr; // expected-warning {{vector component name 'a' is an OpenCL version 2.2 feature}}
+  V = V.xyzr; // expected-warning {{vector component name 'r' is an OpenCL version 2.2 feature}} \
+              // expected-error {{illegal vector component name 'r'}}
+}