[RFC,1/2] Remove seemingly unneeded comparison(s)

Submitted by Emil Velikov on April 24, 2016, 6:22 p.m.

Details

Message ID 1461522177-5796-1-git-send-email-emil.l.velikov@gmail.com
State Under Review
Headers show
Series "Series without cover letter" ( rev: 1 ) in Pixman

Not browsing as part of any series.

Commit Message

Emil Velikov April 24, 2016, 6:22 p.m.
With commit ed39992564b "Use pixman_transform_point_31_16() from
pixman_transform_point()" we added some strange hunks.

Namely: we copy the data from the internal storage to the user vector
only to compare them immediately after.

Cc: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---

Siarhei, what is the intent with the original commit ? Any ideas why
things crash ?

Seemingly this can be dropped/replaced with TRUE, yet it causes one of
the tests () to segfault in the optimised SSE2 codepath -
scaled_bilinear_scanline_sse2_8888_8888_SRC.

BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER

Regards,
Emil
---

 pixman/pixman-matrix.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
index 65b3d32..117015b 100644
--- a/pixman/pixman-matrix.c
+++ b/pixman/pixman-matrix.c
@@ -393,9 +393,7 @@  pixman_transform_point_3d (const struct pixman_transform *transform,
     vector->vector[1] = tmp.v[1];
     vector->vector[2] = tmp.v[2];
 
-    return vector->vector[0] == tmp.v[0] &&
-           vector->vector[1] == tmp.v[1] &&
-           vector->vector[2] == tmp.v[2];
+    return TRUE;
 }
 
 PIXMAN_EXPORT pixman_bool_t
@@ -414,9 +412,7 @@  pixman_transform_point (const struct pixman_transform *transform,
     vector->vector[1] = tmp.v[1];
     vector->vector[2] = tmp.v[2];
 
-    return vector->vector[0] == tmp.v[0] &&
-           vector->vector[1] == tmp.v[1] &&
-           vector->vector[2] == tmp.v[2];
+    return TRUE;
 }
 
 PIXMAN_EXPORT pixman_bool_t

Comments

Doesn't this check for NaNs?

On Sun, Apr 24, 2016 at 8:22 PM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> With commit ed39992564b "Use pixman_transform_point_31_16() from
> pixman_transform_point()" we added some strange hunks.
>
> Namely: we copy the data from the internal storage to the user vector
> only to compare them immediately after.
>
> Cc: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> ---
>
> Siarhei, what is the intent with the original commit ? Any ideas why
> things crash ?
>
> Seemingly this can be dropped/replaced with TRUE, yet it causes one of
> the tests () to segfault in the optimised SSE2 codepath -
> scaled_bilinear_scanline_sse2_8888_8888_SRC.
>
> BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER
>
> Regards,
> Emil
> ---
>
>  pixman/pixman-matrix.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
> index 65b3d32..117015b 100644
> --- a/pixman/pixman-matrix.c
> +++ b/pixman/pixman-matrix.c
> @@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct
> pixman_transform *transform,
>      vector->vector[1] = tmp.v[1];
>      vector->vector[2] = tmp.v[2];
>
> -    return vector->vector[0] == tmp.v[0] &&
> -           vector->vector[1] == tmp.v[1] &&
> -           vector->vector[2] == tmp.v[2];
> +    return TRUE;
>  }
>
>  PIXMAN_EXPORT pixman_bool_t
> @@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform
> *transform,
>      vector->vector[1] = tmp.v[1];
>      vector->vector[2] = tmp.v[2];
>
> -    return vector->vector[0] == tmp.v[0] &&
> -           vector->vector[1] == tmp.v[1] &&
> -           vector->vector[2] == tmp.v[2];
> +    return TRUE;
>  }
>
>  PIXMAN_EXPORT pixman_bool_t
> --
> 2.8.0
>
> _______________________________________________
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
The old code is comparing pixman_fixed_48_16_t values to
pixman_fixed_16_16_t values, thus it is checking for truncation of overflow
values.

It would probably be better to clamp these overflowed values, like
pixman_transform_point_31_16 is doing to clamp to the pixman_fixed_48_16
result. Right now the result is an odd mix of clamping and modulus. A
rewrite to go directly to clamped pixman_fixed_16_16 values would be even
better.



On Tue, Apr 26, 2016 at 10:59 AM, Petr Kobalíček <kobalicek.petr@gmail.com>
wrote:

> Doesn't this check for NaNs?
>
> On Sun, Apr 24, 2016 at 8:22 PM, Emil Velikov <emil.l.velikov@gmail.com>
> wrote:
>
>> With commit ed39992564b "Use pixman_transform_point_31_16() from
>> pixman_transform_point()" we added some strange hunks.
>>
>> Namely: we copy the data from the internal storage to the user vector
>> only to compare them immediately after.
>>
>> Cc: Siarhei Siamashka <siarhei.siamashka@gmail.com>
>> ---
>>
>> Siarhei, what is the intent with the original commit ? Any ideas why
>> things crash ?
>>
>> Seemingly this can be dropped/replaced with TRUE, yet it causes one of
>> the tests () to segfault in the optimised SSE2 codepath -
>> scaled_bilinear_scanline_sse2_8888_8888_SRC.
>>
>> BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER
>>
>> Regards,
>> Emil
>> ---
>>
>>  pixman/pixman-matrix.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
>> index 65b3d32..117015b 100644
>> --- a/pixman/pixman-matrix.c
>> +++ b/pixman/pixman-matrix.c
>> @@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct
>> pixman_transform *transform,
>>      vector->vector[1] = tmp.v[1];
>>      vector->vector[2] = tmp.v[2];
>>
>> -    return vector->vector[0] == tmp.v[0] &&
>> -           vector->vector[1] == tmp.v[1] &&
>> -           vector->vector[2] == tmp.v[2];
>> +    return TRUE;
>>  }
>>
>>  PIXMAN_EXPORT pixman_bool_t
>> @@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform
>> *transform,
>>      vector->vector[1] = tmp.v[1];
>>      vector->vector[2] = tmp.v[2];
>>
>> -    return vector->vector[0] == tmp.v[0] &&
>> -           vector->vector[1] == tmp.v[1] &&
>> -           vector->vector[2] == tmp.v[2];
>> +    return TRUE;
>>  }
>>
>>  PIXMAN_EXPORT pixman_bool_t
>> --
>> 2.8.0
>>
>> _______________________________________________
>> Pixman mailing list
>> Pixman@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/pixman
>>
>
>
> _______________________________________________
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
>
On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
> The old code is comparing pixman_fixed_48_16_t values to
> pixman_fixed_16_16_t values, thus it is checking for truncation of overflow
> values.
>
Indeed it does. I'll grep more before asking silly questions ;-)

> It would probably be better to clamp these overflowed values, like
> pixman_transform_point_31_16 is doing to clamp to the pixman_fixed_48_16
> result. Right now the result is an odd mix of clamping and modulus. A
> rewrite to go directly to clamped pixman_fixed_16_16 values would be even
> better.
>
Sounds like a plan. Sadly I doubt I'll get to it any time soon.

Thanks
Emil
On Wed, 27 Apr 2016 09:56:44 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
> > The old code is comparing pixman_fixed_48_16_t values to
> > pixman_fixed_16_16_t values, thus it is checking for truncation of overflow
> > values.
> >  
> Indeed it does. I'll grep more before asking silly questions ;-)
> 
> > It would probably be better to clamp these overflowed values, like
> > pixman_transform_point_31_16 is doing to clamp to the pixman_fixed_48_16
> > result. Right now the result is an odd mix of clamping and modulus. A
> > rewrite to go directly to clamped pixman_fixed_16_16 values would be even
> > better.
> >  
> Sounds like a plan. Sadly I doubt I'll get to it any time soon.

Wasn't the point of the boolean return from these functions to tell the
caller to drop what it is doing because it cannot be done properly?


Thanks,
pq
On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Wed, 27 Apr 2016 09:56:44 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
> > > The old code is comparing pixman_fixed_48_16_t values to
> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
> overflow
> > > values.
> > >
> > Indeed it does. I'll grep more before asking silly questions ;-)
> >
> > > It would probably be better to clamp these overflowed values, like
> > > pixman_transform_point_31_16 is doing to clamp to the
> pixman_fixed_48_16
> > > result. Right now the result is an odd mix of clamping and modulus. A
> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
> even
> > > better.
> > >
> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>
> Wasn't the point of the boolean return from these functions to tell the
> caller to drop what it is doing because it cannot be done properly?
>

Dropping a fill is a lot worse than trying to simulate it using the clamped
path. It will produce a wrong result if one of the edges connected to a
clamped point passes through the clip, but this often does not happen, and
the transition to the wrong result is gradual as the point moves outside
the clamped region.

More importantly the caller cannot do anything with the return values right
now, as they are modulus MAX_16_16+1. Even the direction they are in is
lost.
On 27 April 2016 at 18:46, Bill Spitzak <spitzak@gmail.com> wrote:
> On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>
>> On Wed, 27 Apr 2016 09:56:44 +0100
>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>
>> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
>> > > The old code is comparing pixman_fixed_48_16_t values to
>> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> > > overflow
>> > > values.
>> > >
>> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >
>> > > It would probably be better to clamp these overflowed values, like
>> > > pixman_transform_point_31_16 is doing to clamp to the
>> > > pixman_fixed_48_16
>> > > result. Right now the result is an odd mix of clamping and modulus. A
>> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> > > even
>> > > better.
>> > >
>> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>>
>> Wasn't the point of the boolean return from these functions to tell the
>> caller to drop what it is doing because it cannot be done properly?
>
>
> Dropping a fill is a lot worse than trying to simulate it using the clamped
> path. It will produce a wrong result if one of the edges connected to a
> clamped point passes through the clip, but this often does not happen, and
> the transition to the wrong result is gradual as the point moves outside the
> clamped region.
>
> More importantly the caller cannot do anything with the return values right
> now, as they are modulus MAX_16_16+1. Even the direction they are in is
> lost.
>
I think that keeping the user provided memory as-is when the function
does not succeed is a good idea.
Afaics currently the contents get overwritten regardless of the result.

This is what you guys were on about, right ? Or perhaps you're
thinking about spinning v2 of the function with different
signature/behaviour ?

-Emil
On Fri, 29 Apr 2016 10:15:37 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 27 April 2016 at 18:46, Bill Spitzak <spitzak@gmail.com> wrote:
> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> >>
> >> On Wed, 27 Apr 2016 09:56:44 +0100
> >> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>  
> >> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:  
> >> > > The old code is comparing pixman_fixed_48_16_t values to
> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
> >> > > overflow
> >> > > values.
> >> > >  
> >> > Indeed it does. I'll grep more before asking silly questions ;-)
> >> >  
> >> > > It would probably be better to clamp these overflowed values, like
> >> > > pixman_transform_point_31_16 is doing to clamp to the
> >> > > pixman_fixed_48_16
> >> > > result. Right now the result is an odd mix of clamping and modulus. A
> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
> >> > > even
> >> > > better.
> >> > >  
> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.  
> >>
> >> Wasn't the point of the boolean return from these functions to tell the
> >> caller to drop what it is doing because it cannot be done properly?  
> >
> >
> > Dropping a fill is a lot worse than trying to simulate it using the clamped
> > path. It will produce a wrong result if one of the edges connected to a
> > clamped point passes through the clip, but this often does not happen, and
> > the transition to the wrong result is gradual as the point moves outside the
> > clamped region.
> >
> > More importantly the caller cannot do anything with the return values right
> > now, as they are modulus MAX_16_16+1. Even the direction they are in is
> > lost.
> >  
> I think that keeping the user provided memory as-is when the function
> does not succeed is a good idea.
> Afaics currently the contents get overwritten regardless of the result.
> 
> This is what you guys were on about, right ? Or perhaps you're
> thinking about spinning v2 of the function with different
> signature/behaviour ?

Hi Emil,

I think the conclusion was that the comparisons are not useless, and
this patch should be dropped. You noted it yourself that this patch
causes a regression in the test suite.


Thanks,
pq
On 29 April 2016 at 11:35, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Fri, 29 Apr 2016 10:15:37 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
>> On 27 April 2016 at 18:46, Bill Spitzak <spitzak@gmail.com> wrote:
>> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> >>
>> >> On Wed, 27 Apr 2016 09:56:44 +0100
>> >> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>
>> >> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
>> >> > > The old code is comparing pixman_fixed_48_16_t values to
>> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> >> > > overflow
>> >> > > values.
>> >> > >
>> >> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >> >
>> >> > > It would probably be better to clamp these overflowed values, like
>> >> > > pixman_transform_point_31_16 is doing to clamp to the
>> >> > > pixman_fixed_48_16
>> >> > > result. Right now the result is an odd mix of clamping and modulus. A
>> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> >> > > even
>> >> > > better.
>> >> > >
>> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>> >>
>> >> Wasn't the point of the boolean return from these functions to tell the
>> >> caller to drop what it is doing because it cannot be done properly?
>> >
>> >
>> > Dropping a fill is a lot worse than trying to simulate it using the clamped
>> > path. It will produce a wrong result if one of the edges connected to a
>> > clamped point passes through the clip, but this often does not happen, and
>> > the transition to the wrong result is gradual as the point moves outside the
>> > clamped region.
>> >
>> > More importantly the caller cannot do anything with the return values right
>> > now, as they are modulus MAX_16_16+1. Even the direction they are in is
>> > lost.
>> >
>> I think that keeping the user provided memory as-is when the function
>> does not succeed is a good idea.
>> Afaics currently the contents get overwritten regardless of the result.
>>
>> This is what you guys were on about, right ? Or perhaps you're
>> thinking about spinning v2 of the function with different
>> signature/behaviour ?
>
> Hi Emil,
>
> I think the conclusion was that the comparisons are not useless, and
> this patch should be dropped. You noted it yourself that this patch
> causes a regression in the test suite.
>
Fully agree on both points. Just trying to understand what you and
Bill are talking about and suggest that if one changes things, would
be nice to avoid "feeding garbage" back to the user [on error].

Thanks
Emil
If the comparison fails, the returned values are modulus fixed_16_16,
rather than clamped. I think that gives a lot less possibilities for the
calling code to recover from or simulate the results of the out-of-range
value.


On Fri, Apr 29, 2016 at 6:33 AM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> On 29 April 2016 at 11:35, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Fri, 29 Apr 2016 10:15:37 +0100
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> >> On 27 April 2016 at 18:46, Bill Spitzak <spitzak@gmail.com> wrote:
> >> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen@gmail.com>
> wrote:
> >> >>
> >> >> On Wed, 27 Apr 2016 09:56:44 +0100
> >> >> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >> >>
> >> >> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak@gmail.com> wrote:
> >> >> > > The old code is comparing pixman_fixed_48_16_t values to
> >> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation
> of
> >> >> > > overflow
> >> >> > > values.
> >> >> > >
> >> >> > Indeed it does. I'll grep more before asking silly questions ;-)
> >> >> >
> >> >> > > It would probably be better to clamp these overflowed values,
> like
> >> >> > > pixman_transform_point_31_16 is doing to clamp to the
> >> >> > > pixman_fixed_48_16
> >> >> > > result. Right now the result is an odd mix of clamping and
> modulus. A
> >> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values
> would be
> >> >> > > even
> >> >> > > better.
> >> >> > >
> >> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
> >> >>
> >> >> Wasn't the point of the boolean return from these functions to tell
> the
> >> >> caller to drop what it is doing because it cannot be done properly?
> >> >
> >> >
> >> > Dropping a fill is a lot worse than trying to simulate it using the
> clamped
> >> > path. It will produce a wrong result if one of the edges connected to
> a
> >> > clamped point passes through the clip, but this often does not
> happen, and
> >> > the transition to the wrong result is gradual as the point moves
> outside the
> >> > clamped region.
> >> >
> >> > More importantly the caller cannot do anything with the return values
> right
> >> > now, as they are modulus MAX_16_16+1. Even the direction they are in
> is
> >> > lost.
> >> >
> >> I think that keeping the user provided memory as-is when the function
> >> does not succeed is a good idea.
> >> Afaics currently the contents get overwritten regardless of the result.
> >>
> >> This is what you guys were on about, right ? Or perhaps you're
> >> thinking about spinning v2 of the function with different
> >> signature/behaviour ?
> >
> > Hi Emil,
> >
> > I think the conclusion was that the comparisons are not useless, and
> > this patch should be dropped. You noted it yourself that this patch
> > causes a regression in the test suite.
> >
> Fully agree on both points. Just trying to understand what you and
> Bill are talking about and suggest that if one changes things, would
> be nice to avoid "feeding garbage" back to the user [on error].
>
> Thanks
> Emil
>