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

[JinGu Kang]

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

Thanks for your good comment again,
JinGu Kang


On 29/08/2013 22:37, Eli Friedman wrote:
> On Thu, Aug 29, 2013 at 10:13 AM, JinGu Kang <jingu at codeplay.com 
> <mailto:jingu at codeplay.com>> wrote:
>
>     Hi all,
>
>     My colleague 'Alistair' tested a simple code and found a error.
>     The simplified code and error message are as following:
>
>     Source code:
>     typedef char char16 __attribute__((ext_vector_type(16)));
>
>     int main(void) {
>       char16 valA;
>       char valB;
>       char valC;
>       char16 destVal = valC ? valA : valB;
>     }
>
>     Error message:
>     error: can't convert between vector values of different size
>     ('char16' and 'int')
>       char16 destVal = valC ? valA : valB;
>                                             ^ ~~~~   ~~~~
>
>     This error message comes from "CheckVectorOperands()" function in
>     "CheckConditionalOperands()" function because
>     "UsualArithmeticConversions(LHS, RHS)" function is executed before
>     "CheckVectorOperands()" function and
>     "UsualArithmeticConversions(LHS, RHS)" function changes RHS as
>     follows:
>     GDB output of original RHS:
>     DeclRefExpr 0xc4b3050 'char' lvalue Var 0xc4b2f50 'valB' 'char'
>
>     GDB output of changed RHS:
>     ImplicitCastExpr 0xc4b30a8 'int' <IntegralCast>
>     `-ImplicitCastExpr 0xc4b3098 'char' <LValueToRValue>
>       `-DeclRefExpr 0xc4b3050 'char' lvalue Var 0xc4b2f50 'valB' 'char'
>
>     So, the error message is generated between 'char16' and 'int'. I
>     am wondering whether this error is correct or not. I have checked
>     other checking functions executes "CheckVectorOperands()" function
>     before "UsualArithmeticConversions(LHS, RHS)" function such as
>     "CheckSubtractionOperands()", "CheckAdditionOperands()" functions
>     and etc... Could someone let me know about this error? I have
>     attached a simple patch to fix it on the assumption that it is not
>     correct.
>
>
> The patch is in the right direction; the call 
> to UsualArithmeticConversions should come after the call to 
> CheckVectorOperands.
>
> 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).
>
> 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.
>
> -Eli

-- 
JinGu Kang
Codeplay Software Ltd
45 York Place, Edinburgh, EH1 3HP
Tel: 0131 466 0503
Fax: 0131 557 6600
Website: http://www.codeplay.com
Twitter: https://twitter.com/codeplaysoft

This email and any attachments may contain confidential and /or privileged information and is for use by the addressee only. If you are not the intended recipient, please notify Codeplay Software Ltd immediately and delete the message from your computer. You may not copy or forward it,or use or disclose its contents to any other person. Any views or other information in this message which do not relate to our business are not authorized by Codeplay software Ltd, nor does this message form part of any contract unless so stated.
As internet communications are capable of data corruption Codeplay Software Ltd does not accept any responsibility for any changes made to this message after it was sent. Please note that Codeplay Software Ltd does not accept any liability or responsibility for viruses and it is your responsibility to scan any attachments.
Company registered in England and Wales, number: 04567874
Registered office: 81 Linkfield Street, Redhill RH1 6BY

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130830/8b5e30e2/attachment.html>
-------------- next part --------------
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp	(revision 189664)
+++ 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)