[patch] Improvement to implicit scalar-vector conversions

[Stephen Canon]

On Apr 2, 2014, at 12:48 PM, Pekka Jääskeläinen <pekka.jaaskelainen at tut.fi> wrote:

> OK,
> 
> Seems you are much more familiar with this issue than I am.
> 
> Could it be possible with not too much effort to provide
> a frontend option to control the behavior so we can adhere
> to the (ill-considered or not) OpenCL specs too, if one wants
> to be strict?

I expect so, though I have never implemented a front-end option, so I am by no means the expert.

> Perhaps it won't pay the effort if you are working towards improving
> this part in the specs.

I wouldn’t expect many (any?) users for such an option, but I also expect that changing the OpenCL spec will be a slow process, so it may have some value.

– Steve

> On 04/02/2014 06:28 PM, Stephen Canon wrote:
>> While it does move closer to the OpenCL behavior in some regards, this
>> patch does not strictly hew to the OpenCL conversion rules of section
>> 6.2.6.  Specifically, it does not adhere to:
>> 
>>> An error shall occur if any scalar operand has greater rank than the type
>>> of the vector element.
>> 
>> This is not an accident; it is the experience of every vector programmer I
>> surveyed that this rule was ill-considered (and , as an early contributor
>> to the OpenCL spec, I’m as responsible for this mistake as anyone; I wish
>> we had detected it back then).  I will work on getting this changed in
>> OpenCL, but that will be a lengthy process, and we should relax it for the
>> vector extensions in C/ObjC/C++, rather than blindly follow after OpenCL’s
>> error.
>> 
>> The best example of why the OpenCL rule is a bad idea is that it makes
>> working with vectors integer types smaller than int extremely painful.
>> Here’s a simple example:  ushort8 foo(ushort8 x, ushort8 y) { return (x + y
>> + 1) >> 1; }
>> 
>> This seems perfectly reasonable, and naively one would like it to work, but
>> it doesn’t:
>> 
>> error: can't convert between vector values of different size ('short8' and
>> 'int')
>> 
>> Instead one must write:
>> 
>> ushort8 foo(ushort8 x, ushort8 y) { return (x + y + (unsigned short)1) >>
>> (unsigned short)1; }
>> 
>> These extraneous casts do not help readability (the intended meaning is
>> unambiguous in the original), and do not help maintain correctness.  They
>> are noise and they are frustrating to the programmer.
>> 
>> With this patch, the example above works as written, but informative
>> warnings are also provided when the conversion would change the value, to
>> help protect new vector programmers who are only familiar with the C scalar
>> promotion rules:
>> 
>> uchar16 bar(uchar16 x) { return x*257 >> 3; } warning: implicit conversion
>> from 'int' to 'uchar16' changes value from 257 to 1
>> [-Wconstant-conversion]
>> 
>> float4 bar(float4 x) { return x/M_PI; } warning: implicit conversion loses
>> floating-point precision: 'double' to 'float4' [-Wconversion]
>> 
>> I realize that the merit of this change is perhaps non-obvious to those who
>> don’t have a lot of experience writing vector code, but it relieves a major
>> pain point that essentially everyone who has ever tried to seriously use
>> ext_vector_type has experienced and complained about.  I am recommending
>> that we choose to be slightly more permissive than OpenCL is to relieve
>> this pain, while providing clear warnings to mitigate the risk of that
>> permissiveness.
>> 
>> – Steve
>> 
>> On Apr 2, 2014, at 4:58 AM, Pekka Jääskeläinen <pekka.jaaskelainen at tut.fi>
>> wrote:
>> 
>>> Hi,
>>> 
>>> Should we try to comply to the OpenCL C specified behavior here or is
>>> there some other relevant standard to look at in this case?
>>> 
>>> This piece from the OpenCL 1.2 specs is relevant here:
>>> 
>>> "6.2.6 Usual Arithmetic Conversions <snip>
>>> 
>>> If I understood it correctly, your patch goes towards the OpenCL C
>>> specified behavior. It LGTM.
>>> 
>>> BR, Pekka
>>> 
>>> On 03/27/2014 02:34 AM, Stephen Canon wrote:
>>>> ExtVectors currently support basic operations with scalar data (which
>>>> is interpreted as an implicit splat).  However, this support has some
>>>> serious issues.  Most critically, at present the type of the result
>>>> depends on operand order:
>>>> 
>>>> typedef float __attribute__((ext_vector_type(2))) float2;
>>>> 
>>>> float2 x; double y = 2.0 + x; // reinterprets y as double, scalar
>>>> double-precision add. float2 z = x + 2.0; // reinterprets x as float2,
>>>> does packed single-precision add.
>>>> 
>>>> Both behaviors are pretty busted; the odds are overwhelming that the
>>>> programmer's intention was to add two to both lanes of x.  What’s
>>>> worse, +, which is a commutative operator for any reasonable FP type,
>>>> doesn’t even return the same type when the operand order is flipped.
>>>> 
>>>> This patch makes it so that “real scalar OP vector” is interpreted as
>>>> “convert the scalar to vector element type and splat, then perform
>>>> OP”, regardless of operand order or conversion rank of the scalar and
>>>> vector type (i.e. the type of the vector elements always “wins”, even
>>>> if the rank of the scalar type is greater).  This is somewhat different
>>>> from the arithmetic promotions for scalar types, but it is by far the
>>>> most sensible behavior; it is what most vector programmers want to
>>>> get.
>>>> 
>>>> This also improves the state of affairs for integer scalars in
>>>> ExtVector expressions.  When operating on vectors with elements smaller
>>>> than int, it has until now been necessary to sprinkle in lots of
>>>> casts:
>>>> 
>>>> typedef unsigned char __attribute__((__ext_vector_type__(16)))
>>>> uchar16; uchar16 baz(uchar16 x) { return x + (unsigned char)2; }
>>>> 
>>>> The extra cast adds little to nothing, and makes simple expressions
>>>> overly verbose.  With this patch, the following works just fine:
>>>> 
>>>> uchar16 baz(uchar16 x) { return x + 2; }
>>>> 
>>>> I also improved the state of warnings for implicit scalar->vector casts
>>>> to make it easier to identify suspicious conversions:
>>>> 
>>>> short4 bar( ) { return 65536; }
>>>> 
>>>> previously this produced no error or warning.  Now, with -Wconversion
>>>> we get:
>>>> 
>>>> foo.c:9:12: warning: implicit conversion from 'int' to 'short4'
>>>> changes value from 65536 to 0 [-Wconstant-conversion]
>>>> 
>>>> Thanks in advance for your feedback! – Steve
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________ cfe-commits mailing
>>>> list cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
> 
> 
> -- 
> Pekka