<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 12/16/2014 10:26 PM, Colin Riddell
wrote:<br>
</div>
<blockquote class=" cite" id="mid_54906458_30903_codeplay_com"
cite="mid:54906458.30903@codeplay.com" type="cite">
<br>
<blockquote class=" cite" id="Cite_4519466" type="cite"> ... or
there is a conversion in section 6.2.1 Implicit Conversions that
<br>
can be applied to one of the expressions to make their types
match, ...
<br>
<br>
Which expression should be implicitly converted? As far as I can
make out, there was no need to mention Section 6.2.1 at this
point in the spec itself, especially because Section 6.2.6 is
meant to clarify exactly this choice: "The purpose is to
determine a common real type for the operands and result.". This
section cites the C99 spec when both operands are scalars:
"Otherwise, if all operands are scalar, the usual arithmetic
conversions apply, per section 6.3.1.8 of the C99 standard."
<br>
</blockquote>
Either exp2 or exp3 should be converted - which does mean they
need to be evaluated. So as far as I can see, we are on the same
understanding.
<br>
</blockquote>
<br>
More about this in the comments to the patch inlined below. I have
also attached an updated version of my writeup. The intention is to
document all the details of the behaviour (to be) implemented in
Clang and not just type conversions. This update is mostly a cleanup
with some enhancements to the table of examples in the end. Now
every negative example points out the reason for the error.<br>
<br>
<blockquote class=" cite" id="mid_54906458_30903_codeplay_com"
cite="mid:54906458.30903@codeplay.com" type="cite">
My latest patch provides the following:
<br>
<br>
* Tests(there are 15 in total, now):
<br>
</blockquote>
<br>
I have attached the tests that I have been working on. These
correspond to the table of examples in the writeup. There might be
overlap with your tests, but I decided to dump whatever I had so you
can take a look too. I have not checked the negative tests with
pristine Clang, but the following fail with your patch: 02, 03, 04,
05, 08, 09 and 10. But first we have to agree that the tests are
valid!<br>
<br>
<blockquote class=" cite" id="mid_54906458_30903_codeplay_com"
cite="mid:54906458.30903@codeplay.com" type="cite">* Code in
SemaExpr::CheckConditionalOperands():
<br>
</blockquote>
<br>
Comments inlined below.<br>
<br>
> diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp<br>
> index 04497f3..e37e0c5 100644<br>
> --- a/lib/Sema/SemaExpr.cpp<br>
> +++ b/lib/Sema/SemaExpr.cpp<br>
> @@ -5817,13 +5817,27 @@ QualType
Sema::CheckConditionalOperands(ExprResult &Cond, ExprResult
&LHS,<br>
> QualType LHSTy = LHS.get()->getType();<br>
> QualType RHSTy = RHS.get()->getType();<br>
> <br>
> - // If the condition is a vector, and both operands are
scalar,<br>
> - // attempt to implicity convert them to the vector type to
act like the<br>
> - // built in select. (OpenCL v1.1 s6.3.i)<br>
> - if (getLangOpts().OpenCL &&
CondTy->isVectorType())<br>
> - if (checkConditionalConvertScalarsToVectors(*this, LHS,
RHS, CondTy))<br>
> - return QualType();<br>
> - <br>
> + if (getLangOpts().OpenCL) {<br>
> + if (CondTy->isVectorType()){<br>
> + if
(CondTy->getAs<VectorType>()->getElementType()->isFloatingType())<br>
<br>
This needs to be checked for the scalar operator too.<br>
<br>
> + Diag(Cond.get()->getLocStart(),
diag::err_use_of_float_ternary_cond)<br>
> + << CondTy;<br>
> + if (LHSTy != RHSTy){<br>
> + if (LHSTy->isVectorType() &&
RHSTy->isScalarType())<br>
> + RHS = ImpCastExprToType(RHS.get(), LHSTy,
CK_IntegralCast);<br>
> + else if (LHSTy->isScalarType() &&
RHSTy->isVectorType())<br>
> + LHS = ImpCastExprToType(LHS.get(), RHSTy,
CK_IntegralCast);<br>
> + }<br>
<br>
This should be moved to the function
"Sema::UsualArithmeticConversions" in the same file. That function
implements C99, but needs to be enhanced for Section 6.2.6 from
OpenCL 1.2 spec, which provides additional conversion rules for
vector operands. Once we have that, we don't need to worry about LHS
and RHS; instead, we just use ResTy which is computed just before
the current point in the code. <br>
<br>
> + else if (LHSTy->isScalarType() &&
RHSTy->isScalarType()){<br>
<br>
If we make the above fix, then we only need to check if ResTy is
scalar at this point.<br>
<br>
> + // Now attempt to implicity convert them to the vector
type to act like the<br>
> + // built in select. (OpenCL v1.1 s6.3.i)<br>
> + if (checkConditionalConvertScalarsToVectors(*this,
LHS, RHS, CondTy))<br>
<br>
This is still casting the element type of the result to the element
type of the condition, which is exactly what the original problem
was! It should do the following instead:<br>
<ol>
<li>Check that the bit-size of scalar ResTy is the same as the
bit-size of the element type in CondTy</li>
<li>Expand ResTy to match the vector width of CondTy.</li>
</ol>
> + return QualType();<br>
> + return CondTy;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
<br>
Wondering if control should ever reach beyond this point for OpenCL!<br>
<br>
> // If both operands have arithmetic type, do the usual
arithmetic conversions<br>
> // to find a common type: C99 6.5.15p3,5.<br>
> if (LHSTy->isArithmeticType() &&
RHSTy->isArithmeticType()) {<br>
> <br>
<br>
Sameer.<br>
</body>
</html>