Patchwork Vector Comparison patch

login
register
mail settings
Submitter Richard Guenther
Date Sept. 28, 2011, 2:23 p.m.
Message ID <CAFiYyc2gUuHWWriYD2qjMETdZcXmytqho6ZbtkTYOfxipuZVAA@mail.gmail.com>
Download mbox | patch
Permalink /patch/116795/
State New
Headers show

Comments

Richard Guenther - Sept. 28, 2011, 2:23 p.m.
On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>> This looks like it has the same issue with maybe needing to use
>>> TYPE_MAIN_VARIANT in type comparisons as the shuffle patch.
>>
>> I don't think so, we move qualifiers to the vector type from the element type
>> in make_vector_type and the tests only look at the component type.
>>
>> I am re-testing the patch currently and will commit it if that succeeds.
>
> Unfortunately gcc.c-torture/execute/vector-compare-1.c fails with -m32
> for
>
>    vector (2, double) d0;
>    vector (2, double) d1;
>    vector (2, long) idres;
>
>    d0 = (vector (2, double)){(double)argc,  10.};
>    d1 = (vector (2, double)){0., (double)-23};
>    idres = (d0 > d1);
>
> as appearantly the type we chose to assign to (d0 > d1) is different
> from that of idres:
>
> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
> error: incompatible types when assigning to type '__vector(2) long
> int' from type '__vector(2) long long int'^M
>
> Adjusting it to vector (2, long long) otoh yields, for -m64:
>
> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
> error: incompatible types when assigning to type '__vector(2) long
> long int' from type '__vector(2) long int'
>
> But those two types are at least compatible from their modes.  Joseph,
> should we accept mode-compatible types in assignments or maybe
> transparently convert them?

Looks like we have a more suitable solution for these automatically
generated vector types - mark them with TYPE_VECTOR_OPAQUE.

I'm testing the following incremental patch.

Richard.

         }
@@ -10063,8 +10065,10 @@ build_binary_op (location_t location, en
             }

           /* Always construct signed integer vector type.  */
-          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
(type0)), 0);
-          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
+          intt = c_common_type_for_size (GET_MODE_BITSIZE
+                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
+          result_type = build_opaque_vector_type (intt,
+                                                 TYPE_VECTOR_SUBPARTS (type0));
           converted = 1;
           break;
         }
Richard Guenther - Sept. 29, 2011, 10 a.m.
On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>>> This looks like it has the same issue with maybe needing to use
>>>> TYPE_MAIN_VARIANT in type comparisons as the shuffle patch.
>>>
>>> I don't think so, we move qualifiers to the vector type from the element type
>>> in make_vector_type and the tests only look at the component type.
>>>
>>> I am re-testing the patch currently and will commit it if that succeeds.
>>
>> Unfortunately gcc.c-torture/execute/vector-compare-1.c fails with -m32
>> for
>>
>>    vector (2, double) d0;
>>    vector (2, double) d1;
>>    vector (2, long) idres;
>>
>>    d0 = (vector (2, double)){(double)argc,  10.};
>>    d1 = (vector (2, double)){0., (double)-23};
>>    idres = (d0 > d1);
>>
>> as appearantly the type we chose to assign to (d0 > d1) is different
>> from that of idres:
>>
>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>> error: incompatible types when assigning to type '__vector(2) long
>> int' from type '__vector(2) long long int'^M
>>
>> Adjusting it to vector (2, long long) otoh yields, for -m64:
>>
>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>> error: incompatible types when assigning to type '__vector(2) long
>> long int' from type '__vector(2) long int'
>>
>> But those two types are at least compatible from their modes.  Joseph,
>> should we accept mode-compatible types in assignments or maybe
>> transparently convert them?
>
> Looks like we have a more suitable solution for these automatically
> generated vector types - mark them with TYPE_VECTOR_OPAQUE.
>
> I'm testing the following incremental patch.
>
> Richard.
>
> Index: gcc/c-typeck.c
> ===================================================================
> --- gcc/c-typeck.c.orig 2011-09-28 16:22:10.000000000 +0200
> +++ gcc/c-typeck.c      2011-09-28 16:18:39.000000000 +0200
> @@ -9928,8 +9928,10 @@ build_binary_op (location_t location, en
>             }
>
>           /* Always construct signed integer vector type.  */
> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
> (type0)), 0);
> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
> +          result_type = build_opaque_vector_type (intt,
> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>           converted = 1;
>           break;
>         }
> @@ -10063,8 +10065,10 @@ build_binary_op (location_t location, en
>             }
>
>           /* Always construct signed integer vector type.  */
> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
> (type0)), 0);
> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
> +          result_type = build_opaque_vector_type (intt,
> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>           converted = 1;
>           break;
>         }

That doesn't seem to work either.  Because we treat the opaque and
non-opaque variants of vector<int> as different (the opaque type isn't
a variant type of the non-opaque one - something suspicious anyway).

I'm going to try to apply some surgery on how we build opaque variants
and then re-visit the above again.

Richard.
Richard Guenther - Sept. 29, 2011, 11:27 a.m.
On Thu, Sep 29, 2011 at 12:00 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>>>> This looks like it has the same issue with maybe needing to use
>>>>> TYPE_MAIN_VARIANT in type comparisons as the shuffle patch.
>>>>
>>>> I don't think so, we move qualifiers to the vector type from the element type
>>>> in make_vector_type and the tests only look at the component type.
>>>>
>>>> I am re-testing the patch currently and will commit it if that succeeds.
>>>
>>> Unfortunately gcc.c-torture/execute/vector-compare-1.c fails with -m32
>>> for
>>>
>>>    vector (2, double) d0;
>>>    vector (2, double) d1;
>>>    vector (2, long) idres;
>>>
>>>    d0 = (vector (2, double)){(double)argc,  10.};
>>>    d1 = (vector (2, double)){0., (double)-23};
>>>    idres = (d0 > d1);
>>>
>>> as appearantly the type we chose to assign to (d0 > d1) is different
>>> from that of idres:
>>>
>>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>>> error: incompatible types when assigning to type '__vector(2) long
>>> int' from type '__vector(2) long long int'^M
>>>
>>> Adjusting it to vector (2, long long) otoh yields, for -m64:
>>>
>>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>>> error: incompatible types when assigning to type '__vector(2) long
>>> long int' from type '__vector(2) long int'
>>>
>>> But those two types are at least compatible from their modes.  Joseph,
>>> should we accept mode-compatible types in assignments or maybe
>>> transparently convert them?
>>
>> Looks like we have a more suitable solution for these automatically
>> generated vector types - mark them with TYPE_VECTOR_OPAQUE.
>>
>> I'm testing the following incremental patch.
>>
>> Richard.
>>
>> Index: gcc/c-typeck.c
>> ===================================================================
>> --- gcc/c-typeck.c.orig 2011-09-28 16:22:10.000000000 +0200
>> +++ gcc/c-typeck.c      2011-09-28 16:18:39.000000000 +0200
>> @@ -9928,8 +9928,10 @@ build_binary_op (location_t location, en
>>             }
>>
>>           /* Always construct signed integer vector type.  */
>> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
>> (type0)), 0);
>> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
>> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
>> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
>> +          result_type = build_opaque_vector_type (intt,
>> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>>           converted = 1;
>>           break;
>>         }
>> @@ -10063,8 +10065,10 @@ build_binary_op (location_t location, en
>>             }
>>
>>           /* Always construct signed integer vector type.  */
>> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
>> (type0)), 0);
>> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
>> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
>> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
>> +          result_type = build_opaque_vector_type (intt,
>> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>>           converted = 1;
>>           break;
>>         }
>
> That doesn't seem to work either.  Because we treat the opaque and
> non-opaque variants of vector<int> as different (the opaque type isn't
> a variant type of the non-opaque one - something suspicious anyway).
>
> I'm going to try to apply some surgery on how we build opaque variants
> and then re-visit the above again.

Bootstrapped and tested on x86_64-unknown-linux-gnu and installed.

Richard.

> Richard.
>
Matthew Gretton-Dann - Sept. 30, 2011, 10:44 a.m.
On 29/09/11 12:27, Richard Guenther wrote:
> On Thu, Sep 29, 2011 at 12:00 PM, Richard Guenther
> <richard.guenther@gmail.com>  wrote:
>> On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
>> <richard.guenther@gmail.com>  wrote:
>>> On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
>>> <richard.guenther@gmail.com>  wrote:
>>>> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
>>>> <richard.guenther@gmail.com>  wrote:
>>>>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers<joseph@codesourcery.com>  wrote:
>>>>>> This looks like it has the same issue with maybe needing to use
>>>>>> TYPE_MAIN_VARIANT in type comparisons as the shuffle patch.
>>>>>
>>>>> I don't think so, we move qualifiers to the vector type from the element type
>>>>> in make_vector_type and the tests only look at the component type.
>>>>>
>>>>> I am re-testing the patch currently and will commit it if that succeeds.
>>>>
>>>> Unfortunately gcc.c-torture/execute/vector-compare-1.c fails with -m32
>>>> for
>>>>
>>>>     vector (2, double) d0;
>>>>     vector (2, double) d1;
>>>>     vector (2, long) idres;
>>>>
>>>>     d0 = (vector (2, double)){(double)argc,  10.};
>>>>     d1 = (vector (2, double)){0., (double)-23};
>>>>     idres = (d0>  d1);
>>>>
>>>> as appearantly the type we chose to assign to (d0>  d1) is different
>>>> from that of idres:
>>>>
>>>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>>>> error: incompatible types when assigning to type '__vector(2) long
>>>> int' from type '__vector(2) long long int'^M
>>>>
>>>> Adjusting it to vector (2, long long) otoh yields, for -m64:
>>>>
>>>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:118:5:
>>>> error: incompatible types when assigning to type '__vector(2) long
>>>> long int' from type '__vector(2) long int'
>>>>
>>>> But those two types are at least compatible from their modes.  Joseph,
>>>> should we accept mode-compatible types in assignments or maybe
>>>> transparently convert them?
>>>
>>> Looks like we have a more suitable solution for these automatically
>>> generated vector types - mark them with TYPE_VECTOR_OPAQUE.
>>>
>>> I'm testing the following incremental patch.
>>>
>>> Richard.
>>>
>>> Index: gcc/c-typeck.c
>>> ===================================================================
>>> --- gcc/c-typeck.c.orig 2011-09-28 16:22:10.000000000 +0200
>>> +++ gcc/c-typeck.c      2011-09-28 16:18:39.000000000 +0200
>>> @@ -9928,8 +9928,10 @@ build_binary_op (location_t location, en
>>>              }
>>>
>>>            /* Always construct signed integer vector type.  */
>>> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
>>> (type0)), 0);
>>> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
>>> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
>>> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
>>> +          result_type = build_opaque_vector_type (intt,
>>> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>>>            converted = 1;
>>>            break;
>>>          }
>>> @@ -10063,8 +10065,10 @@ build_binary_op (location_t location, en
>>>              }
>>>
>>>            /* Always construct signed integer vector type.  */
>>> -          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
>>> (type0)), 0);
>>> -          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
>>> +          intt = c_common_type_for_size (GET_MODE_BITSIZE
>>> +                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
>>> +          result_type = build_opaque_vector_type (intt,
>>> +                                                 TYPE_VECTOR_SUBPARTS (type0));
>>>            converted = 1;
>>>            break;
>>>          }
>>
>> That doesn't seem to work either.  Because we treat the opaque and
>> non-opaque variants of vector<int>  as different (the opaque type isn't
>> a variant type of the non-opaque one - something suspicious anyway).
>>
>> I'm going to try to apply some surgery on how we build opaque variants
>> and then re-visit the above again.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu and installed.
>
> Richard.
>
>> Richard.
>>
>

I'm still getting errors with latest trunk (r179378) for arm-none-eabi. 
  Please see http://gcc.gnu.org/PR50576.

Thanks,

Matt

Patch

Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c.orig 2011-09-28 16:22:10.000000000 +0200
+++ gcc/c-typeck.c      2011-09-28 16:18:39.000000000 +0200
@@ -9928,8 +9928,10 @@  build_binary_op (location_t location, en
             }

           /* Always construct signed integer vector type.  */
-          intt = c_common_type_for_size (TYPE_PRECISION (TREE_TYPE
(type0)), 0);
-          result_type = build_vector_type (intt, TYPE_VECTOR_SUBPARTS (type0));
+          intt = c_common_type_for_size (GET_MODE_BITSIZE
+                                          (TYPE_MODE (TREE_TYPE (type0))), 0);
+          result_type = build_opaque_vector_type (intt,
+                                                 TYPE_VECTOR_SUBPARTS (type0));
           converted = 1;
           break;