This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Vector shifting patch
On Tue, Jun 15, 2010 at 6:33 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Jun 15, 2010 at 8:17 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> This is a reworked Andrew Pinski's vector shifting patch. This is done
>> in terms of GSoC 2010 project.
>>
>> The C part is separated from C++ part and is submitted as a separate patch.
>> OpenCL standard allows vector <op> scalar, scalar <op> vector
>> operations, for op = +,-,*,/,%. In that case scalar is converted to
>> the vector. Cannot see any reason why we would not want to support
>> these type of operations for shifting. So this is added to the patch
>> as well.
>
> You add a test to test for vector of floats that should be rejected.
>
> + ? ? ? ? ? ? ? ?sc = convert (TREE_TYPE (type1), sc);
> + ? ? ? ? ? ? ? ?op0 = build_vector_from_val (sc, type1);
>
> Why don't you push the convert down into the build_vector_from_val.
I asked him not to, convert shouldn't be called from generic
tree routines (I'd like to use build_vector_from_val from the
vectorizer as well).
> + ?gcc_assert (sc == error_mark_node
> + ? ? ? ? ? ? ?|| TREE_TYPE (sc) == TREE_TYPE (vectype));
>
> I think there could be a case where sc is error_mark_node which case
> you should just return error_mark_node..
> An example is:
> #define vector __attribute__((vector_size(4*sizeof(int) )))
> void g(void)
> {
> ?vector int t;
> ?t <<= d; /* { dg-error "undeclared" } ?*/
> }
> --- CUT ---
True, that's a good suggestion.
> How does your patch handle the case where <<= is used and the left
> hand side is an integer type like:
>
> #define vector __attribute__((vector_size(4*sizeof(int) )))
> void g(void)
> {
> ?vector int t;
> ?int d;
> ?d <<= t; /* { dg-error "" } ?*/
> }
>
> --- CUT ---
> +typedef int v4si __attribute__ ((vector_size (16)));
> I think that would be better if you do this:
> +typedef int v4si __attribute__ ((vector_size (4*sizeof(int) )));
>
> So that it is really v4 and I would name it v4int rather than saying
> the internal GCC mode.
>
> Some more from Joseph S. Myers' review of my patch which is why it was
> not applied yet:
>
> Do I understand correctly that it is also intended that the vectors must
> have the same width and number of elements (but possibly different
> signedness)?
I think we can allow arbitrary integer types as long as the vectors
have the same number of elements. At least if the Cell or OpenCL
spec do not put other restrictions on the element types.
> If so, the testcases should cover this (errors for different width, errors
> for different number of elements, allowing different signedness, errors if
> either vector is floating point).
That would be indeed nice to have.
> The tests should also verify that signed shift operations where the sign
> bit is involved act as documented in implement-c.texi for individual
> elements, and should test unsigned shift operations where the result
> differs from that of signed shift (right shifting values with the top bit
> set).
It might be the case that if the operation maps to hardware instructions
the vector hardware behaves differently, so I suppose implement-c.texi
should be adjusted accordingly (or, as those operations are not
standard C, the vector extension documentation should be amended
that the implementation might not follow implement-c.texi?)
> ?It would also be good to run tests for different base types and
> vector lengths (after all, the conversion to scalar code could generate
> things such as shifts on types smaller than int that wouldn't arise
> directly in normal C code, so it may not be obvious that it's well-tested
> whether such operations on char or short work correctly at present,
> although certainly valid GIMPLE), and to test shift count vectors that do
> not have all counts equal.
More exhaustive testsuite coverage would be indeed nice.
Richard.
> --- CUT ---
>
> Thanks,
> Andrew Pinski
>