Re: Vector Comparison patch
Jakub Jelinek schrieb: > On Tue, Oct 04, 2011 at 11:32:37AM +0200, Georg-Johann Lay wrote: >> The patch from >> http://gcc.gnu.org/ml/gcc-patches/2011-09/msg02060.html >> http://gcc.gnu.org/ml/gcc-patches/2011-09/txt00337.txt >> works for me. >> >> If it's ok from maintainer I can apply it for you. > > It is fine with a suitable ChangeLog entry. > > Jakub It's here: http://gcc.gnu.org/viewcvs?view=revision&revision=179497 Johann
Re: Vector Comparison patch
On Tue, Oct 04, 2011 at 11:32:37AM +0200, Georg-Johann Lay wrote: > The patch from > http://gcc.gnu.org/ml/gcc-patches/2011-09/msg02060.html > http://gcc.gnu.org/ml/gcc-patches/2011-09/txt00337.txt > works for me. > > If it's ok from maintainer I can apply it for you. It is fine with a suitable ChangeLog entry. Jakub
Re: Vector Comparison patch
Artem Shinkarov schrieb: > On Fri, Sep 30, 2011 at 4:54 PM, Jakub Jelinek wrote: >> On Fri, Sep 30, 2011 at 04:48:41PM +0100, Artem Shinkarov wrote: >>> Most likely we can. The question is what do we really want to check >>> with this test. My intention was to check that a programmer can >>> statically get correspondence of the types, in a sense that sizeof >>> (float) == sizeof (int) and sizeof (double) == sizeof (long long). As >>> it seems my original assumption does not hold. Before using __typeof, >>> I would try to make sure that there is no other way to determine these >>> correspondences. >> You can use preprocessor too, either just surround the whole test >> with #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ and similar, >> or select the right type through preprocessor >> #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ >> #define FLOATCMPTYPE int >> #elif __SIZEOF_LONG__ == __SIZEOF_FLOAT__ >> #define FLOATCMPTYPE long >> #else >> ... >> or __typeof, etc. >> >>Jakub >> > > Ok, here is a patch which uses __typeof. Passes on x86_64. > > Artem. The patch from http://gcc.gnu.org/ml/gcc-patches/2011-09/msg02060.html http://gcc.gnu.org/ml/gcc-patches/2011-09/txt00337.txt works for me. If it's ok from maintainer I can apply it for you. Johann
Re: Vector Comparison patch
Artem Shinkarov schrieb: On Fri, Sep 30, 2011 at 4:54 PM, Jakub Jelinek wrote: On Fri, Sep 30, 2011 at 04:48:41PM +0100, Artem Shinkarov wrote: Most likely we can. The question is what do we really want to check with this test. My intention was to check that a programmer can statically get correspondence of the types, in a sense that sizeof (float) == sizeof (int) and sizeof (double) == sizeof (long long). As it seems my original assumption does not hold. Before using __typeof, I would try to make sure that there is no other way to determine these correspondences. You can use preprocessor too, either just surround the whole test with #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ and similar, or select the right type through preprocessor #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ #define FLOATCMPTYPE int #elif __SIZEOF_LONG__ == __SIZEOF_FLOAT__ #define FLOATCMPTYPE long #else ... or __typeof, etc. Jakub Ok, here is a patch which uses __typeof. Passes on x86_64. Thanks, I will test on avr next week. Johann Artem.
Re: Vector Comparison patch
On Fri, Sep 30, 2011 at 4:54 PM, Jakub Jelinek wrote:
> On Fri, Sep 30, 2011 at 04:48:41PM +0100, Artem Shinkarov wrote:
>> Most likely we can. The question is what do we really want to check
>> with this test. My intention was to check that a programmer can
>> statically get correspondence of the types, in a sense that sizeof
>> (float) == sizeof (int) and sizeof (double) == sizeof (long long). As
>> it seems my original assumption does not hold. Before using __typeof,
>> I would try to make sure that there is no other way to determine these
>> correspondences.
>
> You can use preprocessor too, either just surround the whole test
> with #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ and similar,
> or select the right type through preprocessor
> #if __SIZEOF_INT__ == __SIZEOF_FLOAT__
> #define FLOATCMPTYPE int
> #elif __SIZEOF_LONG__ == __SIZEOF_FLOAT__
> #define FLOATCMPTYPE long
> #else
> ...
> or __typeof, etc.
>
> Jakub
>
Ok, here is a patch which uses __typeof. Passes on x86_64.
Artem.
Index: gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c
===
--- gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c (revision
179378)
+++ gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c (working copy)
@@ -39,17 +39,17 @@ int main (int argc, char *argv[]) {
int i;
i0 = (vector (4, INT)){argc, 1, 2, 10};
-i1 = (vector (4, INT)){0, 3, 2, (INT)-23};
+i1 = (vector (4, INT)){0, 3, 2, (INT)-23};
test (4, i0, i1, ires, "%i");
#undef INT
-#define INT unsigned int
+#define INT unsigned int
vector (4, int) ures;
vector (4, INT) u0;
vector (4, INT) u1;
u0 = (vector (4, INT)){argc, 1, 2, 10};
-u1 = (vector (4, INT)){0, 3, 2, (INT)-23};
+u1 = (vector (4, INT)){0, 3, 2, (INT)-23};
test (4, u0, u1, ures, "%u");
#undef INT
@@ -60,7 +60,7 @@ int main (int argc, char *argv[]) {
vector (8, short) sres;
s0 = (vector (8, SHORT)){argc, 1, 2, 10, 6, 87, (SHORT)-5, 2};
-s1 = (vector (8, SHORT)){0, 3, 2, (SHORT)-23, 12, 10, (SHORT)-2, 0};
+s1 = (vector (8, SHORT)){0, 3, 2, (SHORT)-23, 12, 10, (SHORT)-2, 0};
test (8, s0, s1, sres, "%i");
#undef SHORT
@@ -70,7 +70,7 @@ int main (int argc, char *argv[]) {
vector (8, short) usres;
us0 = (vector (8, SHORT)){argc, 1, 2, 10, 6, 87, (SHORT)-5, 2};
-us1 = (vector (8, SHORT)){0, 3, 2, (SHORT)-23, 12, 10, (SHORT)-2, 0};
+us1 = (vector (8, SHORT)){0, 3, 2, (SHORT)-23, 12, 10, (SHORT)-2, 0};
test (8, us0, us1, usres, "%u");
#undef SHORT
@@ -102,19 +102,19 @@ int main (int argc, char *argv[]) {
/* Float comparison. */
vector (4, float) f0;
vector (4, float) f1;
-vector (4, int) ifres;
+__typeof (f0 == f1) ifres;
f0 = (vector (4, float)){(float)argc, 1., 2., 10.};
-f1 = (vector (4, float)){0., 3., 2., (float)-23};
+f1 = (vector (4, float)){0., 3., 2., (float)-23};
test (4, f0, f1, ifres, "%f");
-
+
/* Double comparison. */
vector (2, double) d0;
vector (2, double) d1;
-vector (2, long long) idres;
+__typeof (d0 == d1) idres;
d0 = (vector (2, double)){(double)argc, 10.};
-d1 = (vector (2, double)){0., (double)-23};
+d1 = (vector (2, double)){0., (double)-23};
test (2, d0, d1, idres, "%f");
Re: Vector Comparison patch
On Fri, Sep 30, 2011 at 04:48:41PM +0100, Artem Shinkarov wrote: > Most likely we can. The question is what do we really want to check > with this test. My intention was to check that a programmer can > statically get correspondence of the types, in a sense that sizeof > (float) == sizeof (int) and sizeof (double) == sizeof (long long). As > it seems my original assumption does not hold. Before using __typeof, > I would try to make sure that there is no other way to determine these > correspondences. You can use preprocessor too, either just surround the whole test with #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ and similar, or select the right type through preprocessor #if __SIZEOF_INT__ == __SIZEOF_FLOAT__ #define FLOATCMPTYPE int #elif __SIZEOF_LONG__ == __SIZEOF_FLOAT__ #define FLOATCMPTYPE long #else ... or __typeof, etc. Jakub
Re: Vector Comparison patch
On Fri, Sep 30, 2011 at 4:43 PM, Jakub Jelinek wrote:
> On Fri, Sep 30, 2011 at 05:36:47PM +0200, Georg-Johann Lay wrote:
>> >> The target has
>> >>
>> >> 2 = sizeof (short)
>> >> 2 = sizeof (int)
>> >> 4 = sizeof (long int)
>> >> 8 = sizeof (long long int)
>> >>
>> >> Could you fix that? I.e. parametrize sizeof(int) out or skip the test by
>> >> means of
>> >>
>> >> /* { dg-require-effective-target int32plus } */
>> >>
>> >> or similar.
>> >>
>> >> Thanks, Johann
>> >>
>> >> [...]
>> >>
>> > The problem actually happens when we compare float vector with float
>> > vector, it is assumed that we should get int vector as a result, but
>> > it turns out that we are getting long int.
>> >
>> > The same with double, we assume that sizeof (double) == sizeof (long
>> > long). But as it seems double has the same size as float.
>>
>> Yes.
>>
>> sizeof(double) = sizeof(float) = 4
>>
>> > Hm, I can put conditional of sort:
>> > if (sizeof (doulbe) == sizeof (long long)) and others. Or may be there
>> > is more elegant way of solving this?
>>
>> That's too late because this won't prevent the compiler from error.
>> The error already happens at compile time, not at run time.
>
> Isn't it possible to do something like:
> vector (4, float) f0;
> vector (4, float) f1;
> - vector (4, int) ifres;
> + vector (4, __typeof (f0 > f1)) ifres;
>
> f0 = (vector (4, float)){(float)argc, 1., 2., 10.};
> f1 = (vector (4, float)){0., 3., 2., (float)-23};
> test (4, f0, f1, ifres, "%f");
>
> /* Double comparison. */
> vector (2, double) d0;
> vector (2, double) d1;
> - vector (2, long long) idres;
> + vector (2, __typeof (d0 > d1)) idres;
>
> d0 = (vector (2, double)){(double)argc, 10.};
> d1 = (vector (2, double)){0., (double)-23};
> test (2, d0, d1, idres, "%f");
>
> Jakub
>
Most likely we can. The question is what do we really want to check
with this test. My intention was to check that a programmer can
statically get correspondence of the types, in a sense that sizeof
(float) == sizeof (int) and sizeof (double) == sizeof (long long). As
it seems my original assumption does not hold. Before using __typeof,
I would try to make sure that there is no other way to determine these
correspondences.
Artem.
Re: Vector Comparison patch
On Fri, Sep 30, 2011 at 05:36:47PM +0200, Georg-Johann Lay wrote:
> >> The target has
> >>
> >> 2 = sizeof (short)
> >> 2 = sizeof (int)
> >> 4 = sizeof (long int)
> >> 8 = sizeof (long long int)
> >>
> >> Could you fix that? I.e. parametrize sizeof(int) out or skip the test by
> >> means of
> >>
> >> /* { dg-require-effective-target int32plus } */
> >>
> >> or similar.
> >>
> >> Thanks, Johann
> >>
> >> [...]
> >>
> > The problem actually happens when we compare float vector with float
> > vector, it is assumed that we should get int vector as a result, but
> > it turns out that we are getting long int.
> >
> > The same with double, we assume that sizeof (double) == sizeof (long
> > long). But as it seems double has the same size as float.
>
> Yes.
>
> sizeof(double) = sizeof(float) = 4
>
> > Hm, I can put conditional of sort:
> > if (sizeof (doulbe) == sizeof (long long)) and others. Or may be there
> > is more elegant way of solving this?
>
> That's too late because this won't prevent the compiler from error.
> The error already happens at compile time, not at run time.
Isn't it possible to do something like:
vector (4, float) f0;
vector (4, float) f1;
-vector (4, int) ifres;
+vector (4, __typeof (f0 > f1)) ifres;
f0 = (vector (4, float)){(float)argc, 1., 2., 10.};
f1 = (vector (4, float)){0., 3., 2., (float)-23};
test (4, f0, f1, ifres, "%f");
/* Double comparison. */
vector (2, double) d0;
vector (2, double) d1;
-vector (2, long long) idres;
+vector (2, __typeof (d0 > d1)) idres;
d0 = (vector (2, double)){(double)argc, 10.};
d1 = (vector (2, double)){0., (double)-23};
test (2, d0, d1, idres, "%f");
Jakub
Re: Vector Comparison patch
Artem Shinkarov schrieb:
> On Fri, Sep 30, 2011 at 4:01 PM, Georg-Johann Lay wrote:
>> Artem Shinkarov schrieb:
>>> Here is a new version of the patch which considers the changes from
>>> 2011-09-02 Richard Guenther
>>>
>>>
>>> ChangeLog
>>>
>>> 20011-09-06 Artjoms Sinkarovs
>>>
>>>gcc/
>>>* fold-const.c (constant_boolean_node): Adjust the meaning
>>>of boolean for vector types: true = {-1,..}, false = {0,..}.
>>>(fold_unary_loc): Avoid conversion of vector comparison to
>>>boolean type.
>>>* expr.c (expand_expr_real_2): Expand vector comparison by
>>>building an appropriate VEC_COND_EXPR.
>>>* c-typeck.c (build_binary_op): Typecheck vector comparisons.
>>>(c_objc_common_truthvalue_conversion): Adjust.
>>>* tree-vect-generic.c (do_compare): Helper function.
>>>(expand_vector_comparison): Check if hardware supports
>>>vector comparison of the given type or expand vector
>>>piecewise.
>>>(expand_vector_operation): Treat comparison as binary
>>>operation of vector type.
>>>(expand_vector_operations_1): Adjust.
>>>* tree-cfg.c (verify_gimple_comparison): Adjust.
>>>
>>>gcc/config/i386
>>>* i386.c (ix86_expand_sse_movcc): Consider a case when
>>>vcond operators are {-1,..} and {0,..}.
>>>
>>>gcc/doc
>>>* extend.texi: Adjust.
>>>
>>>gcc/testsuite
>>>* gcc.c-torture/execute/vector-compare-1.c: New test.
>>>* gcc.c-torture/execute/vector-compare-2.c: New test.
>>>* gcc.dg/vector-compare-1.c: New test.
>>>* gcc.dg/vector-compare-2.c: New test.
>>>
>>> bootstrapped and tested on x86_64-unknown-linux-gnu.
>>>
>>>
>>> Thanks,
>>> Artem.
>> Hi Artem,
>>
>> the new test case gcc.c-torture/execute/vector-compare-1.c causes bunch of
>> FAILS in regression tests for avr-unknown-none (see attachment).
>>
>> The target has
>>
>> 2 = sizeof (short)
>> 2 = sizeof (int)
>> 4 = sizeof (long int)
>> 8 = sizeof (long long int)
>>
>> Could you fix that? I.e. parametrize sizeof(int) out or skip the test by
>> means of
>>
>> /* { dg-require-effective-target int32plus } */
>>
>> or similar.
>>
>> Thanks, Johann
>>
>> [...]
>>
> Hi
>
> The problem actually happens when we compare float vector with float
> vector, it is assumed that we should get int vector as a result, but
> it turns out that we are getting long int.
>
> The same with double, we assume that sizeof (double) == sizeof (long
> long). But as it seems double has the same size as float.
Yes.
sizeof(double) = sizeof(float) = 4
> Hm, I can put conditional of sort:
> if (sizeof (doulbe) == sizeof (long long)) and others. Or may be there
> is more elegant way of solving this?
That's too late because this won't prevent the compiler from error.
The error already happens at compile time, not at run time.
> I can fix it, but keep in mind that I don't have a permission to
> commit to the trunk.
You could browse ./testsuite/lib/target-supports.exp and try to find some gate
functions that fit the test case's requirement like
check_effective_target_large_double, check_effective_target_double64,
check_effective_target_x32 or a combination of them.
Johann
> Artem.
Re: Vector Comparison patch
On Fri, Sep 30, 2011 at 4:01 PM, Georg-Johann Lay wrote:
> Artem Shinkarov schrieb:
>> Here is a new version of the patch which considers the changes from
>> 2011-09-02 Richard Guenther
>>
>>
>> ChangeLog
>>
>> 20011-09-06 Artjoms Sinkarovs
>>
>> gcc/
>> * fold-const.c (constant_boolean_node): Adjust the meaning
>> of boolean for vector types: true = {-1,..}, false = {0,..}.
>> (fold_unary_loc): Avoid conversion of vector comparison to
>> boolean type.
>> * expr.c (expand_expr_real_2): Expand vector comparison by
>> building an appropriate VEC_COND_EXPR.
>> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
>> (c_objc_common_truthvalue_conversion): Adjust.
>> * tree-vect-generic.c (do_compare): Helper function.
>> (expand_vector_comparison): Check if hardware supports
>> vector comparison of the given type or expand vector
>> piecewise.
>> (expand_vector_operation): Treat comparison as binary
>> operation of vector type.
>> (expand_vector_operations_1): Adjust.
>> * tree-cfg.c (verify_gimple_comparison): Adjust.
>>
>> gcc/config/i386
>> * i386.c (ix86_expand_sse_movcc): Consider a case when
>> vcond operators are {-1,..} and {0,..}.
>>
>> gcc/doc
>> * extend.texi: Adjust.
>>
>> gcc/testsuite
>> * gcc.c-torture/execute/vector-compare-1.c: New test.
>> * gcc.c-torture/execute/vector-compare-2.c: New test.
>> * gcc.dg/vector-compare-1.c: New test.
>> * gcc.dg/vector-compare-2.c: New test.
>>
>> bootstrapped and tested on x86_64-unknown-linux-gnu.
>>
>>
>> Thanks,
>> Artem.
>
> Hi Artem,
>
> the new test case gcc.c-torture/execute/vector-compare-1.c causes bunch of
> FAILS in regression tests for avr-unknown-none (see attachment).
>
> The target has
>
> 2 = sizeof (short)
> 2 = sizeof (int)
> 4 = sizeof (long int)
> 8 = sizeof (long long int)
>
> Could you fix that? I.e. parametrize sizeof(int) out or skip the test by
> means of
>
> /* { dg-require-effective-target int32plus } */
>
> or similar.
>
> Thanks, Johann
>
>
>
>
>
>
>
>
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c: In function 'main':
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./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'
> ./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'
> ./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'
> ./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'
> ./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'
> ./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'
> compiler exited with status 1
> output is:
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c: In function 'main':
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4) long int'
> ./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
> incompatible types when assigning to type '__vector(4) int' from type
> '__vector(4)
Re: Vector Comparison patch
Artem Shinkarov schrieb:
> Here is a new version of the patch which considers the changes from
> 2011-09-02 Richard Guenther
>
>
> ChangeLog
>
> 20011-09-06 Artjoms Sinkarovs
>
>gcc/
>* fold-const.c (constant_boolean_node): Adjust the meaning
>of boolean for vector types: true = {-1,..}, false = {0,..}.
>(fold_unary_loc): Avoid conversion of vector comparison to
>boolean type.
>* expr.c (expand_expr_real_2): Expand vector comparison by
>building an appropriate VEC_COND_EXPR.
>* c-typeck.c (build_binary_op): Typecheck vector comparisons.
>(c_objc_common_truthvalue_conversion): Adjust.
>* tree-vect-generic.c (do_compare): Helper function.
>(expand_vector_comparison): Check if hardware supports
>vector comparison of the given type or expand vector
>piecewise.
>(expand_vector_operation): Treat comparison as binary
>operation of vector type.
>(expand_vector_operations_1): Adjust.
>* tree-cfg.c (verify_gimple_comparison): Adjust.
>
>gcc/config/i386
>* i386.c (ix86_expand_sse_movcc): Consider a case when
>vcond operators are {-1,..} and {0,..}.
>
>gcc/doc
>* extend.texi: Adjust.
>
>gcc/testsuite
>* gcc.c-torture/execute/vector-compare-1.c: New test.
>* gcc.c-torture/execute/vector-compare-2.c: New test.
>* gcc.dg/vector-compare-1.c: New test.
>* gcc.dg/vector-compare-2.c: New test.
>
> bootstrapped and tested on x86_64-unknown-linux-gnu.
>
>
> Thanks,
> Artem.
Hi Artem,
the new test case gcc.c-torture/execute/vector-compare-1.c causes bunch of
FAILS in regression tests for avr-unknown-none (see attachment).
The target has
2 = sizeof (short)
2 = sizeof (int)
4 = sizeof (long int)
8 = sizeof (long long int)
Could you fix that? I.e. parametrize sizeof(int) out or skip the test by means
of
/* { dg-require-effective-target int32plus } */
or similar.
Thanks, Johann
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c: In function 'main':
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./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'
./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'
./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'
./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'
./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'
./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'
compiler exited with status 1
output is:
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c: In function 'main':
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109:5: error:
incompatible types when assigning to type '__vector(4) int' from type
'__vector(4) long int'
./gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:10
Re: Vector Comparison patch
On 29/09/11 12:27, Richard Guenther wrote:
On Thu, Sep 29, 2011 at 12:00 PM, Richard Guenther
wrote:
On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
wrote:
On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
wrote:
On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
wrote:
On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers 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.0 +0200
+++ gcc/c-typeck.c 2011-09-28 16:18:39.0 +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 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
--
Matthew Gretton-Dann
Principal Engineer, PD Software - Tools, ARM Ltd
Re: Vector Comparison patch
On Thu, Sep 29, 2011 at 12:00 PM, Richard Guenther
wrote:
> On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
> wrote:
>> On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
>> wrote:
>>> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
>>> wrote:
On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers
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.0 +0200
>> +++ gcc/c-typeck.c 2011-09-28 16:18:39.0 +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 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.
>
Re: Vector Comparison patch
On Wed, Sep 28, 2011 at 4:23 PM, Richard Guenther
wrote:
> On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
> wrote:
>> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
>> wrote:
>>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers
>>> 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.0 +0200
> +++ gcc/c-typeck.c 2011-09-28 16:18:39.0 +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 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.
Re: Vector Comparison patch
On Mon, Sep 26, 2011 at 5:43 PM, Richard Guenther
wrote:
> On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
> wrote:
>> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers
>> 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.0 +0200
+++ gcc/c-typeck.c 2011-09-28 16:18:39.0 +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;
}
Re: Vector Comparison patch
On Mon, Sep 26, 2011 at 4:25 PM, Richard Guenther
wrote:
> On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers
> 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?
Thanks,
Richard.
> Thanks,
> Richard.
>
>> --
>> Joseph S. Myers
>> jos...@codesourcery.com
>>
>
Re: Vector Comparison patch
On Wed, Sep 7, 2011 at 5:06 PM, Joseph S. Myers 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. Thanks, Richard. > -- > Joseph S. Myers > jos...@codesourcery.com >
Re: Vector Comparison patch
On 08/29/2011 04:41 AM, Paolo Bonzini wrote: > The definition in OpenCL makes zero sense to me. For byte operands > it is custom-tailored after the SSE PMOVMSKB instruction, but there > is no PMOVMSKW/PMOVMSKD instruction so you would need very slow bit > shift operations before PMOVMSK. On the other hand, bit selection is > for example in Altivec. Not PMOVMSKB, but the sse4.1 PBLENDVB. With that, we don't need funny shift operations for wider integer types, but only *because* the comparison produces -1, which means that the MSB of each byte is in fact set. Which means that the Perfect wording probably doesn't want to be specific to bit selection, but include bit selection (aka and-andn-or) as a valid implementation. r~
Re: Vector Comparison patch
On Thu, Sep 8, 2011 at 2:41 PM, Richard Guenther
wrote:
>> All the rest is adjusted in the new version of the patch you can find
>> in the attachment.
>>
>> ChangLog
>>
>>
>> 20011-09-06 Artjoms Sinkarovs
>>
>> gcc/
>> * expr.c (do_store_flag): Expand vector comparison by
>> building an appropriate VEC_COND_EXPR.
>> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
>> (c_objc_common_truthvalue_conversion): Adjust.
>> * tree-vect-generic.c (do_compare): Helper function.
>> (expand_vector_comparison): Check if hardware supports
>> vector comparison of the given type or expand vector
>> piecewise.
>> (expand_vector_operation): Treat comparison as binary
>> operation of vector type.
>> (expand_vector_operations_1): Adjust.
>>
>> gcc/config/i386
>> * i386.c (ix86_expand_sse_movcc): Consider a case when
>> vcond operators are {-1,..} and {0,..}.
>>
>> gcc/doc
>> * extend.texi: Adjust.
>>
>> gcc/testsuite
>> * gcc.c-torture/execute/vector-compare-1.c: New test.
>> * gcc.c-torture/execute/vector-compare-2.c: New test.
>> * gcc.dg/vector-compare-1.c: New test.
>> * gcc.dg/vector-compare-2.c: New test.
>>
>> bootstrapped and tested on x86_64-unknown-linux-gnu.
The x86 part is OK.
Thanks,
Uros.
Re: Vector Comparison patch
On Thu, Sep 8, 2011 at 2:41 PM, Richard Guenther
wrote:
> On Wed, Sep 7, 2011 at 3:15 PM, Artem Shinkarov
> wrote:
>> On Tue, Sep 6, 2011 at 3:56 PM, Richard Guenther
>> wrote:
>>> On Tue, Sep 6, 2011 at 4:50 PM, Artem Shinkarov
>>> wrote:
Here is a new version of the patch which considers the changes from
2011-09-02 Richard Guenther
ChangeLog
20011-09-06 Artjoms Sinkarovs
gcc/
* fold-const.c (constant_boolean_node): Adjust the meaning
of boolean for vector types: true = {-1,..}, false = {0,..}.
(fold_unary_loc): Avoid conversion of vector comparison to
boolean type.
>>>
>>> Both changes have already been done.
>>
>> I missed the way you applied constant_boolean node, sorry for that.
>> But fold_unary_loc seems confusing to me. We have the following code:
>>
>> else if (!INTEGRAL_TYPE_P (type))
>> return build3_loc (loc, COND_EXPR, type, op0,
>> constant_boolean_node (true, type),
>> constant_boolean_node (false, type));
>>
>> But this is wrong for the vector types, because it should construct
>> VEC_COND_EXPR, not COND_EXPR. That is why I had a special case for
>> vectors.
>
> Ah, yeah. I'll fix that.
OTOH, we require that vectors are converted with VIEW_CONVERT_EXPRs,
so the code shouldn't trigger anyway.
Richard.
> The patch looks ok to me from a middle-end point of view. Thus, if
> Joseph is fine with it and Uros is, with the i386 piece the patch is ok.
>
> Thanks,
> Richard.
Re: Vector Comparison patch
On Wed, Sep 7, 2011 at 3:15 PM, Artem Shinkarov
wrote:
> On Tue, Sep 6, 2011 at 3:56 PM, Richard Guenther
> wrote:
>> On Tue, Sep 6, 2011 at 4:50 PM, Artem Shinkarov
>> wrote:
>>> Here is a new version of the patch which considers the changes from
>>> 2011-09-02 Richard Guenther
>>>
>>>
>>> ChangeLog
>>>
>>> 20011-09-06 Artjoms Sinkarovs
>>>
>>> gcc/
>>> * fold-const.c (constant_boolean_node): Adjust the meaning
>>> of boolean for vector types: true = {-1,..}, false = {0,..}.
>>> (fold_unary_loc): Avoid conversion of vector comparison to
>>> boolean type.
>>
>> Both changes have already been done.
>
> I missed the way you applied constant_boolean node, sorry for that.
> But fold_unary_loc seems confusing to me. We have the following code:
>
> else if (!INTEGRAL_TYPE_P (type))
> return build3_loc (loc, COND_EXPR, type, op0,
> constant_boolean_node (true, type),
> constant_boolean_node (false, type));
>
> But this is wrong for the vector types, because it should construct
> VEC_COND_EXPR, not COND_EXPR. That is why I had a special case for
> vectors.
Ah, yeah. I'll fix that.
The patch looks ok to me from a middle-end point of view. Thus, if
Joseph is fine with it and Uros is, with the i386 piece the patch is ok.
Thanks,
Richard.
>>> * expr.c (expand_expr_real_2): Expand vector comparison by
>>> building an appropriate VEC_COND_EXPR.
>>
>> I prefer
>>
>> Index: gcc/expr.c
>> ===
>> *** gcc/expr.c.orig 2011-08-29 11:48:23.0 +0200
>> --- gcc/expr.c 2011-08-29 12:58:59.0 +0200
>> *** do_store_flag (sepops ops, rtx target, e
>> *** 10309,10314
>> --- 10309,10325
>> STRIP_NOPS (arg0);
>> STRIP_NOPS (arg1);
>>
>> + /* For vector typed comparisons emit code to generate the desired
>> + all-ones or all-zeros mask. Conveniently use the VEC_COND_EXPR
>> + expander for this. */
>> + if (TREE_CODE (ops->type) == VECTOR_TYPE)
>> + {
>> + tree ifexp = build2 (ops->code, ops->type, arg0, arg1);
>> + tree if_true = constant_boolean_node (true, ops->type);
>> + tree if_false = constant_boolean_node (false, ops->type);
>> + return expand_vec_cond_expr (ops->type, ifexp, if_true,
>> if_false, target);
>> + }
>> +
>> /* Get the rtx comparison code to use. We know that EXP is a comparison
>>
>> as I said multiple times.
>>
>>> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
>>> (c_objc_common_truthvalue_conversion): Adjust.
>>> * tree-vect-generic.c (do_compare): Helper function.
>>> (expand_vector_comparison): Check if hardware supports
>>> vector comparison of the given type or expand vector
>>> piecewise.
>>> (expand_vector_operation): Treat comparison as binary
>>> operation of vector type.
>>> (expand_vector_operations_1): Adjust.
>>> * tree-cfg.c (verify_gimple_comparison): Adjust.
>>
>> The tree-cfg.c change has already been done.
>>
>> Richard.
>>
>>>
>>> gcc/config/i386
>>> * i386.c (ix86_expand_sse_movcc): Consider a case when
>>> vcond operators are {-1,..} and {0,..}.
>>>
>>> gcc/doc
>>> * extend.texi: Adjust.
>>>
>>> gcc/testsuite
>>> * gcc.c-torture/execute/vector-compare-1.c: New test.
>>> * gcc.c-torture/execute/vector-compare-2.c: New test.
>>> * gcc.dg/vector-compare-1.c: New test.
>>> * gcc.dg/vector-compare-2.c: New test.
>>>
>>> bootstrapped and tested on x86_64-unknown-linux-gnu.
>>>
>>>
>>> Thanks,
>>> Artem.
>>>
>>
>
> All the rest is adjusted in the new version of the patch you can find
> in the attachment.
>
> ChangLog
>
>
> 20011-09-06 Artjoms Sinkarovs
>
> gcc/
> * expr.c (do_store_flag): Expand vector comparison by
> building an appropriate VEC_COND_EXPR.
> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
> (c_objc_common_truthvalue_conversion): Adjust.
> * tree-vect-generic.c (do_compare): Helper function.
> (expand_vector_comparison): Check if hardware supports
> vector comparison of the given type or expand vector
> piecewise.
> (expand_vector_operation): Treat comparison as binary
> operation of vector type.
> (expand_vector_operations_1): Adjust.
>
> gcc/config/i386
> * i386.c (ix86_expand_sse_movcc): Consider a case when
> vcond operators are {-1,..} and {0,..}.
>
> gcc/doc
> * extend.texi: Adjust.
>
> gcc/testsuite
> * gcc.c-torture/execute/vector-compare-1.c: New test.
> * gcc.c-torture/execute/vector-compare-2.c: New test.
> * gcc.dg/vector-compare-1.c: New test.
> * gcc.dg/vector-compare-2.c: New test.
>
> bootstrapped and tested on x86_64-unknown-linux-gnu.
>
Re: Vector Comparison patch
This looks like it has the same issue with maybe needing to use TYPE_MAIN_VARIANT in type comparisons as the shuffle patch. -- Joseph S. Myers jos...@codesourcery.com
Re: Vector Comparison patch
On Tue, Sep 6, 2011 at 3:56 PM, Richard Guenther
wrote:
> On Tue, Sep 6, 2011 at 4:50 PM, Artem Shinkarov
> wrote:
>> Here is a new version of the patch which considers the changes from
>> 2011-09-02 Richard Guenther
>>
>>
>> ChangeLog
>>
>> 20011-09-06 Artjoms Sinkarovs
>>
>> gcc/
>> * fold-const.c (constant_boolean_node): Adjust the meaning
>> of boolean for vector types: true = {-1,..}, false = {0,..}.
>> (fold_unary_loc): Avoid conversion of vector comparison to
>> boolean type.
>
> Both changes have already been done.
I missed the way you applied constant_boolean node, sorry for that.
But fold_unary_loc seems confusing to me. We have the following code:
else if (!INTEGRAL_TYPE_P (type))
return build3_loc (loc, COND_EXPR, type, op0,
constant_boolean_node (true, type),
constant_boolean_node (false, type));
But this is wrong for the vector types, because it should construct
VEC_COND_EXPR, not COND_EXPR. That is why I had a special case for
vectors.
>> * expr.c (expand_expr_real_2): Expand vector comparison by
>> building an appropriate VEC_COND_EXPR.
>
> I prefer
>
> Index: gcc/expr.c
> ===
> *** gcc/expr.c.orig 2011-08-29 11:48:23.0 +0200
> --- gcc/expr.c 2011-08-29 12:58:59.0 +0200
> *** do_store_flag (sepops ops, rtx target, e
> *** 10309,10314
> --- 10309,10325
> STRIP_NOPS (arg0);
> STRIP_NOPS (arg1);
>
> + /* For vector typed comparisons emit code to generate the desired
> + all-ones or all-zeros mask. Conveniently use the VEC_COND_EXPR
> + expander for this. */
> + if (TREE_CODE (ops->type) == VECTOR_TYPE)
> + {
> + tree ifexp = build2 (ops->code, ops->type, arg0, arg1);
> + tree if_true = constant_boolean_node (true, ops->type);
> + tree if_false = constant_boolean_node (false, ops->type);
> + return expand_vec_cond_expr (ops->type, ifexp, if_true,
> if_false, target);
> + }
> +
> /* Get the rtx comparison code to use. We know that EXP is a comparison
>
> as I said multiple times.
>
>> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
>> (c_objc_common_truthvalue_conversion): Adjust.
>> * tree-vect-generic.c (do_compare): Helper function.
>> (expand_vector_comparison): Check if hardware supports
>> vector comparison of the given type or expand vector
>> piecewise.
>> (expand_vector_operation): Treat comparison as binary
>> operation of vector type.
>> (expand_vector_operations_1): Adjust.
>> * tree-cfg.c (verify_gimple_comparison): Adjust.
>
> The tree-cfg.c change has already been done.
>
> Richard.
>
>>
>> gcc/config/i386
>> * i386.c (ix86_expand_sse_movcc): Consider a case when
>> vcond operators are {-1,..} and {0,..}.
>>
>> gcc/doc
>> * extend.texi: Adjust.
>>
>> gcc/testsuite
>> * gcc.c-torture/execute/vector-compare-1.c: New test.
>> * gcc.c-torture/execute/vector-compare-2.c: New test.
>> * gcc.dg/vector-compare-1.c: New test.
>> * gcc.dg/vector-compare-2.c: New test.
>>
>> bootstrapped and tested on x86_64-unknown-linux-gnu.
>>
>>
>> Thanks,
>> Artem.
>>
>
All the rest is adjusted in the new version of the patch you can find
in the attachment.
ChangLog
20011-09-06 Artjoms Sinkarovs
gcc/
* expr.c (do_store_flag): Expand vector comparison by
building an appropriate VEC_COND_EXPR.
* c-typeck.c (build_binary_op): Typecheck vector comparisons.
(c_objc_common_truthvalue_conversion): Adjust.
* tree-vect-generic.c (do_compare): Helper function.
(expand_vector_comparison): Check if hardware supports
vector comparison of the given type or expand vector
piecewise.
(expand_vector_operation): Treat comparison as binary
operation of vector type.
(expand_vector_operations_1): Adjust.
gcc/config/i386
* i386.c (ix86_expand_sse_movcc): Consider a case when
vcond operators are {-1,..} and {0,..}.
gcc/doc
* extend.texi: Adjust.
gcc/testsuite
* gcc.c-torture/execute/vector-compare-1.c: New test.
* gcc.c-torture/execute/vector-compare-2.c: New test.
* gcc.dg/vector-compare-1.c: New test.
* gcc.dg/vector-compare-2.c: New test.
bootstrapped and tested on x86_64-unknown-linux-gnu.
Index: gcc/doc/extend.texi
===
--- gcc/doc/extend.texi (revision 178579)
+++ gcc/doc/extend.texi (working copy)
@@ -6561,6 +6561,29 @@ invoke undefined behavior at runtime. W
accesses for vector subscription can be enabled with
@option{-Warray-bounds}.
+In GNU C vector comparison is supported within standard comparison
+operators: @code{==, !=, <, <=, >, >=}. Comparison operands can be
+vector expressions
Re: Vector Comparison patch
On Tue, Sep 6, 2011 at 4:50 PM, Artem Shinkarov
wrote:
> Here is a new version of the patch which considers the changes from
> 2011-09-02 Richard Guenther
>
>
> ChangeLog
>
> 20011-09-06 Artjoms Sinkarovs
>
> gcc/
> * fold-const.c (constant_boolean_node): Adjust the meaning
> of boolean for vector types: true = {-1,..}, false = {0,..}.
> (fold_unary_loc): Avoid conversion of vector comparison to
> boolean type.
Both changes have already been done.
> * expr.c (expand_expr_real_2): Expand vector comparison by
> building an appropriate VEC_COND_EXPR.
I prefer
Index: gcc/expr.c
===
*** gcc/expr.c.orig 2011-08-29 11:48:23.0 +0200
--- gcc/expr.c 2011-08-29 12:58:59.0 +0200
*** do_store_flag (sepops ops, rtx target, e
*** 10309,10314
--- 10309,10325
STRIP_NOPS (arg0);
STRIP_NOPS (arg1);
+ /* For vector typed comparisons emit code to generate the desired
+ all-ones or all-zeros mask. Conveniently use the VEC_COND_EXPR
+ expander for this. */
+ if (TREE_CODE (ops->type) == VECTOR_TYPE)
+ {
+ tree ifexp = build2 (ops->code, ops->type, arg0, arg1);
+ tree if_true = constant_boolean_node (true, ops->type);
+ tree if_false = constant_boolean_node (false, ops->type);
+ return expand_vec_cond_expr (ops->type, ifexp, if_true,
if_false, target);
+ }
+
/* Get the rtx comparison code to use. We know that EXP is a comparison
as I said multiple times.
> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
> (c_objc_common_truthvalue_conversion): Adjust.
> * tree-vect-generic.c (do_compare): Helper function.
> (expand_vector_comparison): Check if hardware supports
> vector comparison of the given type or expand vector
> piecewise.
> (expand_vector_operation): Treat comparison as binary
> operation of vector type.
> (expand_vector_operations_1): Adjust.
> * tree-cfg.c (verify_gimple_comparison): Adjust.
The tree-cfg.c change has already been done.
Richard.
>
> gcc/config/i386
> * i386.c (ix86_expand_sse_movcc): Consider a case when
> vcond operators are {-1,..} and {0,..}.
>
> gcc/doc
> * extend.texi: Adjust.
>
> gcc/testsuite
> * gcc.c-torture/execute/vector-compare-1.c: New test.
> * gcc.c-torture/execute/vector-compare-2.c: New test.
> * gcc.dg/vector-compare-1.c: New test.
> * gcc.dg/vector-compare-2.c: New test.
>
> bootstrapped and tested on x86_64-unknown-linux-gnu.
>
>
> Thanks,
> Artem.
>
Re: Vector Comparison patch
Here is a new version of the patch which considers the changes from
2011-09-02 Richard Guenther
ChangeLog
20011-09-06 Artjoms Sinkarovs
gcc/
* fold-const.c (constant_boolean_node): Adjust the meaning
of boolean for vector types: true = {-1,..}, false = {0,..}.
(fold_unary_loc): Avoid conversion of vector comparison to
boolean type.
* expr.c (expand_expr_real_2): Expand vector comparison by
building an appropriate VEC_COND_EXPR.
* c-typeck.c (build_binary_op): Typecheck vector comparisons.
(c_objc_common_truthvalue_conversion): Adjust.
* tree-vect-generic.c (do_compare): Helper function.
(expand_vector_comparison): Check if hardware supports
vector comparison of the given type or expand vector
piecewise.
(expand_vector_operation): Treat comparison as binary
operation of vector type.
(expand_vector_operations_1): Adjust.
* tree-cfg.c (verify_gimple_comparison): Adjust.
gcc/config/i386
* i386.c (ix86_expand_sse_movcc): Consider a case when
vcond operators are {-1,..} and {0,..}.
gcc/doc
* extend.texi: Adjust.
gcc/testsuite
* gcc.c-torture/execute/vector-compare-1.c: New test.
* gcc.c-torture/execute/vector-compare-2.c: New test.
* gcc.dg/vector-compare-1.c: New test.
* gcc.dg/vector-compare-2.c: New test.
bootstrapped and tested on x86_64-unknown-linux-gnu.
Thanks,
Artem.
Index: gcc/doc/extend.texi
===
--- gcc/doc/extend.texi (revision 178579)
+++ gcc/doc/extend.texi (working copy)
@@ -6561,6 +6561,29 @@ invoke undefined behavior at runtime. W
accesses for vector subscription can be enabled with
@option{-Warray-bounds}.
+In GNU C vector comparison is supported within standard comparison
+operators: @code{==, !=, <, <=, >, >=}. Comparison operands can be
+vector expressions of integer-type or real-type. Comparison between
+integer-type vectors and real-type vectors are not supported. The
+result of the comparison is a vector of the same width and number of
+elements as the comparison operands with a signed integral element
+type.
+
+Vectors are compared element-wise producing 0 when comparison is false
+and -1 (constant of the appropriate type where all bits are set)
+otherwise. Consider the following example.
+
+@smallexample
+typedef int v4si __attribute__ ((vector_size (16)));
+
+v4si a = @{1,2,3,4@};
+v4si b = @{3,2,1,4@};
+v4si c;
+
+c = a > b; /* The result would be @{0, 0,-1, 0@} */
+c = a == b; /* The result would be @{0,-1, 0,-1@} */
+@end smallexample
+
You can declare variables and use them in function calls and returns, as
well as in assignments and some casts. You can specify a vector type as
a return type for a function. Vector types can also be used as function
Index: gcc/fold-const.c
===
--- gcc/fold-const.c(revision 178579)
+++ gcc/fold-const.c(working copy)
@@ -5934,7 +5934,15 @@ extract_muldiv_1 (tree t, tree c, enum t
tree
constant_boolean_node (bool value, tree type)
{
- if (type == integer_type_node)
+ if (TREE_CODE (type) == VECTOR_TYPE)
+{
+ tree tval;
+
+ gcc_assert (TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE);
+ tval = build_int_cst (TREE_TYPE (type), value ? -1 : 0);
+ return build_vector_from_val (type, tval);
+}
+ else if (type == integer_type_node)
return value ? integer_one_node : integer_zero_node;
else if (type == boolean_type_node)
return value ? boolean_true_node : boolean_false_node;
@@ -7670,6 +7678,16 @@ fold_unary_loc (location_t loc, enum tre
return build2_loc (loc, TREE_CODE (op0), type,
TREE_OPERAND (op0, 0),
TREE_OPERAND (op0, 1));
+ else if (TREE_CODE (type) == VECTOR_TYPE)
+ {
+ tree el_type = TREE_TYPE (type);
+ tree op_el_type = TREE_TYPE (TREE_TYPE (op0));
+
+ if (el_type == op_el_type)
+ return op0;
+ else
+ build1_loc (loc, VIEW_CONVERT_EXPR, type, op0);
+ }
else if (!INTEGRAL_TYPE_P (type))
return build3_loc (loc, COND_EXPR, type, op0,
constant_boolean_node (true, type),
Index: gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c
===
--- gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c (revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c (revision 0)
@@ -0,0 +1,123 @@
+#define vector(elcount, type) \
+__attribute__((vector_size((elcount)*sizeof(type type
+
+#define check_compare(count, res, i0, i1, op, fmt) \
+do { \
+int __i; \
+for (__i = 0; __i < count; __i ++) { \
+ if ((res)[__i] != ((i0)[__i] op (i1)[__i]
Re: Vector Comparison patch
On Mon, Aug 29, 2011 at 2:09 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 9:50 PM, Uros Bizjak wrote:
>> On Mon, Aug 22, 2011 at 5:34 PM, Richard Guenther
>> wrote:
>>
In this case it is simple to analyse that a is a comparison, but you
cannot embed the operations of a into VEC_COND_EXPR.
>>>
>>> Sure, but if the above is C source the frontend would generate
>>> res = a != 0 ? v0 : v1; initially. An optimization pass could still
>>> track this information and replace VEC_COND_EXPR
>>> with VEC_COND_EXPR (no existing one would track
>>> vector contents though).
>>>
Ok, I am testing the patch that removes hooks. Could you push a little
bit the backend-patterns business?
>>>
>>> Well, I suppose we're waiting for Uros here. I hadn't much luck with
>>> fiddling with the mode-iterator stuff myself.
>>
>> It is not _that_ trivial change, since we have ix86_expand_fp_vcond
>> and ix86_expand_int_vcond to merge. ATM, FP version deals with FP
>> operands and vice versa. We have to merge them somehow and split out
>> comparison part that handles FP as well as integer operands.
>>
>> I also don't know why vcond is not allowed to FAIL... probably
>> middle-end should be enhanced for a fallback if some comparison isn't
>> supported by optab.
>
> I wonder, if we make vcond being able to FAIL (well, it would fail for
> invalid input only, like mismatching mode size), if patches along
>
> Index: gcc/config/i386/sse.md
> ===
> --- gcc/config/i386/sse.md (revision 178209)
> +++ gcc/config/i386/sse.md (working copy)
> @@ -1406,13 +1406,13 @@ (define_insn "_ucomi"
> (set_attr "mode" "")])
>
> (define_expand "vcond"
> - [(set (match_operand:VF 0 "register_operand" "")
> - (if_then_else:VF
> + [(set (match_operand 0 "register_operand" "")
> + (if_then_else
> (match_operator 3 ""
> [(match_operand:VF 4 "nonimmediate_operand" "")
> (match_operand:VF 5 "nonimmediate_operand" "")])
> - (match_operand:VF 1 "general_operand" "")
> - (match_operand:VF 2 "general_operand" "")))]
> + (match_operand 1 "general_operand" "")
> + (match_operand 2 "general_operand" "")))]
> "TARGET_SSE"
> {
> bool ok = ix86_expand_fp_vcond (operands);
>
> would be enough to make it accept V4SF < V4SF ? V4SI : V4SI with
> target mode V4SI. The expander code doesn't seem to care about
> the modes of op1/2 too much.
It at least "almost" works, apart from
/space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:122:1:
error: unrecognizable insn:^M
(insn 1813 1812 1814 255 (set (reg:V4SI 1238)^M
(lt:V4SI (reg:V4SF 1236)^M
(reg:V4SF 619 [ D.3579 ])))
/space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:109
-1^M
(nil))^M
/space/rguenther/src/svn/trunk/gcc/testsuite/gcc.c-torture/execute/vector-compare-1.c:122:1:
internal compiler error: in extract_insn, at recog.c:2115
I suppose the compare patterns need similar adjustments (though I
couldn't find any SSE lt one, but that may be an artifact).
Modeless operands are warned on though - do we have something better
to simulate the effect? Like a mode mapper that does a 1:1 translation
of an input mode from a mode iterator to another one? Can we
use define_mode_attr for this? Like
Index: config/i386/sse.md
===
--- config/i386/sse.md (revision 178209)
+++ config/i386/sse.md (working copy)
@@ -161,6 +161,9 @@ (define_mode_attr avx_avx2
(V4SI "avx2") (V2DI "avx2")
(V8SI "avx2") (V4DI "avx2")])
+(define_mode_attr cmpmode
+ [(V8SF "V8SI") (V4SF "V4SI") (V4DF "V4DI") (V2DF "V2DI")])
+
;; Mapping of logic-shift operators
(define_code_iterator lshift [lshiftrt ashift])
@@ -1348,9 +1351,9 @@ (define_insn "_maskcmp3"
(set_attr "mode" "")])
(define_insn "_vmmaskcmp3"
- [(set (match_operand:VF_128 0 "register_operand" "=x,x")
- (vec_merge:VF_128
-(match_operator:VF_128 3 "sse_comparison_operator"
+ [(set (match_operand: 0 "register_operand" "=x,x")
+ (vec_merge:
+(match_operator: 3 "sse_comparison_operator"
[(match_operand:VF_128 1 "register_operand" "0,x")
(match_operand:VF_128 2 "nonimmediate_operand" "xm,xm")])
(match_dup 1)
@@ -1406,13 +1409,13 @@ (define_insn "_ucomi"
(set_attr "mode" "")])
(define_expand "vcond"
- [(set (match_operand:VF 0 "register_operand" "")
- (if_then_else:VF
- (match_operator 3 ""
+ [(set (match_operand 0 "register_operand" "")
+ (if_then_else
+ (match_operator: 3 ""
[(match_operand:VF 4 "nonimmediate_operand" "")
(match_operand:VF 5 "nonimmediate_operand" "")])
- (match_operand:VF 1 "general_operand" "")
- (match_operand:VF 2 "general_operand" "")))]
+ (match_operand 1 "general_operand" ""
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 9:50 PM, Uros Bizjak wrote:
> On Mon, Aug 22, 2011 at 5:34 PM, Richard Guenther
> wrote:
>
>>> In this case it is simple to analyse that a is a comparison, but you
>>> cannot embed the operations of a into VEC_COND_EXPR.
>>
>> Sure, but if the above is C source the frontend would generate
>> res = a != 0 ? v0 : v1; initially. An optimization pass could still
>> track this information and replace VEC_COND_EXPR
>> with VEC_COND_EXPR (no existing one would track
>> vector contents though).
>>
>>> Ok, I am testing the patch that removes hooks. Could you push a little
>>> bit the backend-patterns business?
>>
>> Well, I suppose we're waiting for Uros here. I hadn't much luck with
>> fiddling with the mode-iterator stuff myself.
>
> It is not _that_ trivial change, since we have ix86_expand_fp_vcond
> and ix86_expand_int_vcond to merge. ATM, FP version deals with FP
> operands and vice versa. We have to merge them somehow and split out
> comparison part that handles FP as well as integer operands.
>
> I also don't know why vcond is not allowed to FAIL... probably
> middle-end should be enhanced for a fallback if some comparison isn't
> supported by optab.
I wonder, if we make vcond being able to FAIL (well, it would fail for
invalid input only, like mismatching mode size), if patches along
Index: gcc/config/i386/sse.md
===
--- gcc/config/i386/sse.md (revision 178209)
+++ gcc/config/i386/sse.md (working copy)
@@ -1406,13 +1406,13 @@ (define_insn "_ucomi"
(set_attr "mode" "")])
(define_expand "vcond"
- [(set (match_operand:VF 0 "register_operand" "")
- (if_then_else:VF
+ [(set (match_operand 0 "register_operand" "")
+ (if_then_else
(match_operator 3 ""
[(match_operand:VF 4 "nonimmediate_operand" "")
(match_operand:VF 5 "nonimmediate_operand" "")])
- (match_operand:VF 1 "general_operand" "")
- (match_operand:VF 2 "general_operand" "")))]
+ (match_operand 1 "general_operand" "")
+ (match_operand 2 "general_operand" "")))]
"TARGET_SSE"
{
bool ok = ix86_expand_fp_vcond (operands);
would be enough to make it accept V4SF < V4SF ? V4SI : V4SI with
target mode V4SI. The expander code doesn't seem to care about
the modes of op1/2 too much.
Richard.
> Uros.
>
Re: Vector Comparison patch
On 08/18/2011 11:23 AM, Richard Guenther wrote:
Yeah, well. That's really a question for language lawyers;) I agree
that it would be nice to have mask ? val0 : val1 behave "the same"
for scalars and vectors. The question is whether for vectors you
define it on the bit-level (which makes it equal to (mask& val0) |
(~mask& val1))
or on the vector component level. The vector component level
is probably what people would expect.
Which means we have to treat mask ? val0 : val1 as
mask != {0,...} ? val0 : val1.
The definition in OpenCL makes zero sense to me. For byte operands it
is custom-tailored after the SSE PMOVMSKB instruction, but there is no
PMOVMSKW/PMOVMSKD instruction so you would need very slow bit shift
operations before PMOVMSK. On the other hand, bit selection is for
example in Altivec.
Do we have some way to contact anyone in the OpenCL standards group
(CCing Chris Lattner)?
If you wanted to implement it, it would be mask < {0,...} ? val0 : val1.
But really, since we're not implementing OpenCL C I would really
prefer to have bit-level selection, and let a front-end implement the quirk.
Paolo
Re: Vector Comparison patch
On Sat, Aug 27, 2011 at 3:39 AM, Artem Shinkarov
wrote:
> Hi
>
> Here is a patch with vector comparison only.
> Comparison is expanded using VEC_COND_EXPR, conversions between the
> different types inside the VEC_COND_EXPR are happening in optabs.c.
I have split out the middle-end infrastructure parts to support vector
comparisons apart from the expansion piece and am testing this
(see attached, I adjusted some minor bits). I will commit this if
testing goes ok.
Looking over the rest I wonder why you need to avoid legitimizing stuff
in vector_compare_rtx? I can't produce any error with x86_64 or i586,
but on i586 gcc.c-torture/execute/vector-compare-1.c does not build
because
/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
so the testcases need double-checking for this kind of errors. You
can run tests for both -m32 and -m64 with a command-line like
make check-gcc RUNTESTFLAGS="--target_board=unix/\{,-m32\}
dg.exp=vector-compare*.c"
I'd like to further split the optabs.c and expr.c change which look
independent.
I have the attached incremental patch ontop of yours, I will test the
expr.c and optabs.c parts separately and plan to commit them as well
if that succeeds.
Richard.
> The comparison generally works, however, the x86 backend does not
> recognize vectors of all 1s of type float and double, which is very
> bad, but I hope it could be fixed easily. Here is my humble attempt:
>
> Index: gcc/config/i386/predicates.md
> ===
> --- gcc/config/i386/predicates.md (revision 177665)
> +++ gcc/config/i386/predicates.md (working copy)
> @@ -763,7 +763,19 @@ (define_predicate "vector_all_ones_opera
> for (i = 0; i < nunits; ++i)
> {
> rtx x = CONST_VECTOR_ELT (op, i);
> - if (x != constm1_rtx)
> + rtx y;
> +
> + if (GET_MODE_CLASS (GET_MODE (x)) == MODE_FLOAT)
> + {
> + REAL_VALUE_TYPE r;
> + REAL_VALUE_FROM_INT (r, -1, -1, GET_MODE (x));
> + y = CONST_DOUBLE_FROM_REAL_VALUE (r, GET_MODE (x));
> + }
> + else
> + y = constm1_rtx;
> +
> + /* if (x != constm1_rtx) */
> + if (!rtx_equal_p (x, y))
> return false;
> }
> return true;
>
> But the problem I have here is that -1 actually converts to -1.0,
> where I need to treat -0x1 as float. Something like:
>
> int p = -1;
> void *x = &p;
> float r = *((float *)x);
>
> Is there any way to do that in this context? Or may be there is
> another way to support real-typed vectors of -1 as constants?
>
>
> ChangeLog
>
> 20011-08-27 Artjoms Sinkarovs
>
> gcc/
> * optabs.c (vector_compare_rtx): Allow comparison operands
> and vcond operands have different type.
> (expand_vec_cond_expr): Convert operands in case they do
> not match.
> * fold-const.c (constant_boolean_node): Adjust the meaning
> of boolean for vector types: true = {-1,..}, false = {0,..}.
> (fold_unary_loc): Avoid conversion of vector comparison to
> boolean type.
> * expr.c (expand_expr_real_2): Expand vector comparison by
> building an appropriate VEC_COND_EXPR.
> * c-typeck.c (build_binary_op): Typecheck vector comparisons.
> (c_objc_common_truthvalue_conversion): Adjust.
> * gimplify.c (gimplify_expr): Support vector comparison
> in gimple.
> * tree.def: Adjust comment.
> * tree-vect-generic.c (do_compare): Helper function.
> (expand_vector_comparison): Check if hardware supports
> vector comparison of the given type or expand vector
> piecewise.
> (expand_vector_operation): Treat comparison as binary
> operation of vector type.
> (expand_vector_operations_1): Adjust.
> * tree-cfg.c (verify_gimple_comparison): Adjust.
>
> gcc/config/i386
> * i386.c (ix86_expand_sse_movcc): Consider a case when
> vcond operators are {-1,..} and {0,..}.
>
> gcc/doc
> * extend.texi: Adjust.
>
> gcc/testsuite
> * gcc.c-torture/execute/vector-compare-1.c: New test.
> * gcc.c-torture/execute/vector-compare-2.c: New test.
> * gcc.dg/vector-compare-1.c: New test.
> * gcc.dg/vector-compare-2.c: New test.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>
>
> Artem.
>
vec-compare.v7.diff.r
Description: Binary data
p
Description: Binary data
Re: Vector Comparison patch
Hi
Here is a patch with vector comparison only.
Comparison is expanded using VEC_COND_EXPR, conversions between the
different types inside the VEC_COND_EXPR are happening in optabs.c.
The comparison generally works, however, the x86 backend does not
recognize vectors of all 1s of type float and double, which is very
bad, but I hope it could be fixed easily. Here is my humble attempt:
Index: gcc/config/i386/predicates.md
===
--- gcc/config/i386/predicates.md (revision 177665)
+++ gcc/config/i386/predicates.md (working copy)
@@ -763,7 +763,19 @@ (define_predicate "vector_all_ones_opera
for (i = 0; i < nunits; ++i)
{
rtx x = CONST_VECTOR_ELT (op, i);
- if (x != constm1_rtx)
+ rtx y;
+
+ if (GET_MODE_CLASS (GET_MODE (x)) == MODE_FLOAT)
+ {
+ REAL_VALUE_TYPE r;
+ REAL_VALUE_FROM_INT (r, -1, -1, GET_MODE (x));
+ y = CONST_DOUBLE_FROM_REAL_VALUE (r, GET_MODE (x));
+ }
+ else
+ y = constm1_rtx;
+
+ /* if (x != constm1_rtx) */
+ if (!rtx_equal_p (x, y))
return false;
}
return true;
But the problem I have here is that -1 actually converts to -1.0,
where I need to treat -0x1 as float. Something like:
int p = -1;
void *x = &p;
float r = *((float *)x);
Is there any way to do that in this context? Or may be there is
another way to support real-typed vectors of -1 as constants?
ChangeLog
20011-08-27 Artjoms Sinkarovs
gcc/
* optabs.c (vector_compare_rtx): Allow comparison operands
and vcond operands have different type.
(expand_vec_cond_expr): Convert operands in case they do
not match.
* fold-const.c (constant_boolean_node): Adjust the meaning
of boolean for vector types: true = {-1,..}, false = {0,..}.
(fold_unary_loc): Avoid conversion of vector comparison to
boolean type.
* expr.c (expand_expr_real_2): Expand vector comparison by
building an appropriate VEC_COND_EXPR.
* c-typeck.c (build_binary_op): Typecheck vector comparisons.
(c_objc_common_truthvalue_conversion): Adjust.
* gimplify.c (gimplify_expr): Support vector comparison
in gimple.
* tree.def: Adjust comment.
* tree-vect-generic.c (do_compare): Helper function.
(expand_vector_comparison): Check if hardware supports
vector comparison of the given type or expand vector
piecewise.
(expand_vector_operation): Treat comparison as binary
operation of vector type.
(expand_vector_operations_1): Adjust.
* tree-cfg.c (verify_gimple_comparison): Adjust.
gcc/config/i386
* i386.c (ix86_expand_sse_movcc): Consider a case when
vcond operators are {-1,..} and {0,..}.
gcc/doc
* extend.texi: Adjust.
gcc/testsuite
* gcc.c-torture/execute/vector-compare-1.c: New test.
* gcc.c-torture/execute/vector-compare-2.c: New test.
* gcc.dg/vector-compare-1.c: New test.
* gcc.dg/vector-compare-2.c: New test.
Bootstrapped and tested on x86_64-unknown-linux-gnu.
Artem.
Index: gcc/doc/extend.texi
===
--- gcc/doc/extend.texi (revision 177665)
+++ gcc/doc/extend.texi (working copy)
@@ -6553,6 +6553,29 @@ invoke undefined behavior at runtime. W
accesses for vector subscription can be enabled with
@option{-Warray-bounds}.
+In GNU C vector comparison is supported within standard comparison
+operators: @code{==, !=, <, <=, >, >=}. Comparison operands can be
+vector expressions of integer-type or real-type. Comparison between
+integer-type vectors and real-type vectors are not supported. The
+result of the comparison is a vector of the same width and number of
+elements as the comparison operands with a signed integral element
+type.
+
+Vectors are compared element-wise producing 0 when comparison is false
+and -1 (constant of the appropriate type where all bits are set)
+otherwise. Consider the following example.
+
+@smallexample
+typedef int v4si __attribute__ ((vector_size (16)));
+
+v4si a = @{1,2,3,4@};
+v4si b = @{3,2,1,4@};
+v4si c;
+
+c = a > b; /* The result would be @{0, 0,-1, 0@} */
+c = a == b; /* The result would be @{0,-1, 0,-1@} */
+@end smallexample
+
You can declare variables and use them in function calls and returns, as
well as in assignments and some casts. You can specify a vector type as
a return type for a function. Vector types can also be used as function
Index: gcc/optabs.c
===
--- gcc/optabs.c(revision 177665)
+++ gcc/optabs.c(working copy)
@@ -6502,7 +6502,8 @@ get_rtx_code (enum tree_code tcode, bool
unsigned operators. Do not generate compare instruction. */
static rtx
-vect
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 3:15 PM, Artem Shinkarov
wrote:
> On Thu, Aug 25, 2011 at 2:00 PM, Richard Guenther
> wrote:
>> On Thu, Aug 25, 2011 at 2:45 PM, Artem Shinkarov
>> wrote:
>>> On Thu, Aug 25, 2011 at 12:39 PM, Richard Guenther
>>> wrote:
On Thu, Aug 25, 2011 at 1:07 PM, Artem Shinkarov
wrote:
> On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
> wrote:
>> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
>> wrote:
>>> Here is a cleaned-up patch without the hook. Mostly it works in a way
>>> we discussed.
>>>
>>> So I think it is a right time to do something about vcond patterns,
>>> which would allow me to get rid of conversions that I need to put all
>>> over the code.
>>>
>>> Also at the moment the patch breaks lto frontend with a simple example:
>>> #define vector(elcount, type) \
>>> __attribute__((vector_size((elcount)*sizeof(type type
>>>
>>> int main (int argc, char *argv[]) {
>>> vector (4, float) f0;
>>> vector (4, float) f1;
>>>
>>> f0 = f1 != f0
>>> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4,
>>> float)){0,0,0,0};
>>>
>>> return (int)f0[argc];
>>> }
>>>
>>> test-lto.c:8:14: internal compiler error: in convert, at
>>> lto/lto-lang.c:1244
>>>
>>> I looked into the file, the conversion function is defined as
>>> gcc_unreachable (). I am not very familiar with lto, so I don't really
>>> know what is the right way to treat the conversions.
>>>
>>> And I seriously need help with backend patterns.
>>
>> On the patch.
>>
>> The documentation needs review by a native english speaker, but here
>> are some factual comments:
>>
>> +In C vector comparison is supported within standard comparison
>> operators:
>>
>> it should read 'In GNU C' here and everywhere else as this is a GNU
>> extension.
>>
>> The result of the
>> +comparison is a signed integer-type vector where the size of each
>> +element must be the same as the size of compared vectors element.
>>
>> The result type of the comparison is determined by the C frontend,
>> it isn't under control of the user. What you are implying here is
>> restrictions on vector assignments, which are documented elsewhere.
>> I'd just say
>>
>> 'The result of the comparison is a vector of the same width and number
>> of elements as the comparison operands with a signed integral element
>> type.'
>>
>> +In addition to the vector comparison C supports conditional expressions
>>
>> See above.
>>
>> +For the convenience condition in the vector conditional can be just a
>> +vector of signed integer type.
>>
>> 'of integer type.' I don't see a reason to disallow unsigned integers,
>> they can be equally well compared against zero.
>
> I'll have a final go on the documentation, it is untouched from the old
> patches.
>
>> Index: gcc/targhooks.h
>> ===
>> --- gcc/targhooks.h (revision 177665)
>> +++ gcc/targhooks.h (working copy)
>> @@ -86,6 +86,7 @@ extern int default_builtin_vectorization
>> extern tree default_builtin_reciprocal (unsigned int, bool, bool);
>>
>> extern bool default_builtin_vector_alignment_reachable (const_tree,
>> bool);
>> +
>> extern bool
>> default_builtin_support_vector_misalignment (enum machine_mode mode,
>> const_tree,
>>
>> spurious whitespace change.
>
> Yes, thanks.
>
>> Index: gcc/optabs.c
>> ===
>> --- gcc/optabs.c (revision 177665)
>> +++ gcc/optabs.c (working copy)
>> @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
>> ...
>> + else
>> + {
>> + rtx rtx_op0;
>> + rtx vec;
>> +
>> + rtx_op0 = expand_normal (op0);
>> + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
>> + vec = CONST0_RTX (mode);
>> +
>> + create_output_operand (&ops[0], target, mode);
>> + create_input_operand (&ops[1], rtx_op1, mode);
>> + create_input_operand (&ops[2], rtx_op2, mode);
>> + create_input_operand (&ops[3], comparison, mode);
>> + create_input_operand (&ops[4], rtx_op0, mode);
>> + create_input_operand (&ops[5], vec, mode);
>>
>> this still builds the fake(?) != comparison, but as you said you need
>> help
>> with the .md part if we want to use a machine specific pattern for this
>> case (which we eventually want, for the sake of using XOP vcond).
>
> Yes, I am waiting for it. This is the only way at the moment to make
> sure that in
> m = a > b;
> r =
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 2:00 PM, Richard Guenther
wrote:
> On Thu, Aug 25, 2011 at 2:45 PM, Artem Shinkarov
> wrote:
>> On Thu, Aug 25, 2011 at 12:39 PM, Richard Guenther
>> wrote:
>>> On Thu, Aug 25, 2011 at 1:07 PM, Artem Shinkarov
>>> wrote:
On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
wrote:
> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
> wrote:
>> Here is a cleaned-up patch without the hook. Mostly it works in a way
>> we discussed.
>>
>> So I think it is a right time to do something about vcond patterns,
>> which would allow me to get rid of conversions that I need to put all
>> over the code.
>>
>> Also at the moment the patch breaks lto frontend with a simple example:
>> #define vector(elcount, type) \
>> __attribute__((vector_size((elcount)*sizeof(type type
>>
>> int main (int argc, char *argv[]) {
>> vector (4, float) f0;
>> vector (4, float) f1;
>>
>> f0 = f1 != f0
>> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4,
>> float)){0,0,0,0};
>>
>> return (int)f0[argc];
>> }
>>
>> test-lto.c:8:14: internal compiler error: in convert, at
>> lto/lto-lang.c:1244
>>
>> I looked into the file, the conversion function is defined as
>> gcc_unreachable (). I am not very familiar with lto, so I don't really
>> know what is the right way to treat the conversions.
>>
>> And I seriously need help with backend patterns.
>
> On the patch.
>
> The documentation needs review by a native english speaker, but here
> are some factual comments:
>
> +In C vector comparison is supported within standard comparison operators:
>
> it should read 'In GNU C' here and everywhere else as this is a GNU
> extension.
>
> The result of the
> +comparison is a signed integer-type vector where the size of each
> +element must be the same as the size of compared vectors element.
>
> The result type of the comparison is determined by the C frontend,
> it isn't under control of the user. What you are implying here is
> restrictions on vector assignments, which are documented elsewhere.
> I'd just say
>
> 'The result of the comparison is a vector of the same width and number
> of elements as the comparison operands with a signed integral element
> type.'
>
> +In addition to the vector comparison C supports conditional expressions
>
> See above.
>
> +For the convenience condition in the vector conditional can be just a
> +vector of signed integer type.
>
> 'of integer type.' I don't see a reason to disallow unsigned integers,
> they can be equally well compared against zero.
I'll have a final go on the documentation, it is untouched from the old
patches.
> Index: gcc/targhooks.h
> ===
> --- gcc/targhooks.h (revision 177665)
> +++ gcc/targhooks.h (working copy)
> @@ -86,6 +86,7 @@ extern int default_builtin_vectorization
> extern tree default_builtin_reciprocal (unsigned int, bool, bool);
>
> extern bool default_builtin_vector_alignment_reachable (const_tree,
> bool);
> +
> extern bool
> default_builtin_support_vector_misalignment (enum machine_mode mode,
> const_tree,
>
> spurious whitespace change.
Yes, thanks.
> Index: gcc/optabs.c
> ===
> --- gcc/optabs.c (revision 177665)
> +++ gcc/optabs.c (working copy)
> @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
> ...
> + else
> + {
> + rtx rtx_op0;
> + rtx vec;
> +
> + rtx_op0 = expand_normal (op0);
> + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
> + vec = CONST0_RTX (mode);
> +
> + create_output_operand (&ops[0], target, mode);
> + create_input_operand (&ops[1], rtx_op1, mode);
> + create_input_operand (&ops[2], rtx_op2, mode);
> + create_input_operand (&ops[3], comparison, mode);
> + create_input_operand (&ops[4], rtx_op0, mode);
> + create_input_operand (&ops[5], vec, mode);
>
> this still builds the fake(?) != comparison, but as you said you need help
> with the .md part if we want to use a machine specific pattern for this
> case (which we eventually want, for the sake of using XOP vcond).
Yes, I am waiting for it. This is the only way at the moment to make
sure that in
m = a > b;
r = m ? c : d;
m in the vcond is not transformed into the m != 0.
> Index: gcc/target.h
> ===
> --- gcc/target.h
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 2:45 PM, Artem Shinkarov
wrote:
> On Thu, Aug 25, 2011 at 12:39 PM, Richard Guenther
> wrote:
>> On Thu, Aug 25, 2011 at 1:07 PM, Artem Shinkarov
>> wrote:
>>> On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
>>> wrote:
On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
wrote:
> Here is a cleaned-up patch without the hook. Mostly it works in a way
> we discussed.
>
> So I think it is a right time to do something about vcond patterns,
> which would allow me to get rid of conversions that I need to put all
> over the code.
>
> Also at the moment the patch breaks lto frontend with a simple example:
> #define vector(elcount, type) \
> __attribute__((vector_size((elcount)*sizeof(type type
>
> int main (int argc, char *argv[]) {
> vector (4, float) f0;
> vector (4, float) f1;
>
> f0 = f1 != f0
> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4,
> float)){0,0,0,0};
>
> return (int)f0[argc];
> }
>
> test-lto.c:8:14: internal compiler error: in convert, at
> lto/lto-lang.c:1244
>
> I looked into the file, the conversion function is defined as
> gcc_unreachable (). I am not very familiar with lto, so I don't really
> know what is the right way to treat the conversions.
>
> And I seriously need help with backend patterns.
On the patch.
The documentation needs review by a native english speaker, but here
are some factual comments:
+In C vector comparison is supported within standard comparison operators:
it should read 'In GNU C' here and everywhere else as this is a GNU
extension.
The result of the
+comparison is a signed integer-type vector where the size of each
+element must be the same as the size of compared vectors element.
The result type of the comparison is determined by the C frontend,
it isn't under control of the user. What you are implying here is
restrictions on vector assignments, which are documented elsewhere.
I'd just say
'The result of the comparison is a vector of the same width and number
of elements as the comparison operands with a signed integral element
type.'
+In addition to the vector comparison C supports conditional expressions
See above.
+For the convenience condition in the vector conditional can be just a
+vector of signed integer type.
'of integer type.' I don't see a reason to disallow unsigned integers,
they can be equally well compared against zero.
>>>
>>> I'll have a final go on the documentation, it is untouched from the old
>>> patches.
>>>
Index: gcc/targhooks.h
===
--- gcc/targhooks.h (revision 177665)
+++ gcc/targhooks.h (working copy)
@@ -86,6 +86,7 @@ extern int default_builtin_vectorization
extern tree default_builtin_reciprocal (unsigned int, bool, bool);
extern bool default_builtin_vector_alignment_reachable (const_tree, bool);
+
extern bool
default_builtin_support_vector_misalignment (enum machine_mode mode,
const_tree,
spurious whitespace change.
>>>
>>> Yes, thanks.
>>>
Index: gcc/optabs.c
===
--- gcc/optabs.c (revision 177665)
+++ gcc/optabs.c (working copy)
@@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
...
+ else
+ {
+ rtx rtx_op0;
+ rtx vec;
+
+ rtx_op0 = expand_normal (op0);
+ comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
+ vec = CONST0_RTX (mode);
+
+ create_output_operand (&ops[0], target, mode);
+ create_input_operand (&ops[1], rtx_op1, mode);
+ create_input_operand (&ops[2], rtx_op2, mode);
+ create_input_operand (&ops[3], comparison, mode);
+ create_input_operand (&ops[4], rtx_op0, mode);
+ create_input_operand (&ops[5], vec, mode);
this still builds the fake(?) != comparison, but as you said you need help
with the .md part if we want to use a machine specific pattern for this
case (which we eventually want, for the sake of using XOP vcond).
>>>
>>> Yes, I am waiting for it. This is the only way at the moment to make
>>> sure that in
>>> m = a > b;
>>> r = m ? c : d;
>>>
>>> m in the vcond is not transformed into the m != 0.
>>>
Index: gcc/target.h
===
--- gcc/target.h (revision 177665)
+++ gcc/target.h (working copy)
@@ -51,6 +51,7 @@
#define GCC_TARGET_H
#include "insn-modes.h"
+#include "gimple.h"
#ifdef
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 12:39 PM, Richard Guenther
wrote:
> On Thu, Aug 25, 2011 at 1:07 PM, Artem Shinkarov
> wrote:
>> On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
>> wrote:
>>> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
>>> wrote:
Here is a cleaned-up patch without the hook. Mostly it works in a way
we discussed.
So I think it is a right time to do something about vcond patterns,
which would allow me to get rid of conversions that I need to put all
over the code.
Also at the moment the patch breaks lto frontend with a simple example:
#define vector(elcount, type) \
__attribute__((vector_size((elcount)*sizeof(type type
int main (int argc, char *argv[]) {
vector (4, float) f0;
vector (4, float) f1;
f0 = f1 != f0
? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
return (int)f0[argc];
}
test-lto.c:8:14: internal compiler error: in convert, at
lto/lto-lang.c:1244
I looked into the file, the conversion function is defined as
gcc_unreachable (). I am not very familiar with lto, so I don't really
know what is the right way to treat the conversions.
And I seriously need help with backend patterns.
>>>
>>> On the patch.
>>>
>>> The documentation needs review by a native english speaker, but here
>>> are some factual comments:
>>>
>>> +In C vector comparison is supported within standard comparison operators:
>>>
>>> it should read 'In GNU C' here and everywhere else as this is a GNU
>>> extension.
>>>
>>> The result of the
>>> +comparison is a signed integer-type vector where the size of each
>>> +element must be the same as the size of compared vectors element.
>>>
>>> The result type of the comparison is determined by the C frontend,
>>> it isn't under control of the user. What you are implying here is
>>> restrictions on vector assignments, which are documented elsewhere.
>>> I'd just say
>>>
>>> 'The result of the comparison is a vector of the same width and number
>>> of elements as the comparison operands with a signed integral element
>>> type.'
>>>
>>> +In addition to the vector comparison C supports conditional expressions
>>>
>>> See above.
>>>
>>> +For the convenience condition in the vector conditional can be just a
>>> +vector of signed integer type.
>>>
>>> 'of integer type.' I don't see a reason to disallow unsigned integers,
>>> they can be equally well compared against zero.
>>
>> I'll have a final go on the documentation, it is untouched from the old
>> patches.
>>
>>> Index: gcc/targhooks.h
>>> ===
>>> --- gcc/targhooks.h (revision 177665)
>>> +++ gcc/targhooks.h (working copy)
>>> @@ -86,6 +86,7 @@ extern int default_builtin_vectorization
>>> extern tree default_builtin_reciprocal (unsigned int, bool, bool);
>>>
>>> extern bool default_builtin_vector_alignment_reachable (const_tree, bool);
>>> +
>>> extern bool
>>> default_builtin_support_vector_misalignment (enum machine_mode mode,
>>> const_tree,
>>>
>>> spurious whitespace change.
>>
>> Yes, thanks.
>>
>>> Index: gcc/optabs.c
>>> ===
>>> --- gcc/optabs.c (revision 177665)
>>> +++ gcc/optabs.c (working copy)
>>> @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
>>> ...
>>> + else
>>> + {
>>> + rtx rtx_op0;
>>> + rtx vec;
>>> +
>>> + rtx_op0 = expand_normal (op0);
>>> + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
>>> + vec = CONST0_RTX (mode);
>>> +
>>> + create_output_operand (&ops[0], target, mode);
>>> + create_input_operand (&ops[1], rtx_op1, mode);
>>> + create_input_operand (&ops[2], rtx_op2, mode);
>>> + create_input_operand (&ops[3], comparison, mode);
>>> + create_input_operand (&ops[4], rtx_op0, mode);
>>> + create_input_operand (&ops[5], vec, mode);
>>>
>>> this still builds the fake(?) != comparison, but as you said you need help
>>> with the .md part if we want to use a machine specific pattern for this
>>> case (which we eventually want, for the sake of using XOP vcond).
>>
>> Yes, I am waiting for it. This is the only way at the moment to make
>> sure that in
>> m = a > b;
>> r = m ? c : d;
>>
>> m in the vcond is not transformed into the m != 0.
>>
>>> Index: gcc/target.h
>>> ===
>>> --- gcc/target.h (revision 177665)
>>> +++ gcc/target.h (working copy)
>>> @@ -51,6 +51,7 @@
>>> #define GCC_TARGET_H
>>>
>>> #include "insn-modes.h"
>>> +#include "gimple.h"
>>>
>>> #ifdef ENABLE_CHECKING
>>>
>>> spurious change.
>>
>> Old stuff, fixed.
>>
>>> @@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr
>>> floating-point, we can only do some of these simplifica
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 1:07 PM, Artem Shinkarov
wrote:
> On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
> wrote:
>> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
>> wrote:
>>> Here is a cleaned-up patch without the hook. Mostly it works in a way
>>> we discussed.
>>>
>>> So I think it is a right time to do something about vcond patterns,
>>> which would allow me to get rid of conversions that I need to put all
>>> over the code.
>>>
>>> Also at the moment the patch breaks lto frontend with a simple example:
>>> #define vector(elcount, type) \
>>> __attribute__((vector_size((elcount)*sizeof(type type
>>>
>>> int main (int argc, char *argv[]) {
>>> vector (4, float) f0;
>>> vector (4, float) f1;
>>>
>>> f0 = f1 != f0
>>> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
>>>
>>> return (int)f0[argc];
>>> }
>>>
>>> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244
>>>
>>> I looked into the file, the conversion function is defined as
>>> gcc_unreachable (). I am not very familiar with lto, so I don't really
>>> know what is the right way to treat the conversions.
>>>
>>> And I seriously need help with backend patterns.
>>
>> On the patch.
>>
>> The documentation needs review by a native english speaker, but here
>> are some factual comments:
>>
>> +In C vector comparison is supported within standard comparison operators:
>>
>> it should read 'In GNU C' here and everywhere else as this is a GNU
>> extension.
>>
>> The result of the
>> +comparison is a signed integer-type vector where the size of each
>> +element must be the same as the size of compared vectors element.
>>
>> The result type of the comparison is determined by the C frontend,
>> it isn't under control of the user. What you are implying here is
>> restrictions on vector assignments, which are documented elsewhere.
>> I'd just say
>>
>> 'The result of the comparison is a vector of the same width and number
>> of elements as the comparison operands with a signed integral element
>> type.'
>>
>> +In addition to the vector comparison C supports conditional expressions
>>
>> See above.
>>
>> +For the convenience condition in the vector conditional can be just a
>> +vector of signed integer type.
>>
>> 'of integer type.' I don't see a reason to disallow unsigned integers,
>> they can be equally well compared against zero.
>
> I'll have a final go on the documentation, it is untouched from the old
> patches.
>
>> Index: gcc/targhooks.h
>> ===
>> --- gcc/targhooks.h (revision 177665)
>> +++ gcc/targhooks.h (working copy)
>> @@ -86,6 +86,7 @@ extern int default_builtin_vectorization
>> extern tree default_builtin_reciprocal (unsigned int, bool, bool);
>>
>> extern bool default_builtin_vector_alignment_reachable (const_tree, bool);
>> +
>> extern bool
>> default_builtin_support_vector_misalignment (enum machine_mode mode,
>> const_tree,
>>
>> spurious whitespace change.
>
> Yes, thanks.
>
>> Index: gcc/optabs.c
>> ===
>> --- gcc/optabs.c (revision 177665)
>> +++ gcc/optabs.c (working copy)
>> @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
>> ...
>> + else
>> + {
>> + rtx rtx_op0;
>> + rtx vec;
>> +
>> + rtx_op0 = expand_normal (op0);
>> + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
>> + vec = CONST0_RTX (mode);
>> +
>> + create_output_operand (&ops[0], target, mode);
>> + create_input_operand (&ops[1], rtx_op1, mode);
>> + create_input_operand (&ops[2], rtx_op2, mode);
>> + create_input_operand (&ops[3], comparison, mode);
>> + create_input_operand (&ops[4], rtx_op0, mode);
>> + create_input_operand (&ops[5], vec, mode);
>>
>> this still builds the fake(?) != comparison, but as you said you need help
>> with the .md part if we want to use a machine specific pattern for this
>> case (which we eventually want, for the sake of using XOP vcond).
>
> Yes, I am waiting for it. This is the only way at the moment to make
> sure that in
> m = a > b;
> r = m ? c : d;
>
> m in the vcond is not transformed into the m != 0.
>
>> Index: gcc/target.h
>> ===
>> --- gcc/target.h (revision 177665)
>> +++ gcc/target.h (working copy)
>> @@ -51,6 +51,7 @@
>> #define GCC_TARGET_H
>>
>> #include "insn-modes.h"
>> +#include "gimple.h"
>>
>> #ifdef ENABLE_CHECKING
>>
>> spurious change.
>
> Old stuff, fixed.
>
>> @@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr
>> floating-point, we can only do some of these simplifications.) */
>> if (operand_equal_p (arg0, arg1, 0))
>> {
>> + tree arg0_type = TREE_TYPE (arg0);
>> +
>> switch (code)
>> {
>> case EQ_EXPR:
>> - if (! FLOAT_TYPE_P (T
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther
wrote:
> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
> wrote:
>> Here is a cleaned-up patch without the hook. Mostly it works in a way
>> we discussed.
>>
>> So I think it is a right time to do something about vcond patterns,
>> which would allow me to get rid of conversions that I need to put all
>> over the code.
>>
>> Also at the moment the patch breaks lto frontend with a simple example:
>> #define vector(elcount, type) \
>> __attribute__((vector_size((elcount)*sizeof(type type
>>
>> int main (int argc, char *argv[]) {
>> vector (4, float) f0;
>> vector (4, float) f1;
>>
>> f0 = f1 != f0
>> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
>>
>> return (int)f0[argc];
>> }
>>
>> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244
>>
>> I looked into the file, the conversion function is defined as
>> gcc_unreachable (). I am not very familiar with lto, so I don't really
>> know what is the right way to treat the conversions.
>>
>> And I seriously need help with backend patterns.
>
> On the patch.
>
> The documentation needs review by a native english speaker, but here
> are some factual comments:
>
> +In C vector comparison is supported within standard comparison operators:
>
> it should read 'In GNU C' here and everywhere else as this is a GNU
> extension.
>
> The result of the
> +comparison is a signed integer-type vector where the size of each
> +element must be the same as the size of compared vectors element.
>
> The result type of the comparison is determined by the C frontend,
> it isn't under control of the user. What you are implying here is
> restrictions on vector assignments, which are documented elsewhere.
> I'd just say
>
> 'The result of the comparison is a vector of the same width and number
> of elements as the comparison operands with a signed integral element
> type.'
>
> +In addition to the vector comparison C supports conditional expressions
>
> See above.
>
> +For the convenience condition in the vector conditional can be just a
> +vector of signed integer type.
>
> 'of integer type.' I don't see a reason to disallow unsigned integers,
> they can be equally well compared against zero.
I'll have a final go on the documentation, it is untouched from the old patches.
> Index: gcc/targhooks.h
> ===
> --- gcc/targhooks.h (revision 177665)
> +++ gcc/targhooks.h (working copy)
> @@ -86,6 +86,7 @@ extern int default_builtin_vectorization
> extern tree default_builtin_reciprocal (unsigned int, bool, bool);
>
> extern bool default_builtin_vector_alignment_reachable (const_tree, bool);
> +
> extern bool
> default_builtin_support_vector_misalignment (enum machine_mode mode,
> const_tree,
>
> spurious whitespace change.
Yes, thanks.
> Index: gcc/optabs.c
> ===
> --- gcc/optabs.c (revision 177665)
> +++ gcc/optabs.c (working copy)
> @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
> ...
> + else
> + {
> + rtx rtx_op0;
> + rtx vec;
> +
> + rtx_op0 = expand_normal (op0);
> + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
> + vec = CONST0_RTX (mode);
> +
> + create_output_operand (&ops[0], target, mode);
> + create_input_operand (&ops[1], rtx_op1, mode);
> + create_input_operand (&ops[2], rtx_op2, mode);
> + create_input_operand (&ops[3], comparison, mode);
> + create_input_operand (&ops[4], rtx_op0, mode);
> + create_input_operand (&ops[5], vec, mode);
>
> this still builds the fake(?) != comparison, but as you said you need help
> with the .md part if we want to use a machine specific pattern for this
> case (which we eventually want, for the sake of using XOP vcond).
Yes, I am waiting for it. This is the only way at the moment to make
sure that in
m = a > b;
r = m ? c : d;
m in the vcond is not transformed into the m != 0.
> Index: gcc/target.h
> ===
> --- gcc/target.h (revision 177665)
> +++ gcc/target.h (working copy)
> @@ -51,6 +51,7 @@
> #define GCC_TARGET_H
>
> #include "insn-modes.h"
> +#include "gimple.h"
>
> #ifdef ENABLE_CHECKING
>
> spurious change.
Old stuff, fixed.
> @@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr
> floating-point, we can only do some of these simplifications.) */
> if (operand_equal_p (arg0, arg1, 0))
> {
> + tree arg0_type = TREE_TYPE (arg0);
> +
> switch (code)
> {
> case EQ_EXPR:
> - if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
> - || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
> + if (! FLOAT_TYPE_P (arg0_type)
> + || ! HONOR_NANS (TYPE_MODE (arg0_type)))
> ...
Ok.
>
> Likewise.
>
> @@
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
wrote:
> Here is a cleaned-up patch without the hook. Mostly it works in a way
> we discussed.
>
> So I think it is a right time to do something about vcond patterns,
> which would allow me to get rid of conversions that I need to put all
> over the code.
>
> Also at the moment the patch breaks lto frontend with a simple example:
> #define vector(elcount, type) \
> __attribute__((vector_size((elcount)*sizeof(type type
>
> int main (int argc, char *argv[]) {
> vector (4, float) f0;
> vector (4, float) f1;
>
> f0 = f1 != f0
> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
>
> return (int)f0[argc];
> }
>
> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244
>
> I looked into the file, the conversion function is defined as
> gcc_unreachable (). I am not very familiar with lto, so I don't really
> know what is the right way to treat the conversions.
>
> And I seriously need help with backend patterns.
On the patch.
The documentation needs review by a native english speaker, but here
are some factual comments:
+In C vector comparison is supported within standard comparison operators:
it should read 'In GNU C' here and everywhere else as this is a GNU
extension.
The result of the
+comparison is a signed integer-type vector where the size of each
+element must be the same as the size of compared vectors element.
The result type of the comparison is determined by the C frontend,
it isn't under control of the user. What you are implying here is
restrictions on vector assignments, which are documented elsewhere.
I'd just say
'The result of the comparison is a vector of the same width and number
of elements as the comparison operands with a signed integral element
type.'
+In addition to the vector comparison C supports conditional expressions
See above.
+For the convenience condition in the vector conditional can be just a
+vector of signed integer type.
'of integer type.' I don't see a reason to disallow unsigned integers,
they can be equally well compared against zero.
Index: gcc/targhooks.h
===
--- gcc/targhooks.h (revision 177665)
+++ gcc/targhooks.h (working copy)
@@ -86,6 +86,7 @@ extern int default_builtin_vectorization
extern tree default_builtin_reciprocal (unsigned int, bool, bool);
extern bool default_builtin_vector_alignment_reachable (const_tree, bool);
+
extern bool
default_builtin_support_vector_misalignment (enum machine_mode mode,
const_tree,
spurious whitespace change.
Index: gcc/optabs.c
===
--- gcc/optabs.c(revision 177665)
+++ gcc/optabs.c(working copy)
@@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type
...
+ else
+{
+ rtx rtx_op0;
+ rtx vec;
+
+ rtx_op0 = expand_normal (op0);
+ comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX);
+ vec = CONST0_RTX (mode);
+
+ create_output_operand (&ops[0], target, mode);
+ create_input_operand (&ops[1], rtx_op1, mode);
+ create_input_operand (&ops[2], rtx_op2, mode);
+ create_input_operand (&ops[3], comparison, mode);
+ create_input_operand (&ops[4], rtx_op0, mode);
+ create_input_operand (&ops[5], vec, mode);
this still builds the fake(?) != comparison, but as you said you need help
with the .md part if we want to use a machine specific pattern for this
case (which we eventually want, for the sake of using XOP vcond).
Index: gcc/target.h
===
--- gcc/target.h(revision 177665)
+++ gcc/target.h(working copy)
@@ -51,6 +51,7 @@
#define GCC_TARGET_H
#include "insn-modes.h"
+#include "gimple.h"
#ifdef ENABLE_CHECKING
spurious change.
@@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr
floating-point, we can only do some of these simplifications.) */
if (operand_equal_p (arg0, arg1, 0))
{
+ tree arg0_type = TREE_TYPE (arg0);
+
switch (code)
{
case EQ_EXPR:
- if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
- || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
+ if (! FLOAT_TYPE_P (arg0_type)
+ || ! HONOR_NANS (TYPE_MODE (arg0_type)))
...
Likewise.
@@ -8440,6 +8440,37 @@ expand_expr_real_2 (sepops ops, rtx targ
case UNGE_EXPR:
case UNEQ_EXPR:
case LTGT_EXPR:
+ if (TREE_CODE (ops->type) == VECTOR_TYPE)
+ {
+ enum tree_code code = ops->code;
+ tree arg0 = ops->op0;
+ tree arg1 = ops->op1;
move this code to do_store_flag (we really store a flag value). It should
also simply do what expand_vec_cond_expr does, probably simply
calling that with the {-1,...} {0,...} extra args should work.
As for the still required conversions, you should be able to
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 8:34 AM, Richard Guenther
wrote:
> On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
> wrote:
>> Here is a cleaned-up patch without the hook. Mostly it works in a way
>> we discussed.
>>
>> So I think it is a right time to do something about vcond patterns,
>> which would allow me to get rid of conversions that I need to put all
>> over the code.
>>
>> Also at the moment the patch breaks lto frontend with a simple example:
>> #define vector(elcount, type) \
>> __attribute__((vector_size((elcount)*sizeof(type type
>>
>> int main (int argc, char *argv[]) {
>> vector (4, float) f0;
>> vector (4, float) f1;
>>
>> f0 = f1 != f0
>> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
>>
>> return (int)f0[argc];
>> }
>>
>> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244
>>
>> I looked into the file, the conversion function is defined as
>> gcc_unreachable (). I am not very familiar with lto, so I don't really
>> know what is the right way to treat the conversions.
>
> convert cannot be called from the middle-end, instead use fold_convert.
Thanks, great. I didn't know that. Using fold_convert solves my
problem and make all my tests pass.
>
>> And I seriously need help with backend patterns.
>
> I'll look at the patch in detail later today.
Thanks,
Artem.
> Richard.
>
>>
>> Thanks,
>> Artem.
>>
>
Re: Vector Comparison patch
On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov
wrote:
> Here is a cleaned-up patch without the hook. Mostly it works in a way
> we discussed.
>
> So I think it is a right time to do something about vcond patterns,
> which would allow me to get rid of conversions that I need to put all
> over the code.
>
> Also at the moment the patch breaks lto frontend with a simple example:
> #define vector(elcount, type) \
> __attribute__((vector_size((elcount)*sizeof(type type
>
> int main (int argc, char *argv[]) {
> vector (4, float) f0;
> vector (4, float) f1;
>
> f0 = f1 != f0
> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0};
>
> return (int)f0[argc];
> }
>
> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244
>
> I looked into the file, the conversion function is defined as
> gcc_unreachable (). I am not very familiar with lto, so I don't really
> know what is the right way to treat the conversions.
convert cannot be called from the middle-end, instead use fold_convert.
> And I seriously need help with backend patterns.
I'll look at the patch in detail later today.
Richard.
>
> Thanks,
> Artem.
>
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 12:23 PM, Richard Guenther
wrote:
> On Tue, Aug 23, 2011 at 1:11 PM, Artem Shinkarov
> wrote:
>> On Tue, Aug 23, 2011 at 11:56 AM, Richard Guenther
>> wrote:
>>> On Tue, Aug 23, 2011 at 12:45 PM, Artem Shinkarov
>>> wrote:
I'm confused.
There is a set of problems which are tightly connected and you address
only one one of them.
I need to do something with C_MAYBE_CONST_EXPR node to allow the
gimplification of the expression. In order to achieve that I am
wrapping expression which can contain C_MAYBE_EXPR_NODE into
SAVE_EXPR. This works fine, but, the vector condition is lifted out.
So the question is how to get rid of C_MAYBE_CONST_EXPR nodes, making
sure that the expression is still inside VEC_COND_EXPR?
>>>
>>> I can't answer this, but no C_MAYBE_CONST_EXPR nodes may survive
>>> until gimplification. I thought c_fully_fold is exactly used (instead
>>> of c_save_expr) because it _doesn't_ wrap things in C_MAYBE_CONST_EXPR
>>> nodes. Instead you delay that (well, commented out in your patch).
>>
>> Ok. So for the time being save_expr is the only way that we know to
>> avoid C_MAYBE_CONST_EXPR nodes.
>>
All the rest is fine -- a > b is transformed to VEC_COND_EXPR of the
integer type, and when we are using it we can add != 0 to the mask, no
problem. The problem is to make sure that the vector expression is not
lifted out from the VEC_COND_EXPR and that C_MAYBE_CONST_EXPRs are
also no there at the same time.
>>>
>>> Well, for example for floating-point comparisons and -fnon-call-exceptions
>>> you _will_ get comparisons lifted out of the VEC_COND_EXPR. But
>>> that shouldn't be an issue because C semantics are ensured for
>>> the mask ? v0 : v1 source form by changing it to mask != 0 ? v0 : v1 and
>>> the VEC_COND_EXPR semantic for a non-comparison mask operand
>>> is (v0 & mask) | (v1 & ~mask). Which means that we have to be able to
>>> expand mask = v0 < v1 anyway, but we'll simply expand it if it were
>>> VEC_COND_EXPR .
>>
>> Richard, I think you almost get it, but there is a tiny thing you have
>> missed.
>> Look, let's assume, that by some reason when we gimplified a > b, the
>> comparison was lifted out. So we have the following situation:
>>
>> D.1 = a > b;
>> comp = vcond
>> ...
>>
>> Ok?
>> Now, I fully agree that we want to treat lifted a > b as VCOND. Now,
>> what I am doing in the veclower is when I meet vector comparison a >
>> b, I wrap it in the VCOND, otherwise it would not be recognized by
>> optabs. literally I am doing:
>>
>> rhs = gimplify_build3 (gsi, VEC_COND_EXPR, a, b, {-1}, {0}>
>>
>> And here is a devil hidden. By some reason, when this expression is
>> gimplified, a > b is lifted again and is left outside the
>> VEC_COND_EXPR, and that is the problem I am trying to fight with. Have
>> any ideas what could be done here?
>
> Well, don't do it. Check if the target can expand
>
> D.1 = a > b;
>
> via feeding it vcond and if not, expand it
> piecewise
> in veclower. If it can handle it - leave it alone!
>
> In expand_expr_real_2 add to the EQ_EXPR (etc.) case the case
> of a vector-typed comparison and use the vcond optab for it, again
> via vcond . If you look at the EQ_EXPR case
> it dispatches to do_store_flag - that's the best place to handle
> vector-typed compares.
>
> Richard.
>
That sounds like a plan. I'll investigate if it can be done.
Also, if we can handle a > b, then we don't need to construct vcond b, {-1}, {0}>, we will know that it would be constructed correctly
when expanding.
Thanks for your help,
Artem.
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 1:11 PM, Artem Shinkarov
wrote:
> On Tue, Aug 23, 2011 at 11:56 AM, Richard Guenther
> wrote:
>> On Tue, Aug 23, 2011 at 12:45 PM, Artem Shinkarov
>> wrote:
>>> I'm confused.
>>> There is a set of problems which are tightly connected and you address
>>> only one one of them.
>>>
>>> I need to do something with C_MAYBE_CONST_EXPR node to allow the
>>> gimplification of the expression. In order to achieve that I am
>>> wrapping expression which can contain C_MAYBE_EXPR_NODE into
>>> SAVE_EXPR. This works fine, but, the vector condition is lifted out.
>>> So the question is how to get rid of C_MAYBE_CONST_EXPR nodes, making
>>> sure that the expression is still inside VEC_COND_EXPR?
>>
>> I can't answer this, but no C_MAYBE_CONST_EXPR nodes may survive
>> until gimplification. I thought c_fully_fold is exactly used (instead
>> of c_save_expr) because it _doesn't_ wrap things in C_MAYBE_CONST_EXPR
>> nodes. Instead you delay that (well, commented out in your patch).
>
> Ok. So for the time being save_expr is the only way that we know to
> avoid C_MAYBE_CONST_EXPR nodes.
>
>>> All the rest is fine -- a > b is transformed to VEC_COND_EXPR of the
>>> integer type, and when we are using it we can add != 0 to the mask, no
>>> problem. The problem is to make sure that the vector expression is not
>>> lifted out from the VEC_COND_EXPR and that C_MAYBE_CONST_EXPRs are
>>> also no there at the same time.
>>
>> Well, for example for floating-point comparisons and -fnon-call-exceptions
>> you _will_ get comparisons lifted out of the VEC_COND_EXPR. But
>> that shouldn't be an issue because C semantics are ensured for
>> the mask ? v0 : v1 source form by changing it to mask != 0 ? v0 : v1 and
>> the VEC_COND_EXPR semantic for a non-comparison mask operand
>> is (v0 & mask) | (v1 & ~mask). Which means that we have to be able to
>> expand mask = v0 < v1 anyway, but we'll simply expand it if it were
>> VEC_COND_EXPR .
>
> Richard, I think you almost get it, but there is a tiny thing you have missed.
> Look, let's assume, that by some reason when we gimplified a > b, the
> comparison was lifted out. So we have the following situation:
>
> D.1 = a > b;
> comp = vcond
> ...
>
> Ok?
> Now, I fully agree that we want to treat lifted a > b as VCOND. Now,
> what I am doing in the veclower is when I meet vector comparison a >
> b, I wrap it in the VCOND, otherwise it would not be recognized by
> optabs. literally I am doing:
>
> rhs = gimplify_build3 (gsi, VEC_COND_EXPR, a, b, {-1}, {0}>
>
> And here is a devil hidden. By some reason, when this expression is
> gimplified, a > b is lifted again and is left outside the
> VEC_COND_EXPR, and that is the problem I am trying to fight with. Have
> any ideas what could be done here?
Well, don't do it. Check if the target can expand
D.1 = a > b;
via feeding it vcond and if not, expand it piecewise
in veclower. If it can handle it - leave it alone!
In expand_expr_real_2 add to the EQ_EXPR (etc.) case the case
of a vector-typed comparison and use the vcond optab for it, again
via vcond . If you look at the EQ_EXPR case
it dispatches to do_store_flag - that's the best place to handle
vector-typed compares.
Richard.
Re: Vector Comparison patch
Sorry, not
rhs = gimplify_build3 (gsi, VEC_COND_EXPR, a, b, {-1}, {0}>
but rather
rhs = gimplify_build3 (gsi, VEC_COND_EXPR, build2 (GT_EXPR, type, a,
b), {-1}, {0}>
Artem.
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 11:56 AM, Richard Guenther
wrote:
> On Tue, Aug 23, 2011 at 12:45 PM, Artem Shinkarov
> wrote:
>> On Tue, Aug 23, 2011 at 11:33 AM, Richard Guenther
>> wrote:
>>> On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
>>> wrote:
On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
wrote:
> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
> wrote:
>> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>>> wrote:
I'll just send you my current version. I'll be a little bit more
specific.
The problem starts when you try to lower the following expression:
x = a > b;
x1 = vcond
vcond
Now, you go from the beginning to the end of the block, and you cannot
leave a > b, because only vconds are valid expressions to expand.
Now, you meet a > b first. You try to transform it into vcond b,
-1, 0>, you build this expression, then you try to gimplify it, and
you see that you have something like:
x' = a >b;
x = vcond
x1 = vcond
vcond
and your gsi stands at the x1 now, so the gimplification created a
comparison that optab would not understand. And I am not really sure
that you would be able to solve this problem easily.
It would helpr, if you could create vcond, but you
cant and x op y is a single tree that must be gimplified, and I am not
sure that you can persuade gimplifier to leave this expression
untouched.
In the attachment the current version of the patch.
>>>
>>> I can't reproduce it with your patch. For
>>>
>>> #define vector(elcount, type) \
>>> __attribute__((vector_size((elcount)*sizeof(type type
>>>
>>> vector (4, float) x, y;
>>> vector (4, int) a,b;
>>> int
>>> main (int argc, char *argv[])
>>> {
>>> vector (4, int) i0 = x < y;
>>> vector (4, int) i1 = i0 ? a : b;
>>> return 0;
>>> }
>>>
>>> I get from the C frontend:
>>>
>>> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
>>> -1, -1 } , { 0, 0, 0, 0 } > ;
>>> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
>>> SAVE_EXPR > ;
>>>
>>> but I have expected i0 != 0 in the second VEC_COND_EXPR.
>>
>> I don't put it there. This patch adds != 0, rather removing. But this
>> could be changed.
>
> ?
>
>>> I do see that the gimplifier pulls away the condition for the first
>>> VEC_COND_EXPR though:
>>>
>>> x.0 = x;
>>> y.1 = y;
>>> D.2735 = x.0 < y.1;
>>> D.2734 = D.2735;
>>> D.2736 = D.2734;
>>> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
>>> { 0, 0, 0, 0 } > ;
>>>
>>> which is, I believe because of the SAVE_EXPR wrapped around the
>>> comparison. Why do you bother wrapping all operands in save-exprs?
>>
>> I bother because they could be MAYBE_CONST which breaks the
>> gimplifier. But I don't really know if you can do it better. I can
>> always do this checking on operands of constructed vcond...
>
> Err, the patch does
>
> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
> + tmp = c_fully_fold (ifexp, false, &maybe_const);
> + ifexp = save_expr (tmp);
> + wrap &= maybe_const;
>
> why is
>
> ifexp = save_expr (tmp);
>
> necessary here? SAVE_EXPR is if you need to protect side-effects
> from being evaluated twice if you use an operand twice. But all
> operands are just used a single time.
Again, the only reason why save_expr is there is to avoid MAYBE_CONST
nodes to break the gimplification. But may be it is a wrong way of
doing it, but it does the job.
> And I expected, instead of
>
> + if ((COMPARISON_CLASS_P (ifexp)
> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
> + || TREE_TYPE (ifexp) != TREE_TYPE (op1))
> + {
> + tree comp_type = COMPARISON_CLASS_P (ifexp)
> + ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
> + : TREE_TYPE (ifexp);
> +
> + op1 = convert (comp_type, op1);
> + op2 = convert (comp_type, op2);
> + vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
> + vcond = convert (TREE_TYPE (op1), vcond);
> + }
> + else
> + vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
>
> if (!COMPARISON_CLASS_P (ifexp))
> ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
> build_vector_from_val (TREE_TYPE (ifexp), 0));
>
> if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
> {
> ...
>
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 12:45 PM, Artem Shinkarov
wrote:
> On Tue, Aug 23, 2011 at 11:33 AM, Richard Guenther
> wrote:
>> On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
>> wrote:
>>> On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
>>> wrote:
On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
wrote:
> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>> wrote:
>>> I'll just send you my current version. I'll be a little bit more
>>> specific.
>>>
>>> The problem starts when you try to lower the following expression:
>>>
>>> x = a > b;
>>> x1 = vcond
>>> vcond
>>>
>>> Now, you go from the beginning to the end of the block, and you cannot
>>> leave a > b, because only vconds are valid expressions to expand.
>>>
>>> Now, you meet a > b first. You try to transform it into vcond b,
>>> -1, 0>, you build this expression, then you try to gimplify it, and
>>> you see that you have something like:
>>>
>>> x' = a >b;
>>> x = vcond
>>> x1 = vcond
>>> vcond
>>>
>>> and your gsi stands at the x1 now, so the gimplification created a
>>> comparison that optab would not understand. And I am not really sure
>>> that you would be able to solve this problem easily.
>>>
>>> It would helpr, if you could create vcond, but you
>>> cant and x op y is a single tree that must be gimplified, and I am not
>>> sure that you can persuade gimplifier to leave this expression
>>> untouched.
>>>
>>> In the attachment the current version of the patch.
>>
>> I can't reproduce it with your patch. For
>>
>> #define vector(elcount, type) \
>> __attribute__((vector_size((elcount)*sizeof(type type
>>
>> vector (4, float) x, y;
>> vector (4, int) a,b;
>> int
>> main (int argc, char *argv[])
>> {
>> vector (4, int) i0 = x < y;
>> vector (4, int) i1 = i0 ? a : b;
>> return 0;
>> }
>>
>> I get from the C frontend:
>>
>> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
>> -1, -1 } , { 0, 0, 0, 0 } > ;
>> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
>> SAVE_EXPR > ;
>>
>> but I have expected i0 != 0 in the second VEC_COND_EXPR.
>
> I don't put it there. This patch adds != 0, rather removing. But this
> could be changed.
?
>> I do see that the gimplifier pulls away the condition for the first
>> VEC_COND_EXPR though:
>>
>> x.0 = x;
>> y.1 = y;
>> D.2735 = x.0 < y.1;
>> D.2734 = D.2735;
>> D.2736 = D.2734;
>> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
>> { 0, 0, 0, 0 } > ;
>>
>> which is, I believe because of the SAVE_EXPR wrapped around the
>> comparison. Why do you bother wrapping all operands in save-exprs?
>
> I bother because they could be MAYBE_CONST which breaks the
> gimplifier. But I don't really know if you can do it better. I can
> always do this checking on operands of constructed vcond...
Err, the patch does
+ /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
+ tmp = c_fully_fold (ifexp, false, &maybe_const);
+ ifexp = save_expr (tmp);
+ wrap &= maybe_const;
why is
ifexp = save_expr (tmp);
necessary here? SAVE_EXPR is if you need to protect side-effects
from being evaluated twice if you use an operand twice. But all
operands are just used a single time.
>>>
>>> Again, the only reason why save_expr is there is to avoid MAYBE_CONST
>>> nodes to break the gimplification. But may be it is a wrong way of
>>> doing it, but it does the job.
>>>
And I expected, instead of
+ if ((COMPARISON_CLASS_P (ifexp)
+ && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
+ || TREE_TYPE (ifexp) != TREE_TYPE (op1))
+ {
+ tree comp_type = COMPARISON_CLASS_P (ifexp)
+ ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
+ : TREE_TYPE (ifexp);
+
+ op1 = convert (comp_type, op1);
+ op2 = convert (comp_type, op2);
+ vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
+ vcond = convert (TREE_TYPE (op1), vcond);
+ }
+ else
+ vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
if (!COMPARISON_CLASS_P (ifexp))
ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
build_vector_from_val (TREE_TYPE (ifexp), 0));
if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
{
...
>>> Why?
>>> This is a function to constuct any vcond. The result of ifexp is
>>> always signed integer vector if it is a comparison, but we need to
>>> make sure that all the elements of vcond have the
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 11:33 AM, Richard Guenther
wrote:
> On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
> wrote:
>> On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
>> wrote:
>>> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
>>> wrote:
On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
> wrote:
>> I'll just send you my current version. I'll be a little bit more
>> specific.
>>
>> The problem starts when you try to lower the following expression:
>>
>> x = a > b;
>> x1 = vcond
>> vcond
>>
>> Now, you go from the beginning to the end of the block, and you cannot
>> leave a > b, because only vconds are valid expressions to expand.
>>
>> Now, you meet a > b first. You try to transform it into vcond b,
>> -1, 0>, you build this expression, then you try to gimplify it, and
>> you see that you have something like:
>>
>> x' = a >b;
>> x = vcond
>> x1 = vcond
>> vcond
>>
>> and your gsi stands at the x1 now, so the gimplification created a
>> comparison that optab would not understand. And I am not really sure
>> that you would be able to solve this problem easily.
>>
>> It would helpr, if you could create vcond, but you
>> cant and x op y is a single tree that must be gimplified, and I am not
>> sure that you can persuade gimplifier to leave this expression
>> untouched.
>>
>> In the attachment the current version of the patch.
>
> I can't reproduce it with your patch. For
>
> #define vector(elcount, type) \
> __attribute__((vector_size((elcount)*sizeof(type type
>
> vector (4, float) x, y;
> vector (4, int) a,b;
> int
> main (int argc, char *argv[])
> {
> vector (4, int) i0 = x < y;
> vector (4, int) i1 = i0 ? a : b;
> return 0;
> }
>
> I get from the C frontend:
>
> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
> -1, -1 } , { 0, 0, 0, 0 } > ;
> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
> SAVE_EXPR > ;
>
> but I have expected i0 != 0 in the second VEC_COND_EXPR.
I don't put it there. This patch adds != 0, rather removing. But this
could be changed.
>>>
>>> ?
>>>
> I do see that the gimplifier pulls away the condition for the first
> VEC_COND_EXPR though:
>
> x.0 = x;
> y.1 = y;
> D.2735 = x.0 < y.1;
> D.2734 = D.2735;
> D.2736 = D.2734;
> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
> { 0, 0, 0, 0 } > ;
>
> which is, I believe because of the SAVE_EXPR wrapped around the
> comparison. Why do you bother wrapping all operands in save-exprs?
I bother because they could be MAYBE_CONST which breaks the
gimplifier. But I don't really know if you can do it better. I can
always do this checking on operands of constructed vcond...
>>>
>>> Err, the patch does
>>>
>>> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
>>> + tmp = c_fully_fold (ifexp, false, &maybe_const);
>>> + ifexp = save_expr (tmp);
>>> + wrap &= maybe_const;
>>>
>>> why is
>>>
>>> ifexp = save_expr (tmp);
>>>
>>> necessary here? SAVE_EXPR is if you need to protect side-effects
>>> from being evaluated twice if you use an operand twice. But all
>>> operands are just used a single time.
>>
>> Again, the only reason why save_expr is there is to avoid MAYBE_CONST
>> nodes to break the gimplification. But may be it is a wrong way of
>> doing it, but it does the job.
>>
>>> And I expected, instead of
>>>
>>> + if ((COMPARISON_CLASS_P (ifexp)
>>> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
>>> + || TREE_TYPE (ifexp) != TREE_TYPE (op1))
>>> + {
>>> + tree comp_type = COMPARISON_CLASS_P (ifexp)
>>> + ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
>>> + : TREE_TYPE (ifexp);
>>> +
>>> + op1 = convert (comp_type, op1);
>>> + op2 = convert (comp_type, op2);
>>> + vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
>>> + vcond = convert (TREE_TYPE (op1), vcond);
>>> + }
>>> + else
>>> + vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
>>>
>>> if (!COMPARISON_CLASS_P (ifexp))
>>> ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
>>> build_vector_from_val (TREE_TYPE (ifexp), 0));
>>>
>>> if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
>>> {
>>> ...
>>>
>> Why?
>> This is a function to constuct any vcond. The result of ifexp is
>> always signed integer vector if it is a comparison, but we need to
>> make sure that all the elements of vcond have the same type.
>>
>> And I didn't really understand if we can guarantee that vector
>> comparison would not be lifted out by the gimplifier. It happens in
>> case I put this save_expr, it could possibly h
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
wrote:
> On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
> wrote:
>> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
>> wrote:
>>> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
wrote:
> I'll just send you my current version. I'll be a little bit more specific.
>
> The problem starts when you try to lower the following expression:
>
> x = a > b;
> x1 = vcond
> vcond
>
> Now, you go from the beginning to the end of the block, and you cannot
> leave a > b, because only vconds are valid expressions to expand.
>
> Now, you meet a > b first. You try to transform it into vcond b,
> -1, 0>, you build this expression, then you try to gimplify it, and
> you see that you have something like:
>
> x' = a >b;
> x = vcond
> x1 = vcond
> vcond
>
> and your gsi stands at the x1 now, so the gimplification created a
> comparison that optab would not understand. And I am not really sure
> that you would be able to solve this problem easily.
>
> It would helpr, if you could create vcond, but you
> cant and x op y is a single tree that must be gimplified, and I am not
> sure that you can persuade gimplifier to leave this expression
> untouched.
>
> In the attachment the current version of the patch.
I can't reproduce it with your patch. For
#define vector(elcount, type) \
__attribute__((vector_size((elcount)*sizeof(type type
vector (4, float) x, y;
vector (4, int) a,b;
int
main (int argc, char *argv[])
{
vector (4, int) i0 = x < y;
vector (4, int) i1 = i0 ? a : b;
return 0;
}
I get from the C frontend:
vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
-1, -1 } , { 0, 0, 0, 0 } > ;
vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
SAVE_EXPR > ;
but I have expected i0 != 0 in the second VEC_COND_EXPR.
>>>
>>> I don't put it there. This patch adds != 0, rather removing. But this
>>> could be changed.
>>
>> ?
>>
I do see that the gimplifier pulls away the condition for the first
VEC_COND_EXPR though:
x.0 = x;
y.1 = y;
D.2735 = x.0 < y.1;
D.2734 = D.2735;
D.2736 = D.2734;
i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
{ 0, 0, 0, 0 } > ;
which is, I believe because of the SAVE_EXPR wrapped around the
comparison. Why do you bother wrapping all operands in save-exprs?
>>>
>>> I bother because they could be MAYBE_CONST which breaks the
>>> gimplifier. But I don't really know if you can do it better. I can
>>> always do this checking on operands of constructed vcond...
>>
>> Err, the patch does
>>
>> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
>> + tmp = c_fully_fold (ifexp, false, &maybe_const);
>> + ifexp = save_expr (tmp);
>> + wrap &= maybe_const;
>>
>> why is
>>
>> ifexp = save_expr (tmp);
>>
>> necessary here? SAVE_EXPR is if you need to protect side-effects
>> from being evaluated twice if you use an operand twice. But all
>> operands are just used a single time.
>
> Again, the only reason why save_expr is there is to avoid MAYBE_CONST
> nodes to break the gimplification. But may be it is a wrong way of
> doing it, but it does the job.
>
>> And I expected, instead of
>>
>> + if ((COMPARISON_CLASS_P (ifexp)
>> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
>> + || TREE_TYPE (ifexp) != TREE_TYPE (op1))
>> + {
>> + tree comp_type = COMPARISON_CLASS_P (ifexp)
>> + ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
>> + : TREE_TYPE (ifexp);
>> +
>> + op1 = convert (comp_type, op1);
>> + op2 = convert (comp_type, op2);
>> + vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
>> + vcond = convert (TREE_TYPE (op1), vcond);
>> + }
>> + else
>> + vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
>>
>> if (!COMPARISON_CLASS_P (ifexp))
>> ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
>> build_vector_from_val (TREE_TYPE (ifexp), 0));
>>
>> if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
>> {
>> ...
>>
> Why?
> This is a function to constuct any vcond. The result of ifexp is
> always signed integer vector if it is a comparison, but we need to
> make sure that all the elements of vcond have the same type.
>
> And I didn't really understand if we can guarantee that vector
> comparison would not be lifted out by the gimplifier. It happens in
> case I put this save_expr, it could possibly happen in some other
> cases. How can we prevent that?
We don't need to prevent it. If the C frontend makes sure that the
mask of a VEC_COND_EXPR is always {-1,...} or {0,} by expanding
mask ? v1 : v2 to V
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
wrote:
> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
> wrote:
>> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>>> wrote:
I'll just send you my current version. I'll be a little bit more specific.
The problem starts when you try to lower the following expression:
x = a > b;
x1 = vcond
vcond
Now, you go from the beginning to the end of the block, and you cannot
leave a > b, because only vconds are valid expressions to expand.
Now, you meet a > b first. You try to transform it into vcond b,
-1, 0>, you build this expression, then you try to gimplify it, and
you see that you have something like:
x' = a >b;
x = vcond
x1 = vcond
vcond
and your gsi stands at the x1 now, so the gimplification created a
comparison that optab would not understand. And I am not really sure
that you would be able to solve this problem easily.
It would helpr, if you could create vcond, but you
cant and x op y is a single tree that must be gimplified, and I am not
sure that you can persuade gimplifier to leave this expression
untouched.
In the attachment the current version of the patch.
>>>
>>> I can't reproduce it with your patch. For
>>>
>>> #define vector(elcount, type) \
>>> __attribute__((vector_size((elcount)*sizeof(type type
>>>
>>> vector (4, float) x, y;
>>> vector (4, int) a,b;
>>> int
>>> main (int argc, char *argv[])
>>> {
>>> vector (4, int) i0 = x < y;
>>> vector (4, int) i1 = i0 ? a : b;
>>> return 0;
>>> }
>>>
>>> I get from the C frontend:
>>>
>>> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
>>> -1, -1 } , { 0, 0, 0, 0 } > ;
>>> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
>>> SAVE_EXPR > ;
>>>
>>> but I have expected i0 != 0 in the second VEC_COND_EXPR.
>>
>> I don't put it there. This patch adds != 0, rather removing. But this
>> could be changed.
>
> ?
>
>>> I do see that the gimplifier pulls away the condition for the first
>>> VEC_COND_EXPR though:
>>>
>>> x.0 = x;
>>> y.1 = y;
>>> D.2735 = x.0 < y.1;
>>> D.2734 = D.2735;
>>> D.2736 = D.2734;
>>> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
>>> { 0, 0, 0, 0 } > ;
>>>
>>> which is, I believe because of the SAVE_EXPR wrapped around the
>>> comparison. Why do you bother wrapping all operands in save-exprs?
>>
>> I bother because they could be MAYBE_CONST which breaks the
>> gimplifier. But I don't really know if you can do it better. I can
>> always do this checking on operands of constructed vcond...
>
> Err, the patch does
>
> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
> + tmp = c_fully_fold (ifexp, false, &maybe_const);
> + ifexp = save_expr (tmp);
> + wrap &= maybe_const;
>
> why is
>
> ifexp = save_expr (tmp);
>
> necessary here? SAVE_EXPR is if you need to protect side-effects
> from being evaluated twice if you use an operand twice. But all
> operands are just used a single time.
Again, the only reason why save_expr is there is to avoid MAYBE_CONST
nodes to break the gimplification. But may be it is a wrong way of
doing it, but it does the job.
> And I expected, instead of
>
> + if ((COMPARISON_CLASS_P (ifexp)
> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
> + || TREE_TYPE (ifexp) != TREE_TYPE (op1))
> + {
> + tree comp_type = COMPARISON_CLASS_P (ifexp)
> + ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
> + : TREE_TYPE (ifexp);
> +
> + op1 = convert (comp_type, op1);
> + op2 = convert (comp_type, op2);
> + vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
> + vcond = convert (TREE_TYPE (op1), vcond);
> + }
> + else
> + vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
>
> if (!COMPARISON_CLASS_P (ifexp))
> ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
> build_vector_from_val (TREE_TYPE (ifexp), 0));
>
> if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
> {
> ...
>
Why?
This is a function to constuct any vcond. The result of ifexp is
always signed integer vector if it is a comparison, but we need to
make sure that all the elements of vcond have the same type.
And I didn't really understand if we can guarantee that vector
comparison would not be lifted out by the gimplifier. It happens in
case I put this save_expr, it could possibly happen in some other
cases. How can we prevent that?
Artem.
>> You are right, that if you just put a comparison of variables there
>> then we are fine. My point is that whenever gimplifier is pulling out
>> the comparison from the first operand, replacing it with the variable,
>> then we are screwed, because there is no chance to put it back, and
>> that is exactly what happens in expand_vector_comparison, if you
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
wrote:
> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>> wrote:
>>> I'll just send you my current version. I'll be a little bit more specific.
>>>
>>> The problem starts when you try to lower the following expression:
>>>
>>> x = a > b;
>>> x1 = vcond
>>> vcond
>>>
>>> Now, you go from the beginning to the end of the block, and you cannot
>>> leave a > b, because only vconds are valid expressions to expand.
>>>
>>> Now, you meet a > b first. You try to transform it into vcond b,
>>> -1, 0>, you build this expression, then you try to gimplify it, and
>>> you see that you have something like:
>>>
>>> x' = a >b;
>>> x = vcond
>>> x1 = vcond
>>> vcond
>>>
>>> and your gsi stands at the x1 now, so the gimplification created a
>>> comparison that optab would not understand. And I am not really sure
>>> that you would be able to solve this problem easily.
>>>
>>> It would helpr, if you could create vcond, but you
>>> cant and x op y is a single tree that must be gimplified, and I am not
>>> sure that you can persuade gimplifier to leave this expression
>>> untouched.
>>>
>>> In the attachment the current version of the patch.
>>
>> I can't reproduce it with your patch. For
>>
>> #define vector(elcount, type) \
>> __attribute__((vector_size((elcount)*sizeof(type type
>>
>> vector (4, float) x, y;
>> vector (4, int) a,b;
>> int
>> main (int argc, char *argv[])
>> {
>> vector (4, int) i0 = x < y;
>> vector (4, int) i1 = i0 ? a : b;
>> return 0;
>> }
>>
>> I get from the C frontend:
>>
>> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
>> -1, -1 } , { 0, 0, 0, 0 } > ;
>> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
>> SAVE_EXPR > ;
>>
>> but I have expected i0 != 0 in the second VEC_COND_EXPR.
>
> I don't put it there. This patch adds != 0, rather removing. But this
> could be changed.
?
>> I do see that the gimplifier pulls away the condition for the first
>> VEC_COND_EXPR though:
>>
>> x.0 = x;
>> y.1 = y;
>> D.2735 = x.0 < y.1;
>> D.2734 = D.2735;
>> D.2736 = D.2734;
>> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
>> { 0, 0, 0, 0 } > ;
>>
>> which is, I believe because of the SAVE_EXPR wrapped around the
>> comparison. Why do you bother wrapping all operands in save-exprs?
>
> I bother because they could be MAYBE_CONST which breaks the
> gimplifier. But I don't really know if you can do it better. I can
> always do this checking on operands of constructed vcond...
Err, the patch does
+ /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
+ tmp = c_fully_fold (ifexp, false, &maybe_const);
+ ifexp = save_expr (tmp);
+ wrap &= maybe_const;
why is
ifexp = save_expr (tmp);
necessary here? SAVE_EXPR is if you need to protect side-effects
from being evaluated twice if you use an operand twice. But all
operands are just used a single time.
And I expected, instead of
+ if ((COMPARISON_CLASS_P (ifexp)
+ && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
+ || TREE_TYPE (ifexp) != TREE_TYPE (op1))
+{
+ tree comp_type = COMPARISON_CLASS_P (ifexp)
+ ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
+ : TREE_TYPE (ifexp);
+
+ op1 = convert (comp_type, op1);
+ op2 = convert (comp_type, op2);
+ vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
+ vcond = convert (TREE_TYPE (op1), vcond);
+}
+ else
+vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
if (!COMPARISON_CLASS_P (ifexp))
ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
build_vector_from_val (TREE_TYPE (ifexp), 0));
if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
{
...
> You are right, that if you just put a comparison of variables there
> then we are fine. My point is that whenever gimplifier is pulling out
> the comparison from the first operand, replacing it with the variable,
> then we are screwed, because there is no chance to put it back, and
> that is exactly what happens in expand_vector_comparison, if you
> uncomment the replacement -- comparison is always represented as x = a
>> b.
>
>> With that the
>>
>> /* Currently the expansion of VEC_COND_EXPR does not allow
>> expessions where the type of vectors you compare differs
>> form the type of vectors you select from. For the time
>> being we insert implicit conversions. */
>> if ((COMPARISON_CLASS_P (ifexp)
>> && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
>> || TREE_TYPE (ifexp) != TREE_TYPE (op1))
>>
>> checks will fail (because ifexp is a SAVE_EXPR).
>>
>> I'll run into errors when not adding the SAVE_EXPR around the ifexp,
>> the transform into x < y ? {-1,...} : {0,...} is not happening.
Re: Vector Comparison patch
On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
> wrote:
>> I'll just send you my current version. I'll be a little bit more specific.
>>
>> The problem starts when you try to lower the following expression:
>>
>> x = a > b;
>> x1 = vcond
>> vcond
>>
>> Now, you go from the beginning to the end of the block, and you cannot
>> leave a > b, because only vconds are valid expressions to expand.
>>
>> Now, you meet a > b first. You try to transform it into vcond b,
>> -1, 0>, you build this expression, then you try to gimplify it, and
>> you see that you have something like:
>>
>> x' = a >b;
>> x = vcond
>> x1 = vcond
>> vcond
>>
>> and your gsi stands at the x1 now, so the gimplification created a
>> comparison that optab would not understand. And I am not really sure
>> that you would be able to solve this problem easily.
>>
>> It would helpr, if you could create vcond, but you
>> cant and x op y is a single tree that must be gimplified, and I am not
>> sure that you can persuade gimplifier to leave this expression
>> untouched.
>>
>> In the attachment the current version of the patch.
>
> I can't reproduce it with your patch. For
>
> #define vector(elcount, type) \
> __attribute__((vector_size((elcount)*sizeof(type type
>
> vector (4, float) x, y;
> vector (4, int) a,b;
> int
> main (int argc, char *argv[])
> {
> vector (4, int) i0 = x < y;
> vector (4, int) i1 = i0 ? a : b;
> return 0;
> }
>
> I get from the C frontend:
>
> vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
> -1, -1 } , { 0, 0, 0, 0 } > ;
> vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
> SAVE_EXPR > ;
>
> but I have expected i0 != 0 in the second VEC_COND_EXPR.
I don't put it there. This patch adds != 0, rather removing. But this
could be changed.
> I do see that the gimplifier pulls away the condition for the first
> VEC_COND_EXPR though:
>
> x.0 = x;
> y.1 = y;
> D.2735 = x.0 < y.1;
> D.2734 = D.2735;
> D.2736 = D.2734;
> i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
> { 0, 0, 0, 0 } > ;
>
> which is, I believe because of the SAVE_EXPR wrapped around the
> comparison. Why do you bother wrapping all operands in save-exprs?
I bother because they could be MAYBE_CONST which breaks the
gimplifier. But I don't really know if you can do it better. I can
always do this checking on operands of constructed vcond...
You are right, that if you just put a comparison of variables there
then we are fine. My point is that whenever gimplifier is pulling out
the comparison from the first operand, replacing it with the variable,
then we are screwed, because there is no chance to put it back, and
that is exactly what happens in expand_vector_comparison, if you
uncomment the replacement -- comparison is always represented as x = a
> b.
> With that the
>
> /* Currently the expansion of VEC_COND_EXPR does not allow
> expessions where the type of vectors you compare differs
> form the type of vectors you select from. For the time
> being we insert implicit conversions. */
> if ((COMPARISON_CLASS_P (ifexp)
> && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
> || TREE_TYPE (ifexp) != TREE_TYPE (op1))
>
> checks will fail (because ifexp is a SAVE_EXPR).
>
> I'll run into errors when not adding the SAVE_EXPR around the ifexp,
> the transform into x < y ? {-1,...} : {0,...} is not happening.
>>
>> Thanks,
>> Artem.
>>
>>
>> On Mon, Aug 22, 2011 at 9:58 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 10:49 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 9:42 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:58 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
> wrote:
>> Richard
>>
>> I formalized an approach a little-bit, now it works without
>> target
>> hooks, but some polishing is still required. I want you to
>> comment on
>> the several important approaches that I use in the patch.
>>
>> So how does it work.
>> 1) All the vector comparisons at the level of type-checker are
>> introduced using VEC_COND_EXPR with constant selection operands
>> being
>> {-1} and {0}. For example v0 > v1 is transformed
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
wrote:
> I'll just send you my current version. I'll be a little bit more specific.
>
> The problem starts when you try to lower the following expression:
>
> x = a > b;
> x1 = vcond
> vcond
>
> Now, you go from the beginning to the end of the block, and you cannot
> leave a > b, because only vconds are valid expressions to expand.
>
> Now, you meet a > b first. You try to transform it into vcond b,
> -1, 0>, you build this expression, then you try to gimplify it, and
> you see that you have something like:
>
> x' = a >b;
> x = vcond
> x1 = vcond
> vcond
>
> and your gsi stands at the x1 now, so the gimplification created a
> comparison that optab would not understand. And I am not really sure
> that you would be able to solve this problem easily.
>
> It would helpr, if you could create vcond, but you
> cant and x op y is a single tree that must be gimplified, and I am not
> sure that you can persuade gimplifier to leave this expression
> untouched.
>
> In the attachment the current version of the patch.
I can't reproduce it with your patch. For
#define vector(elcount, type) \
__attribute__((vector_size((elcount)*sizeof(type type
vector (4, float) x, y;
vector (4, int) a,b;
int
main (int argc, char *argv[])
{
vector (4, int) i0 = x < y;
vector (4, int) i1 = i0 ? a : b;
return 0;
}
I get from the C frontend:
vector(4) int i0 = VEC_COND_EXPR < SAVE_EXPR , { -1, -1,
-1, -1 } , { 0, 0, 0, 0 } > ;
vector(4) int i1 = VEC_COND_EXPR < SAVE_EXPR , SAVE_EXPR ,
SAVE_EXPR > ;
but I have expected i0 != 0 in the second VEC_COND_EXPR.
I do see that the gimplifier pulls away the condition for the first
VEC_COND_EXPR though:
x.0 = x;
y.1 = y;
D.2735 = x.0 < y.1;
D.2734 = D.2735;
D.2736 = D.2734;
i0 = [vec_cond_expr] VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
{ 0, 0, 0, 0 } > ;
which is, I believe because of the SAVE_EXPR wrapped around the
comparison. Why do you bother wrapping all operands in save-exprs?
With that the
/* Currently the expansion of VEC_COND_EXPR does not allow
expessions where the type of vectors you compare differs
form the type of vectors you select from. For the time
being we insert implicit conversions. */
if ((COMPARISON_CLASS_P (ifexp)
&& TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
|| TREE_TYPE (ifexp) != TREE_TYPE (op1))
checks will fail (because ifexp is a SAVE_EXPR).
I'll run into errors when not adding the SAVE_EXPR around the ifexp,
the transform into x < y ? {-1,...} : {0,...} is not happening.
>
> Thanks,
> Artem.
>
>
> On Mon, Aug 22, 2011 at 9:58 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 10:49 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 9:42 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 5:58 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
wrote:
> Richard
>
> I formalized an approach a little-bit, now it works without target
> hooks, but some polishing is still required. I want you to
> comment on
> the several important approaches that I use in the patch.
>
> So how does it work.
> 1) All the vector comparisons at the level of type-checker are
> introduced using VEC_COND_EXPR with constant selection operands
> being
> {-1} and {0}. For example v0 > v1 is transformed into
> VEC_COND_EXPR> v1, {-1}, {0}>.
>
> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
> 2.a) first operand of VEC_COND_EXPR is comparison, in that case
> nothing changes.
> 2.b) first operand is something else, in that case, we specially
> mark
> this case, recognize it in the backend, and do not create a
> comparison, but use the mask as it was a result of some
> comparison.
>
> 3) In order to make sure that mask in VEC_COND_EXPR
> is a
> vector comparison we use is_vector_comparison function, if it
> returns
> false, then we replace mask with mask != {0}.
>
> So we end-up with the following functionality:
> VEC_COND_EXPR -- if we know that
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 10:49 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 9:42 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 5:58 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
>> wrote:
>>> Richard
>>>
>>> I formalized an approach a little-bit, now it works without target
>>> hooks, but some polishing is still required. I want you to comment
>>> on
>>> the several important approaches that I use in the patch.
>>>
>>> So how does it work.
>>> 1) All the vector comparisons at the level of type-checker are
>>> introduced using VEC_COND_EXPR with constant selection operands
>>> being
>>> {-1} and {0}. For example v0 > v1 is transformed into
>>> VEC_COND_EXPR>>> v1, {-1}, {0}>.
>>>
>>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case
>>> nothing changes.
>>> 2.b) first operand is something else, in that case, we specially
>>> mark
>>> this case, recognize it in the backend, and do not create a
>>> comparison, but use the mask as it was a result of some comparison.
>>>
>>> 3) In order to make sure that mask in VEC_COND_EXPR
>>> is a
>>> vector comparison we use is_vector_comparison function, if it
>>> returns
>>> false, then we replace mask with mask != {0}.
>>>
>>> So we end-up with the following functionality:
>>> VEC_COND_EXPR -- if we know that mask is a result of
>>> comparison of two vectors, we leave it as it is, otherwise change
>>> with
>>> mask != {0}.
>>>
>>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>>> which correctly expands, without creating useless masking.
>>>
>>>
>>> Basically for me there are two questions:
>>> 1) Can we perform information passing in optabs in a nicer way?
>>> 2) How is_vector_comparison could be improved? I have several ideas,
>>> like checking if constant vector all consists of 0 and -1, and so
>>> on.
>>> But first is it conceptually fine.
>>>
>>> P.S. I tired to put the functionality of is_vector_comparison in
>>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>>> which I find inappropriate, and the functionality gets more
>>> complicated.
>>
>> Why is it inappropriate to not optimize it at -O0? If the user
>> separates comparison and ?: expression it's his own fault.
>
> Well, because all the information is there, and I perfectly envision
> the case when user expressed comparison separately, just to avoid code
> duplication.
>
> Like:
> mask = a > b;
> res1 = mask ? v0 : v1;
> res2 = mask ? v2 : v3;
>
> Which in this case would be different from
> res1 = a > b ? v0 : v1;
> res2 = a > b ? v2 : v3;
>
>> Btw, the new hook is still in the patch.
>>
>> I would simply always create != 0 if it isn't and let optimizers
>> (tree-ssa-forwprop.c) optimize this. You still have to deal with
>> non-comparison operands during expansion though, but if
>> you always forced a != 0 from the start you can then simply
>> interpret it as a proper comparison result (in which case I'd
>> modify the backends to have an alternate pattern or directly
>> expand to masking operations - using the fake comparison
>> RTX is too much of a hack).
>
> Richard, I think you didn't get the problem.
> I really need the way, to pass the information, that the expression
> that is in the first operand of vcond is an appropriate mask. I though
> for quite a while and this hack is the only answer I found, is there a
> better way to do that. I could for example introduce another
> tree-node, but it would be overkill as well.
>
> Now why do I need it so much:
> I want to implement the comparison in a way that {1, 5, 0, -1} is
> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
> VEC_COND_EXPR is a real
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 9:42 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:58 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
> wrote:
>> Richard
>>
>> I formalized an approach a little-bit, now it works without target
>> hooks, but some polishing is still required. I want you to comment on
>> the several important approaches that I use in the patch.
>>
>> So how does it work.
>> 1) All the vector comparisons at the level of type-checker are
>> introduced using VEC_COND_EXPR with constant selection operands being
>> {-1} and {0}. For example v0 > v1 is transformed into
>> VEC_COND_EXPR>> v1, {-1}, {0}>.
>>
>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case
>> nothing changes.
>> 2.b) first operand is something else, in that case, we specially mark
>> this case, recognize it in the backend, and do not create a
>> comparison, but use the mask as it was a result of some comparison.
>>
>> 3) In order to make sure that mask in VEC_COND_EXPR is
>> a
>> vector comparison we use is_vector_comparison function, if it returns
>> false, then we replace mask with mask != {0}.
>>
>> So we end-up with the following functionality:
>> VEC_COND_EXPR -- if we know that mask is a result of
>> comparison of two vectors, we leave it as it is, otherwise change
>> with
>> mask != {0}.
>>
>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>> which correctly expands, without creating useless masking.
>>
>>
>> Basically for me there are two questions:
>> 1) Can we perform information passing in optabs in a nicer way?
>> 2) How is_vector_comparison could be improved? I have several ideas,
>> like checking if constant vector all consists of 0 and -1, and so on.
>> But first is it conceptually fine.
>>
>> P.S. I tired to put the functionality of is_vector_comparison in
>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>> which I find inappropriate, and the functionality gets more
>> complicated.
>
> Why is it inappropriate to not optimize it at -O0? If the user
> separates comparison and ?: expression it's his own fault.
Well, because all the information is there, and I perfectly envision
the case when user expressed comparison separately, just to avoid code
duplication.
Like:
mask = a > b;
res1 = mask ? v0 : v1;
res2 = mask ? v2 : v3;
Which in this case would be different from
res1 = a > b ? v0 : v1;
res2 = a > b ? v2 : v3;
> Btw, the new hook is still in the patch.
>
> I would simply always create != 0 if it isn't and let optimizers
> (tree-ssa-forwprop.c) optimize this. You still have to deal with
> non-comparison operands during expansion though, but if
> you always forced a != 0 from the start you can then simply
> interpret it as a proper comparison result (in which case I'd
> modify the backends to have an alternate pattern or directly
> expand to masking operations - using the fake comparison
> RTX is too much of a hack).
Richard, I think you didn't get the problem.
I really need the way, to pass the information, that the expression
that is in the first operand of vcond is an appropriate mask. I though
for quite a while and this hack is the only answer I found, is there a
better way to do that. I could for example introduce another
tree-node, but it would be overkill as well.
Now why do I need it so much:
I want to implement the comparison in a way that {1, 5, 0, -1} is
actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
always). To check the stuff, I use is_vector_comparison in
tree-vect-generic.
So I really have the difference between mask ? x : y and mask != {0}
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 8:50 PM, Uros Bizjak wrote: > On Mon, Aug 22, 2011 at 5:34 PM, Richard Guenther > wrote: > >>> In this case it is simple to analyse that a is a comparison, but you >>> cannot embed the operations of a into VEC_COND_EXPR. >> >> Sure, but if the above is C source the frontend would generate >> res = a != 0 ? v0 : v1; initially. An optimization pass could still >> track this information and replace VEC_COND_EXPR >> with VEC_COND_EXPR (no existing one would track >> vector contents though). >> >>> Ok, I am testing the patch that removes hooks. Could you push a little >>> bit the backend-patterns business? >> >> Well, I suppose we're waiting for Uros here. I hadn't much luck with >> fiddling with the mode-iterator stuff myself. > > It is not _that_ trivial change, since we have ix86_expand_fp_vcond > and ix86_expand_int_vcond to merge. ATM, FP version deals with FP > operands and vice versa. We have to merge them somehow and split out > comparison part that handles FP as well as integer operands. > > I also don't know why vcond is not allowed to FAIL... probably > middle-end should be enhanced for a fallback if some comparison isn't > supported by optab. > > Uros. > Uros My biggest problem in the backend is to create a correct description in *.md, which would accept the generic case. I can imagine adding all the cases, but as I mentioned already it explodes. I mean, we will have to do it for SSE, then the same number of cases for AVX, and so on. I would assume that there is a chance to persuade md, that the only thing that matters is the size of the element type of mask and operands. If you can help me with the pattern, I am happy to merge x86 code. Thanks, Artem.
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 5:58 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
wrote:
> Richard
>
> I formalized an approach a little-bit, now it works without target
> hooks, but some polishing is still required. I want you to comment on
> the several important approaches that I use in the patch.
>
> So how does it work.
> 1) All the vector comparisons at the level of type-checker are
> introduced using VEC_COND_EXPR with constant selection operands being
> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR> v1, {-1}, {0}>.
>
> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
> 2.a) first operand of VEC_COND_EXPR is comparison, in that case
> nothing changes.
> 2.b) first operand is something else, in that case, we specially mark
> this case, recognize it in the backend, and do not create a
> comparison, but use the mask as it was a result of some comparison.
>
> 3) In order to make sure that mask in VEC_COND_EXPR is a
> vector comparison we use is_vector_comparison function, if it returns
> false, then we replace mask with mask != {0}.
>
> So we end-up with the following functionality:
> VEC_COND_EXPR -- if we know that mask is a result of
> comparison of two vectors, we leave it as it is, otherwise change with
> mask != {0}.
>
> Plain vector comparison a b is represented with VEC_COND_EXPR,
> which correctly expands, without creating useless masking.
>
>
> Basically for me there are two questions:
> 1) Can we perform information passing in optabs in a nicer way?
> 2) How is_vector_comparison could be improved? I have several ideas,
> like checking if constant vector all consists of 0 and -1, and so on.
> But first is it conceptually fine.
>
> P.S. I tired to put the functionality of is_vector_comparison in
> tree-ssa-forwprop, but the thing is that it is called only with -On,
> which I find inappropriate, and the functionality gets more
> complicated.
Why is it inappropriate to not optimize it at -O0? If the user
separates comparison and ?: expression it's his own fault.
>>>
>>> Well, because all the information is there, and I perfectly envision
>>> the case when user expressed comparison separately, just to avoid code
>>> duplication.
>>>
>>> Like:
>>> mask = a > b;
>>> res1 = mask ? v0 : v1;
>>> res2 = mask ? v2 : v3;
>>>
>>> Which in this case would be different from
>>> res1 = a > b ? v0 : v1;
>>> res2 = a > b ? v2 : v3;
>>>
Btw, the new hook is still in the patch.
I would simply always create != 0 if it isn't and let optimizers
(tree-ssa-forwprop.c) optimize this. You still have to deal with
non-comparison operands during expansion though, but if
you always forced a != 0 from the start you can then simply
interpret it as a proper comparison result (in which case I'd
modify the backends to have an alternate pattern or directly
expand to masking operations - using the fake comparison
RTX is too much of a hack).
>>>
>>> Richard, I think you didn't get the problem.
>>> I really need the way, to pass the information, that the expression
>>> that is in the first operand of vcond is an appropriate mask. I though
>>> for quite a while and this hack is the only answer I found, is there a
>>> better way to do that. I could for example introduce another
>>> tree-node, but it would be overkill as well.
>>>
>>> Now why do I need it so much:
>>> I want to implement the comparison in a way that {1, 5, 0, -1} is
>>> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
>>> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
>>> always). To check the stuff, I use is_vector_comparison in
>>> tree-vect-generic.
>>>
>>> So I really have the difference between mask ? x : y and mask != {0} ?
>>> x : y, otherwise I could treat mask != {0} in the backend as just
>>> mask.
>>>
>>> If this link between optabs and backend breaks, then the patch falls
>>> apart. Because
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 9:50 PM, Uros Bizjak wrote: > On Mon, Aug 22, 2011 at 5:34 PM, Richard Guenther > wrote: > >>> In this case it is simple to analyse that a is a comparison, but you >>> cannot embed the operations of a into VEC_COND_EXPR. >> >> Sure, but if the above is C source the frontend would generate >> res = a != 0 ? v0 : v1; initially. An optimization pass could still >> track this information and replace VEC_COND_EXPR >> with VEC_COND_EXPR (no existing one would track >> vector contents though). >> >>> Ok, I am testing the patch that removes hooks. Could you push a little >>> bit the backend-patterns business? >> >> Well, I suppose we're waiting for Uros here. I hadn't much luck with >> fiddling with the mode-iterator stuff myself. > > It is not _that_ trivial change, since we have ix86_expand_fp_vcond > and ix86_expand_int_vcond to merge. ATM, FP version deals with FP > operands and vice versa. We have to merge them somehow and split out > comparison part that handles FP as well as integer operands. Yeah, I tried to keep it split in fp and int compare variants but allow the result mode and the 2nd and 3rd operand modes to vary differently but failed to build a mode iterator that would do this... OTOH I'm not sure how merging fp and int compare modes would simplify things here. > I also don't know why vcond is not allowed to FAIL... probably > middle-end should be enhanced for a fallback if some comparison isn't > supported by optab. The vectorizer currently uses the optab presence to verify if the target supports a given vec-cond-expr. I'm not sure if we can make its test more finegrained easily. Richard. > > Uros. >
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 5:34 PM, Richard Guenther wrote: >> In this case it is simple to analyse that a is a comparison, but you >> cannot embed the operations of a into VEC_COND_EXPR. > > Sure, but if the above is C source the frontend would generate > res = a != 0 ? v0 : v1; initially. An optimization pass could still > track this information and replace VEC_COND_EXPR > with VEC_COND_EXPR (no existing one would track > vector contents though). > >> Ok, I am testing the patch that removes hooks. Could you push a little >> bit the backend-patterns business? > > Well, I suppose we're waiting for Uros here. I hadn't much luck with > fiddling with the mode-iterator stuff myself. It is not _that_ trivial change, since we have ix86_expand_fp_vcond and ix86_expand_int_vcond to merge. ATM, FP version deals with FP operands and vice versa. We have to merge them somehow and split out comparison part that handles FP as well as integer operands. I also don't know why vcond is not allowed to FAIL... probably middle-end should be enhanced for a fallback if some comparison isn't supported by optab. Uros.
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 4:50 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
>>> wrote:
Richard
I formalized an approach a little-bit, now it works without target
hooks, but some polishing is still required. I want you to comment on
the several important approaches that I use in the patch.
So how does it work.
1) All the vector comparisons at the level of type-checker are
introduced using VEC_COND_EXPR with constant selection operands being
{-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR v1, {-1}, {0}>.
2) When optabs expand VEC_COND_EXPR, two cases are considered:
2.a) first operand of VEC_COND_EXPR is comparison, in that case
nothing changes.
2.b) first operand is something else, in that case, we specially mark
this case, recognize it in the backend, and do not create a
comparison, but use the mask as it was a result of some comparison.
3) In order to make sure that mask in VEC_COND_EXPR is a
vector comparison we use is_vector_comparison function, if it returns
false, then we replace mask with mask != {0}.
So we end-up with the following functionality:
VEC_COND_EXPR -- if we know that mask is a result of
comparison of two vectors, we leave it as it is, otherwise change with
mask != {0}.
Plain vector comparison a b is represented with VEC_COND_EXPR,
which correctly expands, without creating useless masking.
Basically for me there are two questions:
1) Can we perform information passing in optabs in a nicer way?
2) How is_vector_comparison could be improved? I have several ideas,
like checking if constant vector all consists of 0 and -1, and so on.
But first is it conceptually fine.
P.S. I tired to put the functionality of is_vector_comparison in
tree-ssa-forwprop, but the thing is that it is called only with -On,
which I find inappropriate, and the functionality gets more
complicated.
>>>
>>> Why is it inappropriate to not optimize it at -O0? If the user
>>> separates comparison and ?: expression it's his own fault.
>>
>> Well, because all the information is there, and I perfectly envision
>> the case when user expressed comparison separately, just to avoid code
>> duplication.
>>
>> Like:
>> mask = a > b;
>> res1 = mask ? v0 : v1;
>> res2 = mask ? v2 : v3;
>>
>> Which in this case would be different from
>> res1 = a > b ? v0 : v1;
>> res2 = a > b ? v2 : v3;
>>
>>> Btw, the new hook is still in the patch.
>>>
>>> I would simply always create != 0 if it isn't and let optimizers
>>> (tree-ssa-forwprop.c) optimize this. You still have to deal with
>>> non-comparison operands during expansion though, but if
>>> you always forced a != 0 from the start you can then simply
>>> interpret it as a proper comparison result (in which case I'd
>>> modify the backends to have an alternate pattern or directly
>>> expand to masking operations - using the fake comparison
>>> RTX is too much of a hack).
>>
>> Richard, I think you didn't get the problem.
>> I really need the way, to pass the information, that the expression
>> that is in the first operand of vcond is an appropriate mask. I though
>> for quite a while and this hack is the only answer I found, is there a
>> better way to do that. I could for example introduce another
>> tree-node, but it would be overkill as well.
>>
>> Now why do I need it so much:
>> I want to implement the comparison in a way that {1, 5, 0, -1} is
>> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
>> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
>> always). To check the stuff, I use is_vector_comparison in
>> tree-vect-generic.
>>
>> So I really have the difference between mask ? x : y and mask != {0} ?
>> x : y, otherwise I could treat mask != {0} in the backend as just
>> mask.
>>
>> If this link between optabs and backend breaks, then the patch falls
>> apart. Because every time the comparison is taken out VEC_COND_EXPR, I
>> will have to put != {0}. Keep in mind, that I cannot always put the
>> comparison inside the VEC_C
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 5:43 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
>> wrote:
>>> Richard
>>>
>>> I formalized an approach a little-bit, now it works without target
>>> hooks, but some polishing is still required. I want you to comment on
>>> the several important approaches that I use in the patch.
>>>
>>> So how does it work.
>>> 1) All the vector comparisons at the level of type-checker are
>>> introduced using VEC_COND_EXPR with constant selection operands being
>>> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR>>> v1, {-1}, {0}>.
>>>
>>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
>>> changes.
>>> 2.b) first operand is something else, in that case, we specially mark
>>> this case, recognize it in the backend, and do not create a
>>> comparison, but use the mask as it was a result of some comparison.
>>>
>>> 3) In order to make sure that mask in VEC_COND_EXPR is a
>>> vector comparison we use is_vector_comparison function, if it returns
>>> false, then we replace mask with mask != {0}.
>>>
>>> So we end-up with the following functionality:
>>> VEC_COND_EXPR -- if we know that mask is a result of
>>> comparison of two vectors, we leave it as it is, otherwise change with
>>> mask != {0}.
>>>
>>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>>> which correctly expands, without creating useless masking.
>>>
>>>
>>> Basically for me there are two questions:
>>> 1) Can we perform information passing in optabs in a nicer way?
>>> 2) How is_vector_comparison could be improved? I have several ideas,
>>> like checking if constant vector all consists of 0 and -1, and so on.
>>> But first is it conceptually fine.
>>>
>>> P.S. I tired to put the functionality of is_vector_comparison in
>>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>>> which I find inappropriate, and the functionality gets more
>>> complicated.
>>
>> Why is it inappropriate to not optimize it at -O0? If the user
>> separates comparison and ?: expression it's his own fault.
>
> Well, because all the information is there, and I perfectly envision
> the case when user expressed comparison separately, just to avoid code
> duplication.
>
> Like:
> mask = a > b;
> res1 = mask ? v0 : v1;
> res2 = mask ? v2 : v3;
>
> Which in this case would be different from
> res1 = a > b ? v0 : v1;
> res2 = a > b ? v2 : v3;
>
>> Btw, the new hook is still in the patch.
>>
>> I would simply always create != 0 if it isn't and let optimizers
>> (tree-ssa-forwprop.c) optimize this. You still have to deal with
>> non-comparison operands during expansion though, but if
>> you always forced a != 0 from the start you can then simply
>> interpret it as a proper comparison result (in which case I'd
>> modify the backends to have an alternate pattern or directly
>> expand to masking operations - using the fake comparison
>> RTX is too much of a hack).
>
> Richard, I think you didn't get the problem.
> I really need the way, to pass the information, that the expression
> that is in the first operand of vcond is an appropriate mask. I though
> for quite a while and this hack is the only answer I found, is there a
> better way to do that. I could for example introduce another
> tree-node, but it would be overkill as well.
>
> Now why do I need it so much:
> I want to implement the comparison in a way that {1, 5, 0, -1} is
> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
> always). To check the stuff, I use is_vector_comparison in
> tree-vect-generic.
>
> So I really have the difference between mask ? x : y and mask != {0} ?
> x : y, otherwise I could treat mask != {0} in the backend as just
> mask.
>
> If this link between optabs and backend breaks, then the patch falls
> apart. Because every time the comparison is taken out VEC_COND_EXPR, I
> will have to put != {0}. Keep in mind, that I cannot always put the
> comparison inside the VEC_COND_EXPR, what if it is defined in a
> function-comparison, or somehow else?
>
> So what would be an appropriate way to connect optabs and the backend?
>>
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 4:34 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>>> wrote:
On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
> wrote:
>> Richard
>>
>> I formalized an approach a little-bit, now it works without target
>> hooks, but some polishing is still required. I want you to comment on
>> the several important approaches that I use in the patch.
>>
>> So how does it work.
>> 1) All the vector comparisons at the level of type-checker are
>> introduced using VEC_COND_EXPR with constant selection operands being
>> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR>> v1, {-1}, {0}>.
>>
>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
>> changes.
>> 2.b) first operand is something else, in that case, we specially mark
>> this case, recognize it in the backend, and do not create a
>> comparison, but use the mask as it was a result of some comparison.
>>
>> 3) In order to make sure that mask in VEC_COND_EXPR is a
>> vector comparison we use is_vector_comparison function, if it returns
>> false, then we replace mask with mask != {0}.
>>
>> So we end-up with the following functionality:
>> VEC_COND_EXPR -- if we know that mask is a result of
>> comparison of two vectors, we leave it as it is, otherwise change with
>> mask != {0}.
>>
>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>> which correctly expands, without creating useless masking.
>>
>>
>> Basically for me there are two questions:
>> 1) Can we perform information passing in optabs in a nicer way?
>> 2) How is_vector_comparison could be improved? I have several ideas,
>> like checking if constant vector all consists of 0 and -1, and so on.
>> But first is it conceptually fine.
>>
>> P.S. I tired to put the functionality of is_vector_comparison in
>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>> which I find inappropriate, and the functionality gets more
>> complicated.
>
> Why is it inappropriate to not optimize it at -O0? If the user
> separates comparison and ?: expression it's his own fault.
Well, because all the information is there, and I perfectly envision
the case when user expressed comparison separately, just to avoid code
duplication.
Like:
mask = a > b;
res1 = mask ? v0 : v1;
res2 = mask ? v2 : v3;
Which in this case would be different from
res1 = a > b ? v0 : v1;
res2 = a > b ? v2 : v3;
> Btw, the new hook is still in the patch.
>
> I would simply always create != 0 if it isn't and let optimizers
> (tree-ssa-forwprop.c) optimize this. You still have to deal with
> non-comparison operands during expansion though, but if
> you always forced a != 0 from the start you can then simply
> interpret it as a proper comparison result (in which case I'd
> modify the backends to have an alternate pattern or directly
> expand to masking operations - using the fake comparison
> RTX is too much of a hack).
Richard, I think you didn't get the problem.
I really need the way, to pass the information, that the expression
that is in the first operand of vcond is an appropriate mask. I though
for quite a while and this hack is the only answer I found, is there a
better way to do that. I could for example introduce another
tree-node, but it would be overkill as well.
Now why do I need it so much:
I want to implement the comparison in a way that {1, 5, 0, -1} is
actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
always). To check the stuff, I use is_vector_comparison in
tree-vect-generic.
So I really have the difference between mask ? x : y and mask != {0} ?
x : y, otherwise I could treat mask != {0} in the backend as just
mask.
If this link between optabs and backend breaks, then the patch falls
apart. Because every time the comparison is taken out VEC_COND_EXPR, I
will have to put != {0}. Keep in mind, that I cannot always put the
comparison inside the VEC_COND_EXPR, what if it is defined in a
function-comparison, or somehow else?
So what would be an appropriate way to connect optabs and the backend?
>>>
>>> Well, there is no problem in having the only valid mask operand for
>>> VEC_COND_EXPRs being either a comparison or a {-1,...} / {0,} mask.
>>> Just the C p
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 5:21 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
>> wrote:
>>> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
>>> wrote:
On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
wrote:
> Richard
>
> I formalized an approach a little-bit, now it works without target
> hooks, but some polishing is still required. I want you to comment on
> the several important approaches that I use in the patch.
>
> So how does it work.
> 1) All the vector comparisons at the level of type-checker are
> introduced using VEC_COND_EXPR with constant selection operands being
> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR> v1, {-1}, {0}>.
>
> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
> changes.
> 2.b) first operand is something else, in that case, we specially mark
> this case, recognize it in the backend, and do not create a
> comparison, but use the mask as it was a result of some comparison.
>
> 3) In order to make sure that mask in VEC_COND_EXPR is a
> vector comparison we use is_vector_comparison function, if it returns
> false, then we replace mask with mask != {0}.
>
> So we end-up with the following functionality:
> VEC_COND_EXPR -- if we know that mask is a result of
> comparison of two vectors, we leave it as it is, otherwise change with
> mask != {0}.
>
> Plain vector comparison a b is represented with VEC_COND_EXPR,
> which correctly expands, without creating useless masking.
>
>
> Basically for me there are two questions:
> 1) Can we perform information passing in optabs in a nicer way?
> 2) How is_vector_comparison could be improved? I have several ideas,
> like checking if constant vector all consists of 0 and -1, and so on.
> But first is it conceptually fine.
>
> P.S. I tired to put the functionality of is_vector_comparison in
> tree-ssa-forwprop, but the thing is that it is called only with -On,
> which I find inappropriate, and the functionality gets more
> complicated.
Why is it inappropriate to not optimize it at -O0? If the user
separates comparison and ?: expression it's his own fault.
>>>
>>> Well, because all the information is there, and I perfectly envision
>>> the case when user expressed comparison separately, just to avoid code
>>> duplication.
>>>
>>> Like:
>>> mask = a > b;
>>> res1 = mask ? v0 : v1;
>>> res2 = mask ? v2 : v3;
>>>
>>> Which in this case would be different from
>>> res1 = a > b ? v0 : v1;
>>> res2 = a > b ? v2 : v3;
>>>
Btw, the new hook is still in the patch.
I would simply always create != 0 if it isn't and let optimizers
(tree-ssa-forwprop.c) optimize this. You still have to deal with
non-comparison operands during expansion though, but if
you always forced a != 0 from the start you can then simply
interpret it as a proper comparison result (in which case I'd
modify the backends to have an alternate pattern or directly
expand to masking operations - using the fake comparison
RTX is too much of a hack).
>>>
>>> Richard, I think you didn't get the problem.
>>> I really need the way, to pass the information, that the expression
>>> that is in the first operand of vcond is an appropriate mask. I though
>>> for quite a while and this hack is the only answer I found, is there a
>>> better way to do that. I could for example introduce another
>>> tree-node, but it would be overkill as well.
>>>
>>> Now why do I need it so much:
>>> I want to implement the comparison in a way that {1, 5, 0, -1} is
>>> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
>>> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
>>> always). To check the stuff, I use is_vector_comparison in
>>> tree-vect-generic.
>>>
>>> So I really have the difference between mask ? x : y and mask != {0} ?
>>> x : y, otherwise I could treat mask != {0} in the backend as just
>>> mask.
>>>
>>> If this link between optabs and backend breaks, then the patch falls
>>> apart. Because every time the comparison is taken out VEC_COND_EXPR, I
>>> will have to put != {0}. Keep in mind, that I cannot always put the
>>> comparison inside the VEC_COND_EXPR, what if it is defined in a
>>> function-comparison, or somehow else?
>>>
>>> So what would be an appropriate way to connect optabs and the backend?
>>
>> Well, there is no problem in having the only valid mask operand for
>> VEC_COND_EXPRs being either a comparison or a {-1,...} / {0,} mask.
>> Just the C parser has to transform mask ? vec1 : vec2 to mask != 0 ?
>> vec1 : vec2.
>
> This happens already in the new version of patch (not submitted yet).
>
>> This comparison c
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 4:01 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
>> wrote:
>>> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
>>> wrote:
Richard
I formalized an approach a little-bit, now it works without target
hooks, but some polishing is still required. I want you to comment on
the several important approaches that I use in the patch.
So how does it work.
1) All the vector comparisons at the level of type-checker are
introduced using VEC_COND_EXPR with constant selection operands being
{-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR v1, {-1}, {0}>.
2) When optabs expand VEC_COND_EXPR, two cases are considered:
2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
changes.
2.b) first operand is something else, in that case, we specially mark
this case, recognize it in the backend, and do not create a
comparison, but use the mask as it was a result of some comparison.
3) In order to make sure that mask in VEC_COND_EXPR is a
vector comparison we use is_vector_comparison function, if it returns
false, then we replace mask with mask != {0}.
So we end-up with the following functionality:
VEC_COND_EXPR -- if we know that mask is a result of
comparison of two vectors, we leave it as it is, otherwise change with
mask != {0}.
Plain vector comparison a b is represented with VEC_COND_EXPR,
which correctly expands, without creating useless masking.
Basically for me there are two questions:
1) Can we perform information passing in optabs in a nicer way?
2) How is_vector_comparison could be improved? I have several ideas,
like checking if constant vector all consists of 0 and -1, and so on.
But first is it conceptually fine.
P.S. I tired to put the functionality of is_vector_comparison in
tree-ssa-forwprop, but the thing is that it is called only with -On,
which I find inappropriate, and the functionality gets more
complicated.
>>>
>>> Why is it inappropriate to not optimize it at -O0? If the user
>>> separates comparison and ?: expression it's his own fault.
>>
>> Well, because all the information is there, and I perfectly envision
>> the case when user expressed comparison separately, just to avoid code
>> duplication.
>>
>> Like:
>> mask = a > b;
>> res1 = mask ? v0 : v1;
>> res2 = mask ? v2 : v3;
>>
>> Which in this case would be different from
>> res1 = a > b ? v0 : v1;
>> res2 = a > b ? v2 : v3;
>>
>>> Btw, the new hook is still in the patch.
>>>
>>> I would simply always create != 0 if it isn't and let optimizers
>>> (tree-ssa-forwprop.c) optimize this. You still have to deal with
>>> non-comparison operands during expansion though, but if
>>> you always forced a != 0 from the start you can then simply
>>> interpret it as a proper comparison result (in which case I'd
>>> modify the backends to have an alternate pattern or directly
>>> expand to masking operations - using the fake comparison
>>> RTX is too much of a hack).
>>
>> Richard, I think you didn't get the problem.
>> I really need the way, to pass the information, that the expression
>> that is in the first operand of vcond is an appropriate mask. I though
>> for quite a while and this hack is the only answer I found, is there a
>> better way to do that. I could for example introduce another
>> tree-node, but it would be overkill as well.
>>
>> Now why do I need it so much:
>> I want to implement the comparison in a way that {1, 5, 0, -1} is
>> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
>> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
>> always). To check the stuff, I use is_vector_comparison in
>> tree-vect-generic.
>>
>> So I really have the difference between mask ? x : y and mask != {0} ?
>> x : y, otherwise I could treat mask != {0} in the backend as just
>> mask.
>>
>> If this link between optabs and backend breaks, then the patch falls
>> apart. Because every time the comparison is taken out VEC_COND_EXPR, I
>> will have to put != {0}. Keep in mind, that I cannot always put the
>> comparison inside the VEC_COND_EXPR, what if it is defined in a
>> function-comparison, or somehow else?
>>
>> So what would be an appropriate way to connect optabs and the backend?
>
> Well, there is no problem in having the only valid mask operand for
> VEC_COND_EXPRs being either a comparison or a {-1,...} / {0,} mask.
> Just the C parser has to transform mask ? vec1 : vec2 to mask != 0 ?
> vec1 : vec2.
This happens already in the new version of patch (not submitted yet).
> This comparison can be eliminated by optimization passes
> that
> either replace it by the real comparison computing the mask or just
> propagating the information this mask is already {-1,
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 2:05 PM, Artem Shinkarov
wrote:
> On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
> wrote:
>> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
>> wrote:
>>> Richard
>>>
>>> I formalized an approach a little-bit, now it works without target
>>> hooks, but some polishing is still required. I want you to comment on
>>> the several important approaches that I use in the patch.
>>>
>>> So how does it work.
>>> 1) All the vector comparisons at the level of type-checker are
>>> introduced using VEC_COND_EXPR with constant selection operands being
>>> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR>>> v1, {-1}, {0}>.
>>>
>>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
>>> changes.
>>> 2.b) first operand is something else, in that case, we specially mark
>>> this case, recognize it in the backend, and do not create a
>>> comparison, but use the mask as it was a result of some comparison.
>>>
>>> 3) In order to make sure that mask in VEC_COND_EXPR is a
>>> vector comparison we use is_vector_comparison function, if it returns
>>> false, then we replace mask with mask != {0}.
>>>
>>> So we end-up with the following functionality:
>>> VEC_COND_EXPR -- if we know that mask is a result of
>>> comparison of two vectors, we leave it as it is, otherwise change with
>>> mask != {0}.
>>>
>>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>>> which correctly expands, without creating useless masking.
>>>
>>>
>>> Basically for me there are two questions:
>>> 1) Can we perform information passing in optabs in a nicer way?
>>> 2) How is_vector_comparison could be improved? I have several ideas,
>>> like checking if constant vector all consists of 0 and -1, and so on.
>>> But first is it conceptually fine.
>>>
>>> P.S. I tired to put the functionality of is_vector_comparison in
>>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>>> which I find inappropriate, and the functionality gets more
>>> complicated.
>>
>> Why is it inappropriate to not optimize it at -O0? If the user
>> separates comparison and ?: expression it's his own fault.
>
> Well, because all the information is there, and I perfectly envision
> the case when user expressed comparison separately, just to avoid code
> duplication.
>
> Like:
> mask = a > b;
> res1 = mask ? v0 : v1;
> res2 = mask ? v2 : v3;
>
> Which in this case would be different from
> res1 = a > b ? v0 : v1;
> res2 = a > b ? v2 : v3;
>
>> Btw, the new hook is still in the patch.
>>
>> I would simply always create != 0 if it isn't and let optimizers
>> (tree-ssa-forwprop.c) optimize this. You still have to deal with
>> non-comparison operands during expansion though, but if
>> you always forced a != 0 from the start you can then simply
>> interpret it as a proper comparison result (in which case I'd
>> modify the backends to have an alternate pattern or directly
>> expand to masking operations - using the fake comparison
>> RTX is too much of a hack).
>
> Richard, I think you didn't get the problem.
> I really need the way, to pass the information, that the expression
> that is in the first operand of vcond is an appropriate mask. I though
> for quite a while and this hack is the only answer I found, is there a
> better way to do that. I could for example introduce another
> tree-node, but it would be overkill as well.
>
> Now why do I need it so much:
> I want to implement the comparison in a way that {1, 5, 0, -1} is
> actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
> VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
> always). To check the stuff, I use is_vector_comparison in
> tree-vect-generic.
>
> So I really have the difference between mask ? x : y and mask != {0} ?
> x : y, otherwise I could treat mask != {0} in the backend as just
> mask.
>
> If this link between optabs and backend breaks, then the patch falls
> apart. Because every time the comparison is taken out VEC_COND_EXPR, I
> will have to put != {0}. Keep in mind, that I cannot always put the
> comparison inside the VEC_COND_EXPR, what if it is defined in a
> function-comparison, or somehow else?
>
> So what would be an appropriate way to connect optabs and the backend?
Well, there is no problem in having the only valid mask operand for
VEC_COND_EXPRs being either a comparison or a {-1,...} / {0,} mask.
Just the C parser has to transform mask ? vec1 : vec2 to mask != 0 ?
vec1 : vec2. This comparison can be eliminated by optimization passes
that
either replace it by the real comparison computing the mask or just
propagating the information this mask is already {-1,...} / {0,} by simply
dropping the comparison against zero.
For the backends I'd have vcond patterns for both an embedded comparison
and for a mask. (Now we can rewind the discussion a bit and allow
arbitrary masks and define a vcond with a ma
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 12:25 PM, Richard Guenther
wrote:
> On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
> wrote:
>> Richard
>>
>> I formalized an approach a little-bit, now it works without target
>> hooks, but some polishing is still required. I want you to comment on
>> the several important approaches that I use in the patch.
>>
>> So how does it work.
>> 1) All the vector comparisons at the level of type-checker are
>> introduced using VEC_COND_EXPR with constant selection operands being
>> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR>> v1, {-1}, {0}>.
>>
>> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
>> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
>> changes.
>> 2.b) first operand is something else, in that case, we specially mark
>> this case, recognize it in the backend, and do not create a
>> comparison, but use the mask as it was a result of some comparison.
>>
>> 3) In order to make sure that mask in VEC_COND_EXPR is a
>> vector comparison we use is_vector_comparison function, if it returns
>> false, then we replace mask with mask != {0}.
>>
>> So we end-up with the following functionality:
>> VEC_COND_EXPR -- if we know that mask is a result of
>> comparison of two vectors, we leave it as it is, otherwise change with
>> mask != {0}.
>>
>> Plain vector comparison a b is represented with VEC_COND_EXPR,
>> which correctly expands, without creating useless masking.
>>
>>
>> Basically for me there are two questions:
>> 1) Can we perform information passing in optabs in a nicer way?
>> 2) How is_vector_comparison could be improved? I have several ideas,
>> like checking if constant vector all consists of 0 and -1, and so on.
>> But first is it conceptually fine.
>>
>> P.S. I tired to put the functionality of is_vector_comparison in
>> tree-ssa-forwprop, but the thing is that it is called only with -On,
>> which I find inappropriate, and the functionality gets more
>> complicated.
>
> Why is it inappropriate to not optimize it at -O0? If the user
> separates comparison and ?: expression it's his own fault.
Well, because all the information is there, and I perfectly envision
the case when user expressed comparison separately, just to avoid code
duplication.
Like:
mask = a > b;
res1 = mask ? v0 : v1;
res2 = mask ? v2 : v3;
Which in this case would be different from
res1 = a > b ? v0 : v1;
res2 = a > b ? v2 : v3;
> Btw, the new hook is still in the patch.
>
> I would simply always create != 0 if it isn't and let optimizers
> (tree-ssa-forwprop.c) optimize this. You still have to deal with
> non-comparison operands during expansion though, but if
> you always forced a != 0 from the start you can then simply
> interpret it as a proper comparison result (in which case I'd
> modify the backends to have an alternate pattern or directly
> expand to masking operations - using the fake comparison
> RTX is too much of a hack).
Richard, I think you didn't get the problem.
I really need the way, to pass the information, that the expression
that is in the first operand of vcond is an appropriate mask. I though
for quite a while and this hack is the only answer I found, is there a
better way to do that. I could for example introduce another
tree-node, but it would be overkill as well.
Now why do I need it so much:
I want to implement the comparison in a way that {1, 5, 0, -1} is
actually {-1,-1,-1,-1}. So whenever I am not sure that mask of
VEC_COND_EXPR is a real comparison I transform it to mask != {0} (not
always). To check the stuff, I use is_vector_comparison in
tree-vect-generic.
So I really have the difference between mask ? x : y and mask != {0} ?
x : y, otherwise I could treat mask != {0} in the backend as just
mask.
If this link between optabs and backend breaks, then the patch falls
apart. Because every time the comparison is taken out VEC_COND_EXPR, I
will have to put != {0}. Keep in mind, that I cannot always put the
comparison inside the VEC_COND_EXPR, what if it is defined in a
function-comparison, or somehow else?
So what would be an appropriate way to connect optabs and the backend?
Thanks,
Artem.
All the rest would be adjusted later.
> tree
> constant_boolean_node (int value, tree type)
> {
> - if (type == integer_type_node)
> + if (TREE_CODE (type) == VECTOR_TYPE)
> + {
> + tree tval;
> +
> + gcc_assert (TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE);
> + tval = build_int_cst (TREE_TYPE (type), value);
> + return build_vector_from_val (type, tval);
>
> as value is either 0 or 1 that won't work. Oh, I see you pass -1
> for true in the callers. But I think we should simply decide that true (1)
> means -1 for a vector boolean node (and the value parameter should
> be a bool instead). Thus,
>
> + if (TREE_CODE (type) == VECTOR_TYPE)
> + {
> + tree tval;
> +
> + gcc_assert (TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE);
> + tval = build_int_cst (TREE_TYPE (type),
Re: Vector Comparison patch
On Mon, Aug 22, 2011 at 12:53 AM, Artem Shinkarov
wrote:
> Richard
>
> I formalized an approach a little-bit, now it works without target
> hooks, but some polishing is still required. I want you to comment on
> the several important approaches that I use in the patch.
>
> So how does it work.
> 1) All the vector comparisons at the level of type-checker are
> introduced using VEC_COND_EXPR with constant selection operands being
> {-1} and {0}. For example v0 > v1 is transformed into VEC_COND_EXPR> v1, {-1}, {0}>.
>
> 2) When optabs expand VEC_COND_EXPR, two cases are considered:
> 2.a) first operand of VEC_COND_EXPR is comparison, in that case nothing
> changes.
> 2.b) first operand is something else, in that case, we specially mark
> this case, recognize it in the backend, and do not create a
> comparison, but use the mask as it was a result of some comparison.
>
> 3) In order to make sure that mask in VEC_COND_EXPR is a
> vector comparison we use is_vector_comparison function, if it returns
> false, then we replace mask with mask != {0}.
>
> So we end-up with the following functionality:
> VEC_COND_EXPR -- if we know that mask is a result of
> comparison of two vectors, we leave it as it is, otherwise change with
> mask != {0}.
>
> Plain vector comparison a b is represented with VEC_COND_EXPR,
> which correctly expands, without creating useless masking.
>
>
> Basically for me there are two questions:
> 1) Can we perform information passing in optabs in a nicer way?
> 2) How is_vector_comparison could be improved? I have several ideas,
> like checking if constant vector all consists of 0 and -1, and so on.
> But first is it conceptually fine.
>
> P.S. I tired to put the functionality of is_vector_comparison in
> tree-ssa-forwprop, but the thing is that it is called only with -On,
> which I find inappropriate, and the functionality gets more
> complicated.
Why is it inappropriate to not optimize it at -O0? If the user
separates comparison and ?: expression it's his own fault.
Btw, the new hook is still in the patch.
I would simply always create != 0 if it isn't and let optimizers
(tree-ssa-forwprop.c) optimize this. You still have to deal with
non-comparison operands during expansion though, but if
you always forced a != 0 from the start you can then simply
interpret it as a proper comparison result (in which case I'd
modify the backends to have an alternate pattern or directly
expand to masking operations - using the fake comparison
RTX is too much of a hack).
tree
constant_boolean_node (int value, tree type)
{
- if (type == integer_type_node)
+ if (TREE_CODE (type) == VECTOR_TYPE)
+{
+ tree tval;
+
+ gcc_assert (TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE);
+ tval = build_int_cst (TREE_TYPE (type), value);
+ return build_vector_from_val (type, tval);
as value is either 0 or 1 that won't work. Oh, I see you pass -1
for true in the callers. But I think we should simply decide that true (1)
means -1 for a vector boolean node (and the value parameter should
be a bool instead). Thus,
+ if (TREE_CODE (type) == VECTOR_TYPE)
+{
+ tree tval;
+
+ gcc_assert (TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE);
+ tval = build_int_cst (TREE_TYPE (type), value ? -1 : 0);
+ return build_vector_from_val (type, tval);
instead.
@@ -9073,26 +9082,29 @@ fold_comparison (location_t loc, enum tr
floating-point, we can only do some of these simplifications.) */
if (operand_equal_p (arg0, arg1, 0))
{
+ int true_val = TREE_CODE (type) == VECTOR_TYPE ? -1 : 0;
+ tree arg0_type = TREE_TYPE (arg0);
+
as I said this is not necessary - the FLOAT_TYPE_P and HONOR_NANS
macros work perfectly fine on vector types.
Richard.
>
> Thanks,
> Artem.
>
Re: Vector Comparison patch
On Wed, Aug 17, 2011 at 11:49 AM, Richard Guenther
wrote:
>>> Hm, ok ... let's hope we can sort-out the backend issues before this
>>> patch goes in so we can remove this converting stuff.
>>
>> Hm, I would hope that we could commit this patch even with this issue,
>> because my feeling is that this case would produce errors on all the
>> other architectures as well, as VEC_COND_EXPR is the feature heavily
>> used in auto-vectorizer. So it means that all the backends must be
>> fixed. And another argument, that this conversion is harmless.
>
> It shouldn't be hard to fix all the backends. And if we don't do it now
> it will never happen. I would expect that the codegen part of the
> backends doesn't need any adjustments, just the patterns that
> match what is supported.
>
> Uros, can you convert x86 as an example? Thus, for
>
> (define_expand "vcond"
> [(set (match_operand:VF 0 "register_operand" "")
> (if_then_else:VF
> (match_operator 3 ""
> [(match_operand:VF 4 "nonimmediate_operand" "")
> (match_operand:VF 5 "nonimmediate_operand" "")])
> (match_operand:VF 1 "general_operand" "")
> (match_operand:VF 2 "general_operand" "")))]
> "TARGET_SSE"
> {
> bool ok = ix86_expand_fp_vcond (operands);
> gcc_assert (ok);
> allow any vector mode of the same size (and same number of elements?)
> for the vcond mode and operand 1 and 2? Thus, only restrict the
> embedded comparison to VF?
I am a bit late to this discussion, but I see no problem for the
backend to relax this restriction. I will look into it.
Uros.
Re: Vector Comparison patch
On Fri, Aug 19, 2011 at 5:22 PM, Artem Shinkarov
wrote:
> On Fri, Aug 19, 2011 at 3:54 PM, Richard Guenther
> wrote:
>> On Fri, Aug 19, 2011 at 2:29 AM, Artem Shinkarov
>> wrote:
>>> Hi, I had the problem with passing information about single variable
>>> from expand_vec_cond_expr optab into ix86_expand_*_vcond.
>>>
>>> I looked into it this problem for quite a while and found a solution.
>>> Now the question if it could be done better.
>>>
>>> First of all the problem:
>>>
>>> If we represent any vector comparison with VEC_COND_EXPR < v0 v1
>>> ? {-1,...} : {0,...} >, then in the assembler we do not want to see
>>> this useless comparison with {-1...}.
>>>
>>> Now it is easy to fix the problem about excessive masking. The real
>>> challenge starts when the comparison inside vcond is expressed as a
>>> variable. In that case in order to construct correct vector expression
>>> we need to adjust cond in cond ? v0 : v1 to cond == {-1...} or as we
>>> agreed recently cond != {0,..}. But hat we need to do only to make
>>> vec_cond_expr happy. On the level of assembler we don't want this
>>> condition.
>>>
>>> Now, if I just construct the tree, then in x86, rtx_equal_p, does not
>>> know that this is a constant vector full of -1, because the comparison
>>> operands are not immediate. So I need somehow to mark the fact in
>>> optabs, and then check the information in the x86.
>>
>> Well, this is why I was suggesting the bitwise semantic for a mask
>> operand. What we should do on the tree level (and that should happen
>> already), is forward the comparison into the COND_EXPR. Thus,
>>
>> mask = v1 < v2;
>> v3 = mask ? v4 : v5;
>>
>> should get changed to
>>
>> v3 = v1 < v2 ? v4 : v5;
>>
>> by tree-ssa-forwprop.c. If that is not happening we have to fix that there.
>
> Yeah, that is something I am working on.
>
>> Because we _don't_ know the mask is all -1 or 0 ;) The user might
>> put in {3, 5 ,1 3} and expect it to be treated like {-1,...} but it isn't
>> so already.
>>
>>> At the moment I do something like this:
>>>
>>> optabs:
>>>
>>> if (!COMPARISON_CLASS_P (op0))
>>> ops[3] = gen_rtx_EQ (mode, NULL_RTX, NULL_RTX);
>>>
>>> This expression is preserved while checking and verifying.
>>>
>>> ix86:
>>> if (GET_CODE (comp) == EQ && XEXP (comp, 0) == NULL_RTX
>>> && XEXP (comp, 1) == NULL_RTX)
>>>
>>> See the patch attached for more details. The patch is just to give you
>>> an idea of the way I am doing it and it seems to work. Please don't
>>> criticise the patch itself, better help me to understand if there is a
>>> better way to pass the information from optabs to ix86.
>>
>> Hm, I'm not sure the expand_vec_cond_expr will work that way,
>> I'd have to play with it myself (but will now be running for weekend).
>>
>> Is the special-casing of a < b ? {-1,-1,-1} : {0,0,0,0} in the backend
>> working for you? I think there are probably some rtl all-ones and all-zeros
>> predicates you can re-use.
>>
>> Richard.
>
> It works fine. Masks all ones and all zeroes are predefined, all -1
> are not, but I am switching to all zeroes. The real question is that
All -1 is the same as all ones.
> this special case of comparison with two empty operands is a little
> bit hackish. On the other hand there should be no problem with that,
I didn't mean this special case which I believe is incorrect anyways
due to the above comment, but the special case resulting from
expanding v1 < v2 as v1 < v2 ? {-1,-1...} : {0,0,...}.
> because operand 3 is used only to get the code of comparison, noone is
> looking inside the arguments, so we could use this fact. The question
> is whether there is a better way.
As I said above, we can't rely on the mask being either {-1,...} or {0,...}.
If we can, then we should have propagated a comparison, otherwise
we need a real != compare with { 0,}.
> Thanks,
> Artem.
>
Re: Vector Comparison patch
On Fri, Aug 19, 2011 at 3:54 PM, Richard Guenther
wrote:
> On Fri, Aug 19, 2011 at 2:29 AM, Artem Shinkarov
> wrote:
>> Hi, I had the problem with passing information about single variable
>> from expand_vec_cond_expr optab into ix86_expand_*_vcond.
>>
>> I looked into it this problem for quite a while and found a solution.
>> Now the question if it could be done better.
>>
>> First of all the problem:
>>
>> If we represent any vector comparison with VEC_COND_EXPR < v0 v1
>> ? {-1,...} : {0,...} >, then in the assembler we do not want to see
>> this useless comparison with {-1...}.
>>
>> Now it is easy to fix the problem about excessive masking. The real
>> challenge starts when the comparison inside vcond is expressed as a
>> variable. In that case in order to construct correct vector expression
>> we need to adjust cond in cond ? v0 : v1 to cond == {-1...} or as we
>> agreed recently cond != {0,..}. But hat we need to do only to make
>> vec_cond_expr happy. On the level of assembler we don't want this
>> condition.
>>
>> Now, if I just construct the tree, then in x86, rtx_equal_p, does not
>> know that this is a constant vector full of -1, because the comparison
>> operands are not immediate. So I need somehow to mark the fact in
>> optabs, and then check the information in the x86.
>
> Well, this is why I was suggesting the bitwise semantic for a mask
> operand. What we should do on the tree level (and that should happen
> already), is forward the comparison into the COND_EXPR. Thus,
>
> mask = v1 < v2;
> v3 = mask ? v4 : v5;
>
> should get changed to
>
> v3 = v1 < v2 ? v4 : v5;
>
> by tree-ssa-forwprop.c. If that is not happening we have to fix that there.
Yeah, that is something I am working on.
> Because we _don't_ know the mask is all -1 or 0 ;) The user might
> put in {3, 5 ,1 3} and expect it to be treated like {-1,...} but it isn't
> so already.
>
>> At the moment I do something like this:
>>
>> optabs:
>>
>> if (!COMPARISON_CLASS_P (op0))
>> ops[3] = gen_rtx_EQ (mode, NULL_RTX, NULL_RTX);
>>
>> This expression is preserved while checking and verifying.
>>
>> ix86:
>> if (GET_CODE (comp) == EQ && XEXP (comp, 0) == NULL_RTX
>> && XEXP (comp, 1) == NULL_RTX)
>>
>> See the patch attached for more details. The patch is just to give you
>> an idea of the way I am doing it and it seems to work. Please don't
>> criticise the patch itself, better help me to understand if there is a
>> better way to pass the information from optabs to ix86.
>
> Hm, I'm not sure the expand_vec_cond_expr will work that way,
> I'd have to play with it myself (but will now be running for weekend).
>
> Is the special-casing of a < b ? {-1,-1,-1} : {0,0,0,0} in the backend
> working for you? I think there are probably some rtl all-ones and all-zeros
> predicates you can re-use.
>
> Richard.
It works fine. Masks all ones and all zeroes are predefined, all -1
are not, but I am switching to all zeroes. The real question is that
this special case of comparison with two empty operands is a little
bit hackish. On the other hand there should be no problem with that,
because operand 3 is used only to get the code of comparison, noone is
looking inside the arguments, so we could use this fact. The question
is whether there is a better way.
Thanks,
Artem.
Re: Vector Comparison patch
On Fri, Aug 19, 2011 at 2:29 AM, Artem Shinkarov
wrote:
> Hi, I had the problem with passing information about single variable
> from expand_vec_cond_expr optab into ix86_expand_*_vcond.
>
> I looked into it this problem for quite a while and found a solution.
> Now the question if it could be done better.
>
> First of all the problem:
>
> If we represent any vector comparison with VEC_COND_EXPR < v0 v1
> ? {-1,...} : {0,...} >, then in the assembler we do not want to see
> this useless comparison with {-1...}.
>
> Now it is easy to fix the problem about excessive masking. The real
> challenge starts when the comparison inside vcond is expressed as a
> variable. In that case in order to construct correct vector expression
> we need to adjust cond in cond ? v0 : v1 to cond == {-1...} or as we
> agreed recently cond != {0,..}. But hat we need to do only to make
> vec_cond_expr happy. On the level of assembler we don't want this
> condition.
>
> Now, if I just construct the tree, then in x86, rtx_equal_p, does not
> know that this is a constant vector full of -1, because the comparison
> operands are not immediate. So I need somehow to mark the fact in
> optabs, and then check the information in the x86.
Well, this is why I was suggesting the bitwise semantic for a mask
operand. What we should do on the tree level (and that should happen
already), is forward the comparison into the COND_EXPR. Thus,
mask = v1 < v2;
v3 = mask ? v4 : v5;
should get changed to
v3 = v1 < v2 ? v4 : v5;
by tree-ssa-forwprop.c. If that is not happening we have to fix that there.
Because we _don't_ know the mask is all -1 or 0 ;) The user might
put in {3, 5 ,1 3} and expect it to be treated like {-1,...} but it isn't
so already.
> At the moment I do something like this:
>
> optabs:
>
> if (!COMPARISON_CLASS_P (op0))
> ops[3] = gen_rtx_EQ (mode, NULL_RTX, NULL_RTX);
>
> This expression is preserved while checking and verifying.
>
> ix86:
> if (GET_CODE (comp) == EQ && XEXP (comp, 0) == NULL_RTX
> && XEXP (comp, 1) == NULL_RTX)
>
> See the patch attached for more details. The patch is just to give you
> an idea of the way I am doing it and it seems to work. Please don't
> criticise the patch itself, better help me to understand if there is a
> better way to pass the information from optabs to ix86.
Hm, I'm not sure the expand_vec_cond_expr will work that way,
I'd have to play with it myself (but will now be running for weekend).
Is the special-casing of a < b ? {-1,-1,-1} : {0,0,0,0} in the backend
working for you? I think there are probably some rtl all-ones and all-zeros
predicates you can re-use.
Richard.
>
> Thanks,
> Artem.
>
> On Thu, Aug 18, 2011 at 3:31 PM, Richard Henderson wrote:
>> On 08/18/2011 02:23 AM, Richard Guenther wrote:
> >> The first one (inefficient) is vec0 > vec1 ? {-1,...} : {0,...}
> >> The second is vec0 > vec1. expand_vec_cond_expr is stupid, which is
> >> fine, but it means that we need to construct it carefully.
>
> This is still important.
>>> Yes. I think the backends need to handle optimizing this case,
>>> esp. considering targets that do not have instructions to produce
>>> a {-1,...}/{0,...} bitmask from a comparison but produce a vector
>>> of condition codes. With using vec0 > vec1 ? {-1...} : {0,...} for
>>> mask = vec0 > vec1; we avoid exposing the result kind of
>>> vector comparisons.
>>>
>>> It should be easily possible for x86 for example to recognize
>>> the -1 : 0 case.
>>>
>>
>> I think you've been glossing over the hard part with "..." up there.
>> I challenge you to actually fill that in with something meaningful
>> in rtl.
>>
>> I suspect that you simply have to add another named pattern that
>> will Do What You Want on mips and suchlike that produce a CCmode.
>>
>>
>>
>> r~
>>
>
Re: Vector Comparison patch
Hi, I had the problem with passing information about single variable
from expand_vec_cond_expr optab into ix86_expand_*_vcond.
I looked into it this problem for quite a while and found a solution.
Now the question if it could be done better.
First of all the problem:
If we represent any vector comparison with VEC_COND_EXPR < v0 v1
? {-1,...} : {0,...} >, then in the assembler we do not want to see
this useless comparison with {-1...}.
Now it is easy to fix the problem about excessive masking. The real
challenge starts when the comparison inside vcond is expressed as a
variable. In that case in order to construct correct vector expression
we need to adjust cond in cond ? v0 : v1 to cond == {-1...} or as we
agreed recently cond != {0,..}. But hat we need to do only to make
vec_cond_expr happy. On the level of assembler we don't want this
condition.
Now, if I just construct the tree, then in x86, rtx_equal_p, does not
know that this is a constant vector full of -1, because the comparison
operands are not immediate. So I need somehow to mark the fact in
optabs, and then check the information in the x86.
At the moment I do something like this:
optabs:
if (!COMPARISON_CLASS_P (op0))
ops[3] = gen_rtx_EQ (mode, NULL_RTX, NULL_RTX);
This expression is preserved while checking and verifying.
ix86:
if (GET_CODE (comp) == EQ && XEXP (comp, 0) == NULL_RTX
&& XEXP (comp, 1) == NULL_RTX)
See the patch attached for more details. The patch is just to give you
an idea of the way I am doing it and it seems to work. Please don't
criticise the patch itself, better help me to understand if there is a
better way to pass the information from optabs to ix86.
Thanks,
Artem.
On Thu, Aug 18, 2011 at 3:31 PM, Richard Henderson wrote:
> On 08/18/2011 02:23 AM, Richard Guenther wrote:
>> The first one (inefficient) is vec0 > vec1 ? {-1,...} : {0,...}
>> The second is vec0 > vec1. expand_vec_cond_expr is stupid, which is
>> fine, but it means that we need to construct it carefully.
>>> >
>>> > This is still important.
>> Yes. I think the backends need to handle optimizing this case,
>> esp. considering targets that do not have instructions to produce
>> a {-1,...}/{0,...} bitmask from a comparison but produce a vector
>> of condition codes. With using vec0 > vec1 ? {-1...} : {0,...} for
>> mask = vec0 > vec1; we avoid exposing the result kind of
>> vector comparisons.
>>
>> It should be easily possible for x86 for example to recognize
>> the -1 : 0 case.
>>
>
> I think you've been glossing over the hard part with "..." up there.
> I challenge you to actually fill that in with something meaningful
> in rtl.
>
> I suspect that you simply have to add another named pattern that
> will Do What You Want on mips and suchlike that produce a CCmode.
>
>
>
> r~
>
Index: gcc/optabs.c
===
--- gcc/optabs.c(revision 177665)
+++ gcc/optabs.c(working copy)
@@ -6557,6 +6557,8 @@ expand_vec_cond_expr_p (tree type, enum
/* Generate insns for a VEC_COND_EXPR, given its TYPE and its
three operands. */
+rtx rtx_build_vector_from_val (enum machine_mode, HOST_WIDE_INT);
+rtx gen_const_vector1 (enum machine_mode, int);
rtx
expand_vec_cond_expr (tree vec_cond_type, tree op0, tree op1, tree op2,
@@ -6572,16 +6574,39 @@ expand_vec_cond_expr (tree vec_cond_type
if (icode == CODE_FOR_nothing)
return 0;
- comparison = vector_compare_rtx (op0, unsignedp, icode);
rtx_op1 = expand_normal (op1);
rtx_op2 = expand_normal (op2);
+
+ if (COMPARISON_CLASS_P (op0))
+{
+ comparison = vector_compare_rtx (op0, unsignedp, icode);
+ create_output_operand (&ops[0], target, mode);
+ create_input_operand (&ops[1], rtx_op1, mode);
+ create_input_operand (&ops[2], rtx_op2, mode);
+ create_fixed_operand (&ops[3], comparison);
+ create_fixed_operand (&ops[4], XEXP (comparison, 0));
+ create_fixed_operand (&ops[5], XEXP (comparison, 1));
+
+}
+ else
+{
+ enum rtx_code rcode;
+ rtx rtx_op0;
+ rtx vec;
+
+ rtx_op0 = expand_normal (op0);
+ rcode = get_rtx_code (EQ_EXPR, unsignedp);
+ comparison = gen_rtx_EQ (mode, NULL_RTX, NULL_RTX);
+ vec = rtx_build_vector_from_val (mode, -1);
+
+ create_output_operand (&ops[0], target, mode);
+ create_input_operand (&ops[1], rtx_op1, mode);
+ create_input_operand (&ops[2], rtx_op2, mode);
+ create_input_operand (&ops[3], comparison, mode);
+ create_input_operand (&ops[4], rtx_op0, mode);
+ create_input_operand (&ops[5], vec, mode);
+}
- create_output_operand (&ops[0], target, mode);
- create_input_operand (&ops[1], rtx_op1, mode);
- create_input_operand (&ops[2], rtx_op2, mode);
- create_fixed_operand (&ops[3], comparison);
- create_fixed_operand (&ops[4], XEXP (comparison, 0));
- create_fixed_operand (&ops[5], XEXP (comparison, 1));
expand_insn (icode, 6, ops);
return o
Re: Vector Comparison patch
On 08/18/2011 02:23 AM, Richard Guenther wrote:
>>> >> The first one (inefficient) is vec0 > vec1 ? {-1,...} : {0,...}
>>> >> The second is vec0 > vec1. expand_vec_cond_expr is stupid, which is
>>> >> fine, but it means that we need to construct it carefully.
>> >
>> > This is still important.
> Yes. I think the backends need to handle optimizing this case,
> esp. considering targets that do not have instructions to produce
> a {-1,...}/{0,...} bitmask from a comparison but produce a vector
> of condition codes. With using vec0 > vec1 ? {-1...} : {0,...} for
> mask = vec0 > vec1; we avoid exposing the result kind of
> vector comparisons.
>
> It should be easily possible for x86 for example to recognize
> the -1 : 0 case.
>
I think you've been glossing over the hard part with "..." up there.
I challenge you to actually fill that in with something meaningful
in rtl.
I suspect that you simply have to add another named pattern that
will Do What You Want on mips and suchlike that produce a CCmode.
r~
Re: Vector Comparison patch
Richard, I am trying to make sure that when vcond has {-1} and {0} it
does not trigger masking. Currently I am doing this:
Index: config/i386/i386.c
===
--- config/i386/i386.c (revision 177665)
+++ config/i386/i386.c (working copy)
@@ -25,6 +25,7 @@ along with GCC; see the file COPYING3.
#include "tm.h"
#include "rtl.h"
#include "tree.h"
+#include "tree-flow.h"
#include "tm_p.h"
#include "regs.h"
#include "hard-reg-set.h"
@@ -18434,7 +18435,30 @@ ix86_expand_sse_movcc (rtx dest, rtx cmp
{
enum machine_mode mode = GET_MODE (dest);
rtx t2, t3, x;
-
+ rtx mask_true;
+
+ rtvec v;
+ int units, i;
+ enum machine_mode inner;
+
+ units = GET_MODE_NUNITS (mode);
+ inner = GET_MODE_INNER (mode);
+ v = rtvec_alloc (units);
+ for (i = 0; i < units; ++i)
+RTVEC_ELT (v, i) = gen_rtx_CONST_INT (inner, -1);
+
+ mask_true = gen_rtx_raw_CONST_VECTOR (mode, v);
+
+ fprintf (stderr, "I am here\n");
+ debug_rtx (mask_true);
+ debug_rtx (op_true);
+ if (rtx_equal_p (op_true, mask_true))
+{
+ fprintf (stderr, "Yes it is\n");
+ emit_insn (gen_rtx_SET (VOIDmode, dest, cmp));
+ return;
+}
+ else
if (op_false == CONST0_RTX (mode))
{
op_true = force_reg (mode, op_true);
It works out the case when mask is -1 very well, however in the code
generated by the expansion I still see excessive operations:
ires = i0 < i1 ? (vector (4, int)){-1,-1,-1,-1} : (vector (4, int)){0,0,0,0};
expands to:
pcmpgtd %xmm1, %xmm0
pcmpeqd %xmm1, %xmm1
pcmpeqd %xmm1, %xmm0
movdqa %xmm0, -24(%rsp)
Where the code
ires = i0 < i1;
using my hook expands to:
pcmpgtd %xmm1, %xmm0
movdqa %xmm0, -24(%rsp)
So someone is putting two extra instructions there, and I cannot
really figure out who is doing that. Anyone knows how could I fix
this...
Artem.
Re: Vector Comparison patch
On Thu, 18 Aug 2011, Artem Shinkarov wrote:
> >> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
> >> + sc = c_fully_fold (ifexp, false, &maybe_const);
> >> + sc = save_expr (sc);
> >> + if (!maybe_const)
> >> + ifexp = c_wrap_maybe_const (sc, true);
> >> + else
> >> + ifexp = sc;
> >
> > This looks like it's duplicating c_save_expr; that is, like "ifexp =
> > c_save_expr (ifexp);" would suffice.
> >
> > But, it's not clear that it actually achieves the effect described in the
> > comment; have you actually tried with function calls, assignments etc. in
> > the operands?
>
> I tested it with gcc.dg/vector-compare-2.c:
> typedef int vec __attribute__((vector_size(16)));
>
> vec i,j;
> extern vec a, b, c;
>
> vec
> foo (int x)
> {
> return (x ? i : j) ? a : b;
> }
>
> vec
> bar (int x)
> {
> return a ? (x ? i : j) : b;
> }
>
> vec
> baz (int x)
> {
> return a ? b : (x ? i : j);
> }
>
> Is it good enough?
No, because none of the operands there involve assignment, increment,
decrement, function call or comma operators (which are the main cases that
would trigger the creation of C_MAYBE_CONST_EXPR).
> > The code in build_binary_op uses save_expr rather than
> > c_save_expr because it does some intermediate operations before calling
> > c_wrap_maybe_const, and if you really want to avoid C_MAYBE_CONST in
> > VEC_COND_EXPR then you'll need to continue calling save_expr, as here, but
> > delay the call to c_wrap_maybe_const so that the whole VEC_COND_EXPR is
> > wrapped if required.
>
> Ok, but I need to wrap it at some point, where do you think it would
> be appropriate to do?
After the creation of the VEC_COND_EXPR. I.e. don't just return the
results of build3 or convert, wrap them as needed before returning them.
--
Joseph S. Myers
jos...@codesourcery.com
Re: Vector Comparison patch
On Wed, Aug 17, 2011 at 10:52 PM, Joseph S. Myers
wrote:
> On Wed, 17 Aug 2011, Artem Shinkarov wrote:
>
>> +For the convenience condition in the vector conditional can be just a
>> +vector of signed integer type. In that case this vector is implicitly
>> +compared with vectors of zeroes. Consider an example:
>
> Where is this bit tested in the testcases added?
In the gcc.c-torture/execute/vector-vcond-2.c at the end of test-case:
/* Condition expressed with a single variable. */
dres = l0 ? d0 : d1;
check_compare (2, dres, l0, ((vector (2, long)){-1,-1}), d0, d1, ==,
"%f", "%i");
lres = l1 ? l0 : l1;
check_compare (2, lres, l1, ((vector (2, long)){-1,-1}), l0, l1, ==,
"%i", "%i");
fres = i0 ? f0 : f1;
check_compare (4, fres, i0, ((vector (4, int)){-1,-1,-1,-1}),
f0, f1, ==, "%f", "%i");
ires = i1 ? i0 : i1;
check_compare (4, ires, i1, ((vector (4, int)){-1,-1,-1,-1}),
i0, i1, ==, "%i", "%i");
>
>> + if (TREE_CODE (type1) != VECTOR_TYPE
>> + || TREE_CODE (type2) != VECTOR_TYPE)
>> + {
>> + error_at (colon_loc, "vector comparisom arguments must be of "
>> + "type vector");
>
> "comparison"
Thanks, adjusted.
>> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
>> + sc = c_fully_fold (ifexp, false, &maybe_const);
>> + sc = save_expr (sc);
>> + if (!maybe_const)
>> + ifexp = c_wrap_maybe_const (sc, true);
>> + else
>> + ifexp = sc;
>
> This looks like it's duplicating c_save_expr; that is, like "ifexp =
> c_save_expr (ifexp);" would suffice.
>
> But, it's not clear that it actually achieves the effect described in the
> comment; have you actually tried with function calls, assignments etc. in
> the operands?
I tested it with gcc.dg/vector-compare-2.c:
typedef int vec __attribute__((vector_size(16)));
vec i,j;
extern vec a, b, c;
vec
foo (int x)
{
return (x ? i : j) ? a : b;
}
vec
bar (int x)
{
return a ? (x ? i : j) : b;
}
vec
baz (int x)
{
return a ? b : (x ? i : j);
}
Is it good enough?
> The code in build_binary_op uses save_expr rather than
> c_save_expr because it does some intermediate operations before calling
> c_wrap_maybe_const, and if you really want to avoid C_MAYBE_CONST in
> VEC_COND_EXPR then you'll need to continue calling save_expr, as here, but
> delay the call to c_wrap_maybe_const so that the whole VEC_COND_EXPR is
> wrapped if required.
Ok, but I need to wrap it at some point, where do you think it would
be appropriate to do?
Thanks,
Artem.
Re: Vector Comparison patch
> Yes. I think the backends need to handle optimizing this case,
> esp. considering targets that do not have instructions to produce
> a {-1,...}/{0,...} bitmask from a comparison but produce a vector
> of condition codes. With using vec0 > vec1 ? {-1...} : {0,...} for
> mask = vec0 > vec1; we avoid exposing the result kind of
> vector comparisons.
>
> It should be easily possible for x86 for example to recognize
> the -1 : 0 case.
Ok, I am fine with this approach. Ho could we check if vector
comparison returns {-1..}/{0..} or something else. If I can check
that, I could adjust expand_vec_cond_exrp, and get rid of the hook.
> Yes, and I think the fix is in the backends. I still think we have to
> sort out the best building blocks we want the targets to expose.
> Currently we only have the vectorizer vcond patterns which should
> be enough to get the C language support implemented. After that
> we should concentrate on generating efficient code for all variants.
Ok, see my above comment.
> Yeah, well. That's really a question for language lawyers ;) I agree
> that it would be nice to have mask ? val0 : val1 behave "the same"
> for scalars and vectors. The question is whether for vectors you
> define it on the bit-level (which makes it equal to (mask & val0) |
> (~mask & val1))
> or on the vector component level. The vector component level
> is probably what people would expect.
>
> Which means we have to treat mask ? val0 : val1 as
> mask != {0,...} ? val0 : val1.
> I'd use != {0,0,...} as eventually a zero vector is cheaper to construct
> and it supports the scalar ?: semantics - whenever the mask element
> is non-zero it's true.
Ok, I am fine with x != {0,...}, I can adjust it in both cases.
Artem.
Re: Vector Comparison patch
On Wed, Aug 17, 2011 at 8:51 PM, Artem Shinkarov
wrote:
> On Wed, Aug 17, 2011 at 4:28 PM, Artem Shinkarov
> wrote:
>> On Wed, Aug 17, 2011 at 3:58 PM, Richard Guenther
>> wrote:
>>> On Wed, Aug 17, 2011 at 3:30 PM, Artem Shinkarov
>>> wrote:
Hi
Several comments before the new version of the patch.
1) x != x
I am happy to adjust constant_boolean_node, but look at the code
around line 9074 in fold-const.c, you will see that x x
elimination, even with adjusted constant_boolean_node, will look about
the same as my code. Because I need to check the parameters (!FLOAT_P,
HONOR_NANS) on TREE_TYPE (arg0) not arg0, and I need to construct
constant_boolean_node (-1), not 1 in case of true.
But I will change constant_boolean_node to accept vector types.
>>>
>>> Hm, that should be handled transparently if you look at the defines
>>> of FLOAT_TYPE_P and the HONOR_* macros.
>>>
>>
>> Ok, Currently I have this, what do you think:
>> int true_val = TREE_CODE (type) == VECTOR_TYPE ? -1 : 0;
>> tree arg0_type = TREE_CODE (type) == VECTOR_TYPE
>> ? TREE_TYPE (TREE_TYPE (arg0)) : TREE_TYPE (arg0);
>> switch (code)
>> {
>> case EQ_EXPR:
>> if (! FLOAT_TYPE_P (arg0_type)
>> || ! HONOR_NANS (TYPE_MODE (arg0_type)))
>> return constant_boolean_node (true_val, type);
>> break;
>>
>> case GE_EXPR:
>> case LE_EXPR:
>> if (! FLOAT_TYPE_P (arg0_type)
>> || ! HONOR_NANS (TYPE_MODE (arg0_type)))
>> return constant_boolean_node (true_val, type);
>> return fold_build2_loc (loc, EQ_EXPR, type, arg0, arg1);
>>
>> case NE_EXPR:
>> /* For NE, we can only do this simplification if integer
>> or we don't honor IEEE floating point NaNs. */
>> if (FLOAT_TYPE_P (arg0_type)
>> && HONOR_NANS (TYPE_MODE (arg0_type)))
>> break;
>> /* ... fall through ... */
>> case GT_EXPR:
>> case LT_EXPR:
>> return constant_boolean_node (0, type);
>> default:
>> gcc_unreachable ();
>> }
>>
>> Works fine for both vector and scalar cases.
>
> Please ignore this comment.
>
>>
2) comparison vs vcond
v = v1 < v2;
v = v1 < v2 ? {-1,...} : {0,...};
are not the same.
16,25c16,22
< movdqa .LC1(%rip), %xmm1
< pshufd $225, %xmm1, %xmm1
< pshufd $39, %xmm0, %xmm0
< movss %xmm2, %xmm1
< pshufd $225, %xmm1, %xmm1
< pcmpgtd %xmm1, %xmm0
< pcmpeqd %xmm1, %xmm1
< pcmpeqd %xmm1, %xmm0
< pand %xmm1, %xmm0
< movdqa %xmm0, -24(%rsp)
---
> pshufd $39, %xmm0, %xmm1
> movdqa .LC1(%rip), %xmm0
> pshufd $225, %xmm0, %xmm0
> movss %xmm2, %xmm0
> pshufd $225, %xmm0, %xmm0
> pcmpgtd %xmm0, %xmm1
> movdqa %xmm1, -24(%rsp)
So I would keep the hook, it could be removed at any time when the
standard expansion will start to work fine.
>>>
>>> Which one is which?
>>
>> You must be joking. :)
:)
>> The first one (inefficient) is vec0 > vec1 ? {-1,...} : {0,...}
>> The second is vec0 > vec1. expand_vec_cond_expr is stupid, which is
>> fine, but it means that we need to construct it carefully.
>
> This is still important.
Yes. I think the backends need to handle optimizing this case,
esp. considering targets that do not have instructions to produce
a {-1,...}/{0,...} bitmask from a comparison but produce a vector
of condition codes. With using vec0 > vec1 ? {-1...} : {0,...} for
mask = vec0 > vec1; we avoid exposing the result kind of
vector comparisons.
It should be easily possible for x86 for example to recognize
the -1 : 0 case.
>>> I'd really like to make this patch simpler at first,
>>> and removing that hook is an obvious thing that _should_ be possible,
>>> even optimally (by fixing the targets).
>>
>> Ok, let's remove the hook, then could you provide some more
>> information rather than we just need to do it?
>>
>> Simple in this case means inefficient -- I would hope to make it
>> efficient as well.
>
> This is very important.
Yes, and I think the fix is in the backends. I still think we have to
sort out the best building blocks we want the targets to expose.
Currently we only have the vectorizer vcond patterns which should
be enough to get the C language support implemented. After that
we should concentrate on generating efficient code for all variants.
3) mask ? vec0 : vec1
So no, I don't think we need to convert {3, 4, -1, 5} to {0,0,-1,0}
(that would surprise my anyway, I'd have expected {-1,-1,-1,-1} ;)).
Does OpenCL somehow support you here?
OpenCL says that vector operation mask ? vec0 : vec1 is the same as
select (ve
Re: Vector Comparison patch
On Wed, 17 Aug 2011, Artem Shinkarov wrote:
> +For the convenience condition in the vector conditional can be just a
> +vector of signed integer type. In that case this vector is implicitly
> +compared with vectors of zeroes. Consider an example:
Where is this bit tested in the testcases added?
> + if (TREE_CODE (type1) != VECTOR_TYPE
> + || TREE_CODE (type2) != VECTOR_TYPE)
> +{
> + error_at (colon_loc, "vector comparisom arguments must be of "
> + "type vector");
"comparison"
> + /* Avoid C_MAYBE_CONST in VEC_COND_EXPR. */
> + sc = c_fully_fold (ifexp, false, &maybe_const);
> + sc = save_expr (sc);
> + if (!maybe_const)
> + ifexp = c_wrap_maybe_const (sc, true);
> + else
> + ifexp = sc;
This looks like it's duplicating c_save_expr; that is, like "ifexp =
c_save_expr (ifexp);" would suffice.
But, it's not clear that it actually achieves the effect described in the
comment; have you actually tried with function calls, assignments etc. in
the operands? The code in build_binary_op uses save_expr rather than
c_save_expr because it does some intermediate operations before calling
c_wrap_maybe_const, and if you really want to avoid C_MAYBE_CONST in
VEC_COND_EXPR then you'll need to continue calling save_expr, as here, but
delay the call to c_wrap_maybe_const so that the whole VEC_COND_EXPR is
wrapped if required.
--
Joseph S. Myers
jos...@codesourcery.com
Re: Vector Comparison patch
On Wed, Aug 17, 2011 at 3:58 PM, Richard Guenther
wrote:
> On Wed, Aug 17, 2011 at 3:30 PM, Artem Shinkarov
> wrote:
>> Hi
>>
>> Several comments before the new version of the patch.
>> 1) x != x
>> I am happy to adjust constant_boolean_node, but look at the code
>> around line 9074 in fold-const.c, you will see that x x
>> elimination, even with adjusted constant_boolean_node, will look about
>> the same as my code. Because I need to check the parameters (!FLOAT_P,
>> HONOR_NANS) on TREE_TYPE (arg0) not arg0, and I need to construct
>> constant_boolean_node (-1), not 1 in case of true.
>> But I will change constant_boolean_node to accept vector types.
>
> Hm, that should be handled transparently if you look at the defines
> of FLOAT_TYPE_P and the HONOR_* macros.
>
Ok, Currently I have this, what do you think:
int true_val = TREE_CODE (type) == VECTOR_TYPE ? -1 : 0;
tree arg0_type = TREE_CODE (type) == VECTOR_TYPE
? TREE_TYPE (TREE_TYPE (arg0)) : TREE_TYPE (arg0);
switch (code)
{
case EQ_EXPR:
if (! FLOAT_TYPE_P (arg0_type)
|| ! HONOR_NANS (TYPE_MODE (arg0_type)))
return constant_boolean_node (true_val, type);
break;
case GE_EXPR:
case LE_EXPR:
if (! FLOAT_TYPE_P (arg0_type)
|| ! HONOR_NANS (TYPE_MODE (arg0_type)))
return constant_boolean_node (true_val, type);
return fold_build2_loc (loc, EQ_EXPR, type, arg0, arg1);
case NE_EXPR:
/* For NE, we can only do this simplification if integer
or we don't honor IEEE floating point NaNs. */
if (FLOAT_TYPE_P (arg0_type)
&& HONOR_NANS (TYPE_MODE (arg0_type)))
break;
/* ... fall through ... */
case GT_EXPR:
case LT_EXPR:
return constant_boolean_node (0, type);
default:
gcc_unreachable ();
}
Works fine for both vector and scalar cases.
>>
>> 2) comparison vs vcond
>> v = v1 < v2;
>> v = v1 < v2 ? {-1,...} : {0,...};
>>
>> are not the same.
>> 16,25c16,22
>> < movdqa .LC1(%rip), %xmm1
>> < pshufd $225, %xmm1, %xmm1
>> < pshufd $39, %xmm0, %xmm0
>> < movss %xmm2, %xmm1
>> < pshufd $225, %xmm1, %xmm1
>> < pcmpgtd %xmm1, %xmm0
>> < pcmpeqd %xmm1, %xmm1
>> < pcmpeqd %xmm1, %xmm0
>> < pand %xmm1, %xmm0
>> < movdqa %xmm0, -24(%rsp)
>> ---
>>> pshufd $39, %xmm0, %xmm1
>>> movdqa .LC1(%rip), %xmm0
>>> pshufd $225, %xmm0, %xmm0
>>> movss %xmm2, %xmm0
>>> pshufd $225, %xmm0, %xmm0
>>> pcmpgtd %xmm0, %xmm1
>>> movdqa %xmm1, -24(%rsp)
>>
>> So I would keep the hook, it could be removed at any time when the
>> standard expansion will start to work fine.
>
> Which one is which?
You must be joking. :)
The first one (inefficient) is vec0 > vec1 ? {-1,...} : {0,...}
The second is vec0 > vec1. expand_vec_cond_expr is stupid, which is
fine, but it means that we need to construct it carefully.
> I'd really like to make this patch simpler at first,
> and removing that hook is an obvious thing that _should_ be possible,
> even optimally (by fixing the targets).
Ok, let's remove the hook, then could you provide some more
information rather than we just need to do it?
Simple in this case means inefficient -- I would hope to make it
efficient as well.
>> 3) mask ? vec0 : vec1
>> So no, I don't think we need to convert {3, 4, -1, 5} to {0,0,-1,0}
>> (that would surprise my anyway, I'd have expected {-1,-1,-1,-1} ;)).
>>
>> Does OpenCL somehow support you here?
>>
>> OpenCL says that vector operation mask ? vec0 : vec1 is the same as
>> select (vec0, vec1, mask). The semantics of select operation is the
>> following:
>>
>> gentype select (gentype a, gentype b, igentype c)
>> For each component of a vector type,
>> result[i] = if MSB of c[i] is set ? b[i] : a[i].
>>
>> I am not sure what they really understand using the term MSB. As far
>> as I know MSB is Most Significant Bit, so does it mean that in case of
>> 3-bit integer 100 would trigger true but 011 would be still false...
>
> Yes, MSB is Most Significant Bit - that's a somewhat odd definition ;)
>
>> My reading would be that if all bits set, then take the first element,
>> otherwise the second.
>>
>> It is also confusing when a ? vec0 : vec1, and a != 0 ? vec0 vec1
>> produce different results. So I would stick to all bits set being true
>> scenario.
>
> For the middle-end part definitely. Thus I'd simply leave the mask alone.
>
Well, it seems very unnatural to me. In the case of scalars mask ?
val0 : val1 would not work the same way as (mask & val0) | (~mask &
val1), why should we have the same behaviour for the vector stuff?
>> 4) Backend stuff. Ok, we could always fall back to reject the cases
>> when cond and operands have different type, and then fix the
Re: Vector Comparison patch
On Wed, Aug 17, 2011 at 3:30 PM, Artem Shinkarov
wrote:
> Hi
>
> Several comments before the new version of the patch.
> 1) x != x
> I am happy to adjust constant_boolean_node, but look at the code
> around line 9074 in fold-const.c, you will see that x x
> elimination, even with adjusted constant_boolean_node, will look about
> the same as my code. Because I need to check the parameters (!FLOAT_P,
> HONOR_NANS) on TREE_TYPE (arg0) not arg0, and I need to construct
> constant_boolean_node (-1), not 1 in case of true.
> But I will change constant_boolean_node to accept vector types.
Hm, that should be handled transparently if you look at the defines
of FLOAT_TYPE_P and the HONOR_* macros.
>
> 2) comparison vs vcond
> v = v1 < v2;
> v = v1 < v2 ? {-1,...} : {0,...};
>
> are not the same.
> 16,25c16,22
> < movdqa .LC1(%rip), %xmm1
> < pshufd $225, %xmm1, %xmm1
> < pshufd $39, %xmm0, %xmm0
> < movss %xmm2, %xmm1
> < pshufd $225, %xmm1, %xmm1
> < pcmpgtd %xmm1, %xmm0
> < pcmpeqd %xmm1, %xmm1
> < pcmpeqd %xmm1, %xmm0
> < pand %xmm1, %xmm0
> < movdqa %xmm0, -24(%rsp)
> ---
>> pshufd $39, %xmm0, %xmm1
>> movdqa .LC1(%rip), %xmm0
>> pshufd $225, %xmm0, %xmm0
>> movss %xmm2, %xmm0
>> pshufd $225, %xmm0, %xmm0
>> pcmpgtd %xmm0, %xmm1
>> movdqa %xmm1, -24(%rsp)
>
> So I would keep the hook, it could be removed at any time when the
> standard expansion will start to work fine.
Which one is which? I'd really like to make this patch simpler at first,
and removing that hook is an obvious thing that _should_ be possible,
even optimally (by fixing the targets).
> 3) mask ? vec0 : vec1
> So no, I don't think we need to convert {3, 4, -1, 5} to {0,0,-1,0}
> (that would surprise my anyway, I'd have expected {-1,-1,-1,-1} ;)).
>
> Does OpenCL somehow support you here?
>
> OpenCL says that vector operation mask ? vec0 : vec1 is the same as
> select (vec0, vec1, mask). The semantics of select operation is the
> following:
>
> gentype select (gentype a, gentype b, igentype c)
> For each component of a vector type,
> result[i] = if MSB of c[i] is set ? b[i] : a[i].
>
> I am not sure what they really understand using the term MSB. As far
> as I know MSB is Most Significant Bit, so does it mean that in case of
> 3-bit integer 100 would trigger true but 011 would be still false...
Yes, MSB is Most Significant Bit - that's a somewhat odd definition ;)
> My reading would be that if all bits set, then take the first element,
> otherwise the second.
>
> It is also confusing when a ? vec0 : vec1, and a != 0 ? vec0 vec1
> produce different results. So I would stick to all bits set being true
> scenario.
For the middle-end part definitely. Thus I'd simply leave the mask alone.
> 4) Backend stuff. Ok, we could always fall back to reject the cases
> when cond and operands have different type, and then fix the backend.
>
> Adjustments are coming.
>
>
> Artem.
>
Re: Vector Comparison patch
Hi
Several comments before the new version of the patch.
1) x != x
I am happy to adjust constant_boolean_node, but look at the code
around line 9074 in fold-const.c, you will see that x x
elimination, even with adjusted constant_boolean_node, will look about
the same as my code. Because I need to check the parameters (!FLOAT_P,
HONOR_NANS) on TREE_TYPE (arg0) not arg0, and I need to construct
constant_boolean_node (-1), not 1 in case of true.
But I will change constant_boolean_node to accept vector types.
2) comparison vs vcond
v = v1 < v2;
v = v1 < v2 ? {-1,...} : {0,...};
are not the same.
16,25c16,22
< movdqa .LC1(%rip), %xmm1
< pshufd $225, %xmm1, %xmm1
< pshufd $39, %xmm0, %xmm0
< movss %xmm2, %xmm1
< pshufd $225, %xmm1, %xmm1
< pcmpgtd %xmm1, %xmm0
< pcmpeqd %xmm1, %xmm1
< pcmpeqd %xmm1, %xmm0
< pand%xmm1, %xmm0
< movdqa %xmm0, -24(%rsp)
---
> pshufd $39, %xmm0, %xmm1
> movdqa .LC1(%rip), %xmm0
> pshufd $225, %xmm0, %xmm0
> movss %xmm2, %xmm0
> pshufd $225, %xmm0, %xmm0
> pcmpgtd %xmm0, %xmm1
> movdqa %xmm1, -24(%rsp)
So I would keep the hook, it could be removed at any time when the
standard expansion will start to work fine.
3) mask ? vec0 : vec1
So no, I don't think we need to convert {3, 4, -1, 5} to {0,0,-1,0}
(that would surprise my anyway, I'd have expected {-1,-1,-1,-1} ;)).
Does OpenCL somehow support you here?
OpenCL says that vector operation mask ? vec0 : vec1 is the same as
select (vec0, vec1, mask). The semantics of select operation is the
following:
gentype select (gentype a, gentype b, igentype c)
For each component of a vector type,
result[i] = if MSB of c[i] is set ? b[i] : a[i].
I am not sure what they really understand using the term MSB. As far
as I know MSB is Most Significant Bit, so does it mean that in case of
3-bit integer 100 would trigger true but 011 would be still false...
My reading would be that if all bits set, then take the first element,
otherwise the second.
It is also confusing when a ? vec0 : vec1, and a != 0 ? vec0 vec1
produce different results. So I would stick to all bits set being true
scenario.
4) Backend stuff. Ok, we could always fall back to reject the cases
when cond and operands have different type, and then fix the backend.
Adjustments are coming.
Artem.
Re: Vector Comparison patch
On Tue, Aug 16, 2011 at 11:12 PM, Artem Shinkarov
wrote:
> Hi, here is a new version of the patch with the adjustments.
>
> Two important comments.
> 1) At the moment when I expand expression mask ? vec0 : vec1, I
> replace mask with (mask == {-1,-1,..}). The first reason is that
> expand_vec_cond_expr requires first operand to be a comparison. Second
> reason is that a mask {3, 4, -1, 5} should be transformed into
> {0,0,-1,0} in order to simulate vcond as ((vec0 & mask) | (vec1 &
> ~mask)). So in both cases we need this adjustment.
Well. From a middle-end view I'd say that mask ? vec0 : vec1
should return (vec0 & mask) | (vec1 & ~mask) which is what
the XOP vcond instructions do, btw. Only by defining
v1 < v2 to return a mask constrained to {-1|0, -1|0, ...} the
combination v1 < v2 ? vec0 : vec1 gets it's vector element
selection semantic (instead of being just a bitwise selection,
which it really is).
So no, I don't think we need to convert {3, 4, -1, 5} to {0,0,-1,0}
(that would surprise my anyway, I'd have expected {-1,-1,-1,-1} ;)).
Does OpenCL somehow support you here?
> 2) Vector comparison through optab.
> As far as I just have adjusted expand_vector_operation in
> tree-vect-generic.c, it would be called only when there is no
> sufficient optab. I is being checked in expand_vector_operations_1. So
> the only place where I try to find an optab for the comparison is
> expand_vec_cond_expr_piecewise, which I adjusted.
>
> As for the vector hook, it will be triggered only when we don't have
> an appropriate optab.
>
> bootstrapped and tested on x86_64-unknown-linux-gnu.
> Anything else?
I didn't yet look at the updated patch, I'll wait for another update that
eventually follows my comments to your earlier mail.
Richard.
>
> Artem.
>
Re: Vector Comparison patch
On Tue, Aug 16, 2011 at 6:35 PM, Artem Shinkarov
wrote:
> On Tue, Aug 16, 2011 at 4:28 PM, Richard Guenther
> wrote:
Index: gcc/fold-const.c
===
--- gcc/fold-const.c (revision 177665)
+++ gcc/fold-const.c (working copy)
@@ -9073,34 +9073,61 @@ fold_comparison (location_t loc, enum tr
floating-point, we can only do some of these simplifications.) */
if (operand_equal_p (arg0, arg1, 0))
{
- switch (code)
+ if (TREE_CODE (TREE_TYPE (arg0)) == VECTOR_TYPE)
{
- case EQ_EXPR:
- if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
- || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
- return constant_boolean_node (1, type);
I think this change should go in a separate patch for improved
constant folding. It shouldn't be necessary for enabling vector compares,
no?
>>>
>>> Unfortunately no, this case must be covered here, otherwise x != x
>>> condition fails.
>>
>> How does it fail?
>
> When I have x > x, x == x, and so on, fold-const.c trigger
> operand_equal_p (arg0, arg1, 0), which returns true, and then it calls
> constant_boolean_node (, type). But the problem is that the
> result of the comparison is a vector, not a boolean. So we have an
> assertion failure:
> test.c: In function ‘foo’:
> test.c:9:3: internal compiler error: in build_int_cst_wide, at tree.c:1222
> Please submit a full bug report,
> with preprocessed source if appropriate.
Ok, so we have to either avoid folding it (which would be a shame), or
define how true / false look like for vector typed comparison results.
The documentation above the tree code defintions for comparisons in
tree.def needs updating then, with something like
and the value is either the type used by the language for booleans
or an integer vector type of the same size and with the same number
of elements as the comparison operands. True for a vector of
comparison results has all bits set while false is equal to zero.
or some better wording.
Then changing constant_boolean_node to return a proper true/false
vector would be the fix for your problem.
+ /* Currently the expansion of VEC_COND_EXPR does not allow
+ expessions where the type of vectors you compare differs
+ form the type of vectors you select from. For the time
+ being we insert implicit conversions. */
+ if ((COMPARISON_CLASS_P (ifexp)
Why only for comparison-class?
>>> Not only, there is || involved:
>>> (COMPARISON_CLASS_P (ifexp) && TREE_TYPE (TREE_OPERAND (ifexp, 0)) !=
>>> type1)
>>> || TREE_TYPE (ifexp) != type1
>>>
>>> So if this is a comparison class, we check the first operand, because
>>> the result of the comparison fits, however the operands could not. In
>>> case we have an expression of signed vector, we know that we would
>>> transform it into exp != {0,0,...} in tree-vect-generic.c, but if the
>>> types of operands do not match we convert them.
>>
>> Hm, ok ... let's hope we can sort-out the backend issues before this
>> patch goes in so we can remove this converting stuff.
>
> Hm, I would hope that we could commit this patch even with this issue,
> because my feeling is that this case would produce errors on all the
> other architectures as well, as VEC_COND_EXPR is the feature heavily
> used in auto-vectorizer. So it means that all the backends must be
> fixed. And another argument, that this conversion is harmless.
It shouldn't be hard to fix all the backends. And if we don't do it now
it will never happen. I would expect that the codegen part of the
backends doesn't need any adjustments, just the patterns that
match what is supported.
Uros, can you convert x86 as an example? Thus, for
(define_expand "vcond"
[(set (match_operand:VF 0 "register_operand" "")
(if_then_else:VF
(match_operator 3 ""
[(match_operand:VF 4 "nonimmediate_operand" "")
(match_operand:VF 5 "nonimmediate_operand" "")])
(match_operand:VF 1 "general_operand" "")
(match_operand:VF 2 "general_operand" "")))]
"TARGET_SSE"
{
bool ok = ix86_expand_fp_vcond (operands);
gcc_assert (ok);
allow any vector mode of the same size (and same number of elements?)
for the vcond mode and operand 1 and 2? Thus, only restrict the
embedded comparison to VF?
> So I really hope that someone could shed some light or help me with
> this issue, but even if not I think that the current conversion is ok.
> However, I don't have any architectures different from x86.
[...]
+ /* Expand VEC_COND_EXPR into a vector of scalar COND_EXPRs. */
+ v = VEC_alloc(constructor_elt, gc, nunits);
+ for (i = 0; i < nunits;
+ i += 1, index = int_const_binop (PLUS_EXPR, index, part_width))
+ {
+ tree tcond = tree_vec_extract (gsi, inner_type, var, p
Re: Vector Comparison patch
On Tue, Aug 16, 2011 at 4:28 PM, Richard Guenther
wrote:
> On Mon, Aug 15, 2011 at 6:58 PM, Artem Shinkarov
> wrote:
>> On Mon, Aug 15, 2011 at 3:24 PM, Richard Guenther
>> wrote:
>>> On Fri, Aug 12, 2011 at 4:03 AM, Artem Shinkarov
>>> wrote:
Hi
Here is a completed version of the vector comparison patch we
discussed a long time ago here:
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01184.html
The patch implements vector comparison according to the OpenCL
standard, when the result of the comparison of two vectors is vector
of signed integers, where -1 represents true and 0 false.
The patch implements vector conditional res = VCOND
which is expanded into:
foreach (i in length (V1)) res[i] = V1 == 0 ? V3[i] : V2[i].
>>>
>>> Some comments on the patch below. First, in general I don't see
>>> why you need a new target hook to specify whether to "vectorize"
>>> a comparison. Why are the existing hooks used by the vectorizer
>>> not enough?
>>>
>>> Index: gcc/fold-const.c
>>> ===
>>> --- gcc/fold-const.c (revision 177665)
>>> +++ gcc/fold-const.c (working copy)
>>> @@ -9073,34 +9073,61 @@ fold_comparison (location_t loc, enum tr
>>> floating-point, we can only do some of these simplifications.) */
>>> if (operand_equal_p (arg0, arg1, 0))
>>> {
>>> - switch (code)
>>> + if (TREE_CODE (TREE_TYPE (arg0)) == VECTOR_TYPE)
>>> {
>>> - case EQ_EXPR:
>>> - if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
>>> - || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
>>> - return constant_boolean_node (1, type);
>>>
>>> I think this change should go in a separate patch for improved
>>> constant folding. It shouldn't be necessary for enabling vector compares,
>>> no?
>>
>> Unfortunately no, this case must be covered here, otherwise x != x
>> condition fails.
>
> How does it fail?
When I have x > x, x == x, and so on, fold-const.c trigger
operand_equal_p (arg0, arg1, 0), which returns true, and then it calls
constant_boolean_node (, type). But the problem is that the
result of the comparison is a vector, not a boolean. So we have an
assertion failure:
test.c: In function ‘foo’:
test.c:9:3: internal compiler error: in build_int_cst_wide, at tree.c:1222
Please submit a full bug report,
with preprocessed source if appropriate.
>
>>>
>>> + if (TYPE_UNSIGNED (TREE_TYPE (TREE_TYPE (ifexp
>>> + {
>>> + error_at (colon_loc, "vector comparison must be of signed "
>>> + "integer vector type");
>>> + return error_mark_node;
>>> + }
>>>
>>> why that?
>>
>> Well, later on I rely on this fact. I mean OpenCL says that it should
>> return -1 in the sense that all bits set. I don't really know, I can
>> support unsigned masks as well, but wouldn't it just introduce a
>> source for possible errors. I mean that natural choice for true and
>> flase is 0 and 1, not 0 and -1. Anyway I don't have a strong opinion
>> there, and I could easily adjust it if we decide that we want it.
>
> I think we want to allow both signed and unsigned masks.
Ok, I'll adjust.
>
>>>
>>> + if (TYPE_VECTOR_SUBPARTS (type1) != TYPE_VECTOR_SUBPARTS (type2)
>>> + || TYPE_VECTOR_SUBPARTS (TREE_TYPE (ifexp))
>>> + != TYPE_VECTOR_SUBPARTS (type1))
>>> + {
>>> + error_at (colon_loc, "vectors of different length found in "
>>> + "vector comparison");
>>> + return error_mark_node;
>>> + }
>>>
>>> I miss verification that type1 and type2 are vector types, or is that done
>>> elsewhere? I think type1 and type2 are already verified to be
>>> compatible (but you might double-check). At least the above would be
>>> redundant with
>>
>> Thanks, type1 and type2 both vectors comparison is missing, going to
>> be added in the new version of the patch.
>>>
>>> + if (type1 != type2)
>>> + {
>>> + error_at (colon_loc, "vectors of different types involved in "
>>> + "vector comparison");
>>> + return error_mark_node;
>>> + }
>>
>> You are right, what I meant here is TREE_TYPE (type1) != TREE_TYPE
>> (type2), because vector (4, int) have the same number of elements as
>> vector (4, float). This would be fixed in the new version.
>>
>>>
>>> Joseph may have comments about the fully-fold stuff that follows.
>>>
>>> + /* Currently the expansion of VEC_COND_EXPR does not allow
>>> + expessions where the type of vectors you compare differs
>>> + form the type of vectors you select from. For the time
>>> + being we insert implicit conversions. */
>>> + if ((COMPARISON_CLASS_P (ifexp)
>>>
>>> Why only for comparison-class?
>> Not only, there is || involved:
>> (COMPARISON_CLASS_P (ifexp) && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
>> || TREE_TYPE
Re: Vector Comparison patch
On Mon, Aug 15, 2011 at 6:58 PM, Artem Shinkarov
wrote:
> On Mon, Aug 15, 2011 at 3:24 PM, Richard Guenther
> wrote:
>> On Fri, Aug 12, 2011 at 4:03 AM, Artem Shinkarov
>> wrote:
>>> Hi
>>>
>>> Here is a completed version of the vector comparison patch we
>>> discussed a long time ago here:
>>> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01184.html
>>>
>>> The patch implements vector comparison according to the OpenCL
>>> standard, when the result of the comparison of two vectors is vector
>>> of signed integers, where -1 represents true and 0 false.
>>>
>>> The patch implements vector conditional res = VCOND
>>> which is expanded into:
>>> foreach (i in length (V1)) res[i] = V1 == 0 ? V3[i] : V2[i].
>>
>> Some comments on the patch below. First, in general I don't see
>> why you need a new target hook to specify whether to "vectorize"
>> a comparison. Why are the existing hooks used by the vectorizer
>> not enough?
>>
>> Index: gcc/fold-const.c
>> ===
>> --- gcc/fold-const.c (revision 177665)
>> +++ gcc/fold-const.c (working copy)
>> @@ -9073,34 +9073,61 @@ fold_comparison (location_t loc, enum tr
>> floating-point, we can only do some of these simplifications.) */
>> if (operand_equal_p (arg0, arg1, 0))
>> {
>> - switch (code)
>> + if (TREE_CODE (TREE_TYPE (arg0)) == VECTOR_TYPE)
>> {
>> - case EQ_EXPR:
>> - if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
>> - || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
>> - return constant_boolean_node (1, type);
>>
>> I think this change should go in a separate patch for improved
>> constant folding. It shouldn't be necessary for enabling vector compares,
>> no?
>
> Unfortunately no, this case must be covered here, otherwise x != x
> condition fails.
How does it fail?
>>
>> + if (TYPE_UNSIGNED (TREE_TYPE (TREE_TYPE (ifexp
>> + {
>> + error_at (colon_loc, "vector comparison must be of signed "
>> + "integer vector type");
>> + return error_mark_node;
>> + }
>>
>> why that?
>
> Well, later on I rely on this fact. I mean OpenCL says that it should
> return -1 in the sense that all bits set. I don't really know, I can
> support unsigned masks as well, but wouldn't it just introduce a
> source for possible errors. I mean that natural choice for true and
> flase is 0 and 1, not 0 and -1. Anyway I don't have a strong opinion
> there, and I could easily adjust it if we decide that we want it.
I think we want to allow both signed and unsigned masks.
>>
>> + if (TYPE_VECTOR_SUBPARTS (type1) != TYPE_VECTOR_SUBPARTS (type2)
>> + || TYPE_VECTOR_SUBPARTS (TREE_TYPE (ifexp))
>> + != TYPE_VECTOR_SUBPARTS (type1))
>> + {
>> + error_at (colon_loc, "vectors of different length found in "
>> + "vector comparison");
>> + return error_mark_node;
>> + }
>>
>> I miss verification that type1 and type2 are vector types, or is that done
>> elsewhere? I think type1 and type2 are already verified to be
>> compatible (but you might double-check). At least the above would be
>> redundant with
>
> Thanks, type1 and type2 both vectors comparison is missing, going to
> be added in the new version of the patch.
>>
>> + if (type1 != type2)
>> + {
>> + error_at (colon_loc, "vectors of different types involved in "
>> + "vector comparison");
>> + return error_mark_node;
>> + }
>
> You are right, what I meant here is TREE_TYPE (type1) != TREE_TYPE
> (type2), because vector (4, int) have the same number of elements as
> vector (4, float). This would be fixed in the new version.
>
>>
>> Joseph may have comments about the fully-fold stuff that follows.
>>
>> + /* Currently the expansion of VEC_COND_EXPR does not allow
>> + expessions where the type of vectors you compare differs
>> + form the type of vectors you select from. For the time
>> + being we insert implicit conversions. */
>> + if ((COMPARISON_CLASS_P (ifexp)
>>
>> Why only for comparison-class?
> Not only, there is || involved:
> (COMPARISON_CLASS_P (ifexp) && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
> || TREE_TYPE (ifexp) != type1
>
> So if this is a comparison class, we check the first operand, because
> the result of the comparison fits, however the operands could not. In
> case we have an expression of signed vector, we know that we would
> transform it into exp != {0,0,...} in tree-vect-generic.c, but if the
> types of operands do not match we convert them.
Hm, ok ... let's hope we can sort-out the backend issues before this
patch goes in so we can remove this converting stuff.
>>
>> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
>> + || TREE_TYPE (ifexp) != type1)
>> + {
>> + tree comp_type = COMPARISON_CLASS
Re: Vector Comparison patch
On Mon, Aug 15, 2011 at 3:24 PM, Richard Guenther
wrote:
> On Fri, Aug 12, 2011 at 4:03 AM, Artem Shinkarov
> wrote:
>> Hi
>>
>> Here is a completed version of the vector comparison patch we
>> discussed a long time ago here:
>> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01184.html
>>
>> The patch implements vector comparison according to the OpenCL
>> standard, when the result of the comparison of two vectors is vector
>> of signed integers, where -1 represents true and 0 false.
>>
>> The patch implements vector conditional res = VCOND
>> which is expanded into:
>> foreach (i in length (V1)) res[i] = V1 == 0 ? V3[i] : V2[i].
>
> Some comments on the patch below. First, in general I don't see
> why you need a new target hook to specify whether to "vectorize"
> a comparison. Why are the existing hooks used by the vectorizer
> not enough?
>
> Index: gcc/fold-const.c
> ===
> --- gcc/fold-const.c (revision 177665)
> +++ gcc/fold-const.c (working copy)
> @@ -9073,34 +9073,61 @@ fold_comparison (location_t loc, enum tr
> floating-point, we can only do some of these simplifications.) */
> if (operand_equal_p (arg0, arg1, 0))
> {
> - switch (code)
> + if (TREE_CODE (TREE_TYPE (arg0)) == VECTOR_TYPE)
> {
> - case EQ_EXPR:
> - if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
> - || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
> - return constant_boolean_node (1, type);
>
> I think this change should go in a separate patch for improved
> constant folding. It shouldn't be necessary for enabling vector compares, no?
Unfortunately no, this case must be covered here, otherwise x != x
condition fails.
>
> + if (TYPE_UNSIGNED (TREE_TYPE (TREE_TYPE (ifexp
> + {
> + error_at (colon_loc, "vector comparison must be of signed "
> + "integer vector type");
> + return error_mark_node;
> + }
>
> why that?
Well, later on I rely on this fact. I mean OpenCL says that it should
return -1 in the sense that all bits set. I don't really know, I can
support unsigned masks as well, but wouldn't it just introduce a
source for possible errors. I mean that natural choice for true and
flase is 0 and 1, not 0 and -1. Anyway I don't have a strong opinion
there, and I could easily adjust it if we decide that we want it.
>
> + if (TYPE_VECTOR_SUBPARTS (type1) != TYPE_VECTOR_SUBPARTS (type2)
> + || TYPE_VECTOR_SUBPARTS (TREE_TYPE (ifexp))
> + != TYPE_VECTOR_SUBPARTS (type1))
> + {
> + error_at (colon_loc, "vectors of different length found in "
> + "vector comparison");
> + return error_mark_node;
> + }
>
> I miss verification that type1 and type2 are vector types, or is that done
> elsewhere? I think type1 and type2 are already verified to be
> compatible (but you might double-check). At least the above would be
> redundant with
Thanks, type1 and type2 both vectors comparison is missing, going to
be added in the new version of the patch.
>
> + if (type1 != type2)
> + {
> + error_at (colon_loc, "vectors of different types involved in "
> + "vector comparison");
> + return error_mark_node;
> + }
You are right, what I meant here is TREE_TYPE (type1) != TREE_TYPE
(type2), because vector (4, int) have the same number of elements as
vector (4, float). This would be fixed in the new version.
>
> Joseph may have comments about the fully-fold stuff that follows.
>
> + /* Currently the expansion of VEC_COND_EXPR does not allow
> + expessions where the type of vectors you compare differs
> + form the type of vectors you select from. For the time
> + being we insert implicit conversions. */
> + if ((COMPARISON_CLASS_P (ifexp)
>
> Why only for comparison-class?
Not only, there is || involved:
(COMPARISON_CLASS_P (ifexp) && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
|| TREE_TYPE (ifexp) != type1
So if this is a comparison class, we check the first operand, because
the result of the comparison fits, however the operands could not. In
case we have an expression of signed vector, we know that we would
transform it into exp != {0,0,...} in tree-vect-generic.c, but if the
types of operands do not match we convert them.
>
> + && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
> + || TREE_TYPE (ifexp) != type1)
> + {
> + tree comp_type = COMPARISON_CLASS_P (ifexp)
> + ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
> + : TREE_TYPE (ifexp);
> + tree vcond;
> +
> + op1 = convert (comp_type, op1);
> + op2 = convert (comp_type, op2);
> + vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
> + vcond = convert (type1, vcond);
> + return vcond;
> + }
Re: Vector Comparison patch
On Fri, Aug 12, 2011 at 4:03 AM, Artem Shinkarov
wrote:
> Hi
>
> Here is a completed version of the vector comparison patch we
> discussed a long time ago here:
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01184.html
>
> The patch implements vector comparison according to the OpenCL
> standard, when the result of the comparison of two vectors is vector
> of signed integers, where -1 represents true and 0 false.
>
> The patch implements vector conditional res = VCOND
> which is expanded into:
> foreach (i in length (V1)) res[i] = V1 == 0 ? V3[i] : V2[i].
Some comments on the patch below. First, in general I don't see
why you need a new target hook to specify whether to "vectorize"
a comparison. Why are the existing hooks used by the vectorizer
not enough?
Index: gcc/fold-const.c
===
--- gcc/fold-const.c(revision 177665)
+++ gcc/fold-const.c(working copy)
@@ -9073,34 +9073,61 @@ fold_comparison (location_t loc, enum tr
floating-point, we can only do some of these simplifications.) */
if (operand_equal_p (arg0, arg1, 0))
{
- switch (code)
+ if (TREE_CODE (TREE_TYPE (arg0)) == VECTOR_TYPE)
{
- case EQ_EXPR:
- if (! FLOAT_TYPE_P (TREE_TYPE (arg0))
- || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0
- return constant_boolean_node (1, type);
I think this change should go in a separate patch for improved
constant folding. It shouldn't be necessary for enabling vector compares, no?
+ if (TYPE_UNSIGNED (TREE_TYPE (TREE_TYPE (ifexp
+{
+ error_at (colon_loc, "vector comparison must be of signed "
+ "integer vector type");
+ return error_mark_node;
+}
why that?
+ if (TYPE_VECTOR_SUBPARTS (type1) != TYPE_VECTOR_SUBPARTS (type2)
+ || TYPE_VECTOR_SUBPARTS (TREE_TYPE (ifexp))
+ != TYPE_VECTOR_SUBPARTS (type1))
+{
+ error_at (colon_loc, "vectors of different length found in "
+ "vector comparison");
+ return error_mark_node;
+}
I miss verification that type1 and type2 are vector types, or is that done
elsewhere? I think type1 and type2 are already verified to be
compatible (but you might double-check). At least the above would be
redundant with
+ if (type1 != type2)
+{
+ error_at (colon_loc, "vectors of different types involved in "
+ "vector comparison");
+ return error_mark_node;
+}
Joseph may have comments about the fully-fold stuff that follows.
+ /* Currently the expansion of VEC_COND_EXPR does not allow
+expessions where the type of vectors you compare differs
+form the type of vectors you select from. For the time
+being we insert implicit conversions. */
+ if ((COMPARISON_CLASS_P (ifexp)
Why only for comparison-class?
+ && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != type1)
+ || TREE_TYPE (ifexp) != type1)
+ {
+ tree comp_type = COMPARISON_CLASS_P (ifexp)
+ ? TREE_TYPE (TREE_OPERAND (ifexp, 0))
+ : TREE_TYPE (ifexp);
+ tree vcond;
+
+ op1 = convert (comp_type, op1);
+ op2 = convert (comp_type, op2);
+ vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
+ vcond = convert (type1, vcond);
+ return vcond;
+ }
+ else
+ return build3 (VEC_COND_EXPR, type1, ifexp, op1, op2);
In the end we of course will try to fix the middle-end/backends to
allow mixed types here as the current restriction doesn't really make sense.
case EQ_EXPR:
case NE_EXPR:
+ if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE)
+{
+ tree intt;
+ if (TREE_TYPE (type0) != TREE_TYPE (type1))
+{
+ error_at (location, "comparing vectors with different "
+ "element types");
+ return error_mark_node;
+}
+
+ if (TYPE_VECTOR_SUBPARTS (type0) != TYPE_VECTOR_SUBPARTS (type1))
+{
+ error_at (location, "comparing vectors with different "
+ "number of elements");
+ return error_mark_node;
+}
as above - compatibility should already be ensured, thus type0 == type1
here?
+/* Try a hardware hook for vector comparison or
+ extract comparison piecewise. */
+static tree
+expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree op0,
+ tree op1, enum tree_code code)
comments should mention and describe all function arguments.
+/* Expand vector condition EXP which should have the form
+ VEC_COND_EXPR into the following
+ vector:
+ {cond[i] != 0 ? vec0[i] : vec1[i], ... }
+ i changes from 0 to TYPE_VECTOR_SUBPARTS (TREE_TYPE (vec0)). */
+static tree
+
