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

[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/7850b54d/attachment.html>