<div dir="ltr">On Fri, Aug 30, 2013 at 4:19 AM, JinGu Kang <span dir="ltr"><<a href="mailto:jingu@codeplay.com" target="_blank">jingu@codeplay.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    Hi Eli,<br>
    <br>
    I usually appreciate your comment. <br><div class="im">
    <br>
    >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).<br>
    <br></div>
    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.<br>
    <br>
    Source Code: <a href="http://test.cl" target="_blank">test.cl</a><br>
    typedef char char16 __attribute__((ext_vector_type(16)));<br>
    <br>
    __kernel void test(void) {<br>
      char valA;<br>
      char valB;<br>
      char16 valC;<div class="im"><br>
      char16 destVal = valC ? valA : valB;<br>
    }<br>
    <br></div>
    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?</div></blockquote><div><br></div><div>If you feel like you don't know how to fix <div bgcolor="#FFFFFF">checkConditionalConvertScalarsToVectors, feel free to leave it as-is.<br>
</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div class="im"><br>
    <br>
    >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.<br>
    <br></div>
    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. :)<br><br></div></blockquote><div><br></div><div>I was referring specifically to places which update <span class="">LHS and RHS without updating RHSTy and LHSTy.  Taking another look, the only offender is actually</span><span class=""> <span class=""></span><span class="">checkConditionalConvertScalarsToVectors</span><span class="">, so feel free to ignore that.<br>
<br><br></span></span></div><div><span class=""><span class="">One more review comment I forgot to note the first time around: the patch is missing a testcase.<br><br></span></span></div><div><span class=""><span class="">-Eli<br>
</span></span></div></div></div></div>