[patch] Improvement to implicit scalar-vector conversions

[Pekka Jääskeläinen]

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?

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

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