[cfe-dev] Question about conditional operator with vector type

[jingu]

Hi Eli,

Thank you so much for your good reviewing and comment. I think It's not 
good way to insert "ImplicitCastExpr of LvalueToRValue" on 
checkConditionalConvertScalarsToVectors()" and I have no other idea to 
fix it. I will commit the patch with a test case like attached patch.

Thanks for your good guide again,
JinGu Kang

2013-09-03 ?? 2:28, Eli Friedman ? ?:
> On Fri, Aug 30, 2013 at 4:19 AM, JinGu Kang <jingu at codeplay.com 
> <mailto:jingu at codeplay.com>> wrote:
>
>     Hi Eli,
>
>     I usually appreciate your comment.
>
>     >Shouldn't the UsualArithmeticConversions call come after the
>     checkConditionalConvertScalarsToVectors check? Not completely sure
>     here (the rules aren't really clear, and clang's implementation of
>     the check in question isn't consistent with any interpretation of
>     the OpenCL standard).
>
>     I also agree your opinion but it causes assertion "assert(0 &&
>     "can't implicitly cast lvalue to rvalue with this cast kind");"
>     with following OpenCL example.
>
>     Source Code: test.cl <http://test.cl>
>     typedef char char16 __attribute__((ext_vector_type(16)));
>
>     __kernel void test(void) {
>       char valA;
>       char valB;
>       char16 valC;
>
>       char16 destVal = valC ? valA : valB;
>     }
>
>     This assertion comes from "ImpCastExprToType()" function in
>     "checkConditionalConvertScalarsToVectors()" function because LHS
>     and RHS are LValue at that moment without
>     "UsualArithmeticConversions()".  In order to move
>     "UsualArithmeticConversions()" after
>     "checkConditionalConvertScalarsToVectors ()",
>     "checkConditionalConvertScalarsToVectors ()" needs
>     "DefaultLvalueConversion()" to make 'LValueToRValue'
>     ImplicitCastExpr with LHS and RHS. I think that
>     "DefaultLvalueConversion()" is redundant because
>     "UsualArithmeticConversions()" also executes
>     "DefaultLvalueConversion()".  How do you feel about this?
>
>
> If you feel like you don't know how to fix
> checkConditionalConvertScalarsToVectors, feel free to leave it as-is.
>
>
>
>     >If you're going to go the route of updating CondTy, LHSTy, and
>     RHSTy whenever the expressions change, please make sure they stay
>     consistently up-to-date throughout the function.
>
>     I am so sorry that I can not understand your comment completely
>     due to  my idiot brain. I simply modified patch to use "getType()"
>     function. If there are something wrong, please guide me to correct
>     direction. :)
>
>
> I was referring specifically to places which update LHS and RHS 
> without updating RHSTy and LHSTy. Taking another look, the only 
> offender is actuallycheckConditionalConvertScalarsToVectors, so feel 
> free to ignore that.
>
>
> One more review comment I forgot to note the first time around: the 
> patch is missing a testcase.
>
> -Eli

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130903/83413c4a/attachment.html>
-------------- next part --------------
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp	(revision 189771)
+++ lib/Sema/SemaExpr.cpp	(working copy)
@@ -5444,9 +5444,18 @@
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  // First, check the condition.
   Cond = UsualUnaryConversions(Cond.take());
   if (Cond.isInvalid())
     return QualType();
+  if (checkCondition(*this, Cond.get()))
+    return QualType();
+
+  // Now check the two expressions.
+  if (LHS.get()->getType()->isVectorType() ||
+      RHS.get()->getType()->isVectorType())
+    return CheckVectorOperands(LHS, RHS, QuestionLoc, /*isCompAssign*/false);
+
   UsualArithmeticConversions(LHS, RHS);
   if (LHS.isInvalid() || RHS.isInvalid())
     return QualType();
@@ -5455,14 +5464,6 @@
   QualType LHSTy = LHS.get()->getType();
   QualType RHSTy = RHS.get()->getType();
 
-  // first, check the condition.
-  if (checkCondition(*this, Cond.get()))
-    return QualType();
-
-  // Now check the two expressions.
-  if (LHSTy->isVectorType() || RHSTy->isVectorType())
-    return CheckVectorOperands(LHS, RHS, QuestionLoc, /*isCompAssign*/false);
-
   // 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. (OpenCL v1.1 s6.3.i)
Index: test/CodeGen/ext-vector.c
===================================================================
--- test/CodeGen/ext-vector.c	(revision 189771)
+++ test/CodeGen/ext-vector.c	(working copy)
@@ -291,3 +291,13 @@
 void test16(float2 a, float2 b) {
   float2 t0 = (a + b) / 2;
 } 
+
+typedef char char16 __attribute__((ext_vector_type(16)));
+
+// CHECK: @test17
+void test17(void) {
+  char16 valA;
+  char valB;
+  char valC;
+  char16 destVal = valC ? valA : valB;
+}