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

[Eli Friedman]

LGTM.

-Eli


On Mon, Sep 2, 2013 at 1:20 PM, jingu <jingu at codeplay.com> wrote:

>  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> 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
>> 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
> actually checkConditionalConvertScalarsToVectors, 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/20130902/e58f3608/attachment.html>