[cfe-dev] [PATCH] [OpenCL] Conversions for ternary operations between scalar and vector

[Sahasrabuddhe, Sameer]

On 12/11/2014 5:47 PM, Colin Riddell wrote:
> Yes, this is a good exercise for me as well. It's really forcing me to 
> question my understanding, rather than just hacking something together 
> to make it work. I agree we need more tests, so I will work on a 
> larger set of both positive and negative tests once we have solidified 
> our understanding.

Good to know that. It definitely affects AMD's frontend and I am 
guessing that it also affects the Khronos SPIR generator.

> I reviewed your document and moved it around slightly to fit with my 
> understanding of the spec. In summary, my changes are:

Thanks for looking into it! I have replied below by copying relevant 
parts from the file.

>   * Select rules about bits can be simplified down to something very 
> succinct like: 'use select when exp1 result is a vector. When using 
> select, exp2 and exp3 must match.' (and conversion rules apply to make 
> them match detailed in 4.2). This is because, beyond checking these 
> types, at this stage in the compilation we have no other control.. 
> which is what the checkConditionalConvertScalarsToVectors() function 
> is attempting to do (though this may be incorrect).
>
> I think these changes are more around the order and organization of 
> the rules, rather than the actual rules themselves and think we are 
> starting to come to agreement on them. Let me know what you think of 
> my changes to your document..

Sure, but one motivation for writing this document was to identify gray 
areas, where Clang is following conventions in the absence of rules. See 
below regarding point (6) from my original writeup.

Pasting some of the lines from your attached file:

>       1. exp2 and exp3 can be any type, as long as they match
>
>       2. If exp2 and exp3 don't match, then one of either exp2 or exp3
>          can be converted using the arithmetic conversion rules for 
> vectors
>          ("both" rules, in your wording):
>              1. If one is a vector and the other a scalar, the scalar
>                 may be converted to the base type, and expanded to the 
> vec
>                 type.
>              2. If both are vectors of the same type and size, no implicit
>                 conversion is required.
>              3. If both are vectors of different type or size, no implicit
>                 conversion is ALLOWED
>       3. For *scalars*, the result type is obtained by implicit
>          conversion on exp2 *or* exp3. (section 6.2.1 of CL Spec). Which
>          comes under the "either" rules, by your wording.

We don't actually need to say all this. It's enough to just cite Section 
6.2.6. This was my intention in point (5):

> -5. The result type is obtained by usual arithmetic conversions on exp2
> -   and exp3, as specified in Section 6.2.6.

Aside: Does the minus (-) sign mean that you think this should be deleted?

Also, I think that Section 6.2.1 is not really necessary in this 
situation, the way it occurs on page 220 of the spec:

     ... or there is a conversion in section 6.2.1 Implicit Conversions that
     can be applied to one of the expressions to make their types match, ...

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."

> (I suppose, technically they are both evaulated, but one converted to the
> other. Covered in 4)
> -   2. Both exp2 and exp3 are evaluated.

Yeah, if we have already said that the vector operator behaves like the 
select function call, then it implies that both are evaluated. But I 
thought it would be good to make this an explicit point because it was 
the first thing that tripped me up when I first encountered the ternary 
operator long ago! :)

> - 6. If exp1 is a vector, and the result type is a scalar, then

This is an important point; it covers exactly the part that caused the 
original bug. By "result type", I meant the one obtained by usual 
arithmetic conversions on LHS and RHS. It is the same as ResTy in the 
code. If we had stopped at saying that the ternary operator behaves like 
the select function, then it would be illegal to have an expression like 
"int2 ? float : float". The vector expansion required is not specified 
anywhere --- we want to promote ResTy (scalar float) to a vector that 
matches the length of CondTy, but we do not want to change its element 
type.

 1. CondTy is vector, RHS is scalar and LHS is scalar.
 2. Current behaviour: Use usual arithmetic conversions to cast both RHS
    and LHS to CondTy. Very buggy because it performs illegal
    conversions from scalar to vector.
 3. Expected behaviour: Use usual arithmetic conversions to determine
    ResTy from RHS and LHS, then expand it to match CondTy in length.
    Actually, only one of RHS and LHS will be casted.

Going by the letter of the spec, it is difficult to say whether the 
situation itself is illegal or undefined. Note that a similar situation 
with the select builtin will result in a signature mismatch. Clang 
currently implements the first version, but at least you and I agree 
that the second version is more useful. There is no reason to say that 
one behaviour is correct and the other is not.

Sameer.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141212/b7af60c4/attachment.html>