[cfe-commits] [PATCH][Review place] A Patch to Enable Vector Select for arm general computing

[Douglas Gregor]

On Sep 5, 2012, at 10:52 AM, Stephen Hines <srhines at google.com> wrote:

> Ping for review.

First, some specific comments on the patch as is, then a larger discussion below.

--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -4662,6 +4662,10 @@ static bool checkCondition(Sema &S, Expr *Cond) {
   if (S.getLangOpts().OpenCL && CondTy->isVectorType())
     return false;
 
+  // VectorSelect extension
+  if (S.getLangOpts().VectorSelect && CondTy->isVectorType())
+    return false;
+

Since OpenCL implies VectorSelector, the first 'if' statement could be removed.

In the conditional-checking code, there's support for promoting scalars to vector type for OpenCL, e.g.,

  // OpenCL: If the condition is a vector, and both operands are scalar,
  // attempt to implicity convert them to the vector type to act like the
  // built in select.
  if (getLangOpts().OpenCL && CondTy->isVectorType())
    if (checkConditionalConvertScalarsToVectors(*this, LHS, RHS, CondTy))
      return QualType();

I assume that should check VectorSelect rather than OpenCL.


diff --git a/test/Sema/vector-select.c b/test/Sema/vector-select.c
new file mode 100644
index 0000000..9ed8b3f
--- /dev/null
+++ b/test/Sema/vector-select.c
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -fsyntax-only -w -fvector-select -verify %s
+
+typedef float  float4 __attribute__((ext_vector_type(4)));
+typedef double double4 __attribute__((ext_vector_type(4)));
+typedef signed char   char4 __attribute__((ext_vector_type(4)));
+typedef short  short4 __attribute__((ext_vector_type(4)));
+typedef int    int4 __attribute__((ext_vector_type(4)));
+typedef long   long4 __attribute__((ext_vector_type(4)));
+typedef long long llong4 __attribute__((ext_vector_type(4)));
+typedef unsigned char  uchar4 __attribute__((ext_vector_type(4)));
+typedef unsigned short ushort4 __attribute__((ext_vector_type(4)));
+typedef unsigned int   uint4  __attribute__((ext_vector_type(4)));
+typedef unsigned long  ulong4 __attribute__((ext_vector_type(4)));
+typedef unsigned long long ullong4 __attribute__((ext_vector_type(4)));
+
+#define TEST_FUNC_4 test
+
+#define DEFINE_TEST_FUNC_4(type) \
+extern type __attribute__((overloadable)) \
+TEST_FUNC_4(type a, type b, int selector) { \
+  type c; \
+  switch( selector ){ \
+    case 0: c = a >  b ? a : b; break; \
+    case 1: c = a >= b ? a : b; break; \
+    case 2: c = a != b ? a : b; break; \
+    case 3: c = a == b ? a : b; break; \
+    case 4: c = a <  b ? a : b; break; \
+    case 5: c = a <= b ? a : b; break; \
+    default: \
+      printf("ERROR: undefined selector\n"); \
+  } \
+  return c; \
+}
+
+#define VALIDATE(tester, result) \
+{ \
+  if( tester.x != result.x ) failed++; \
+  if( tester.y != result.y ) failed++; \
+  if( tester.z != result.z ) failed++; \
+  if( tester.w != result.w ) failed++; \
+}
+

This test is trying to do two things at once, and should be separated into two tests.

First, it's checking that semantic analysis accepts this new syntax. The -verify makes sure that no errors occur.

Second, it's trying to do run-time validation to make sure the generated code is correct (via VALIDATE), but that isn't actually getting exercised: -fsyntax-only prevents IR generation from happening, and the regression tests would not link and run the application anyway. The appropriate way to check for proper IR generation is to put a test in test/CodeGen that uses -emit-llvm and FileCheck to verify that the proper IR is generated.

Now, back to the patch itself. Rather than adding a new option -fvector-select, I'd rather replace the OpenCL checks with checks for extended vector types (ExtVectorType), since extended vector types are meant to implement OpenCL. Then we don't need another language-dialect-tweaking flag, and anyone using extended vector types, in any of our dialects, gets the same conditional-operator vector selection behavior as in OpenCL.

	- Doug

> 
> On Fri, Aug 31, 2012 at 10:27 AM, Stephen Hines <srhines at google.com> wrote:
> Hi Yin (and cfe-commits),
> 
> I reworked this patch so that it is not dependent on ARM. I added -fvector-select (and the corresponding LangOpts.VectorSelect) to enable this functionality. I also updated the test to work with non-ARM targets. Can CFE-commits take another look and submit if it is acceptable?
> 
> Thanks,
> Steve
> 
> 
> On Thu, Aug 30, 2012 at 1:34 PM, Yin Ma <yinma at codeaurora.org> wrote:
> Hi Stephen,
> 
>  
> 
>      The attached file are the latest diff file to enable vector select for ARM and
> 
> A test case that should be placed in test/Sema/ directory. The code has been
> 
> rebased on the latest clang source. Please give a review.
> 
>  
> 
> Thanks,
> 
>  
> 
>                             Yin
> 
>  
> 
>  
> 
> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Yin Ma
> Sent: Monday, August 13, 2012 5:55 PM
> To: cfe-commits at cs.uiuc.edu
> Subject: [cfe-commits] [PATCH][Review place] A Patch to Enable Vector Select for general computing
> 
>  
> 
> Hi,
> 
>  
> 
> So far vector select, such as  a < b ? a : c is  not enabled in clang for C/C++ code.
> 
> It only enabled for OpenCL. I have made a very small change in clang to enable
> 
> Vector select. I have tested for ARM.  Everything works fine so far.
> 
>    The file attached enables vector select for general computing. Please give
> 
> A review.
> 
>  
> 
> Thanks,
> 
>  
> 
>                Yin
> 
>  
> 
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120910/a4ca990b/attachment.html>