[Pixman] Cleaning patchwork

[Oded Gabbay]

On Mon, Jan 4, 2016 at 4:31 PM, Ben Avison <bavison at riscosopen.org> wrote:
> On Tue, 22 Dec 2015 13:14:21 -0000, Oded Gabbay <oded.gabbay at gmail.com>
> wrote:
>
>> Hi Ben,
>>
>> I would like to clean pixman's patchwork and you have about 20
>> outstanding patches.
>
> [...]
>
>> [4/4] pixman-fast-path: Make bilinear cover fetcher use COVER_CLIP_TIGHT
>> flag
>> [3/4] armv7/mips/sse2: Make bilinear cover fast paths use COVER_CLIP_TIGHT
>> flag
>> [2/4] Introduce FAST_PATH_SAMPLES_COVER_CLIP_TIGHT_BILINEAR flag
>
>
> I still think these are a worthwhile improvement and have described some
> conditions under which it should be measurable using affine-bench. I believe
> Pekka wanted to prove it using real-world traces though, and since there was
> no measurable change for better or worse using the normal Cairo traces, he
> was attempting to capture some new ones that would exercise the cases I
> described. Last I heard though, he had found that Cairo's tracer was broken,
> so he was unable to make progress. Under the circumstances, do you think
> affine-bench results alone would be acceptable?

Hi Pekka,
I admit I didn't follow these patches when Ben sent them as you and
Siarhei reviewed them.
May I ask you to write your opinion on the matter ?

>
>> Resolve implementation-defined behaviour for division rounded to -infinity
>
>
> This one got bogged down in an argument about whether C89 should still be
> supported or not, which I'm not sure was ever resolved?

So from reading back the thread, I saw two things:
1. Pekka and Siarhei  wrote about adding test(s) to verify behavior of % and /
2. C89 compatibility issue. Because we have already forked 0.34 and
0.36 seems like a 2017 story, I think we can start phasing it out in
the 0.35 development releases and see if someone complains.

>
>> [05/37,v2] composite flags: Early detection of fully opaque 1x1
>> repeating source images
>> [10/37,v2] pixman-fast-path: Add in_8888_8 fast path
>> [11/37,v2] armv6: Add in_8888_8 fast path
>> [23/37,v2] armv6: Add optimised scanline fetchers and writeback for
>> r5g6b5 and a8
>> [24/37,v2] armv6: Add optimised scanline fetcher for a1r5g5b5
>> [25/37,v2] armv6: Add src_1555_8888 fast path
>
>
> v1 of these patches were reviewed (excluding the ARM assembly parts) by
> Søren on 05 Oct 2014, and v2 addressed the issue he raised. There haven't
> been any comments on the reposted versions.

Could you re-post only the the ones you fixed with the v2 remarks ?
I'll take a look and hopefully we could merge them.

>
>> armv7: Re-use existing fast paths in more cases
>> armv7: Re-use existing fast paths in more cases
>
>
> These apply the same rules that Søren suggested for my ARMv6 paths in the v2
> patches above to the pre-existing ARMv7 paths as well. The only review
> comment received so far was that the two patches needed different names.

Same comment as above.

>
>> [2/5,v2] armv7: Add in_8888_8 fast path
>
>
> The only difference for v2 here was again just a matter of defining the
> widest possible set of pixel formats to match the fast path.

Same comment as above.

>
>> [5/5] armv7: Add src_1555_8888 fast path
>> [4/5] armv7: Add in_reverse_8888_8888 fast path
>> [3/5] armv7: Add in_n_8888 fast path
>> [1/5] armv7: Use prefetch for small-width images too
>> [3/3] armv7: Use VLD-to-all-lanes
>> [2/3] armv7: Faster fill operations
>> [1/3] armv7: Coalesce scalar accesses where possible
>> Require destination images to be bitmaps
>
>
> None of these have received any comments to date. In many cases, I suspect
> it's the fact that they involve ARM assembly, and we don't have many (any?)
> active reviewers who know it. I'm not sure what we can do about that.

Send them as a different series and I'll take a look (as much as I
can, I also don't know ARM assembly).

>
>> I also suggest that for the relevant ones (if there are any), you
>> would rebase them on the latest master and re-send them as a single
>> series in order to restart the review process.
>
>
> I can certainly do that, but I had previously been asked to split my
> postings into smaller series that were independent. I have a lot more
> patches waiting to post that depend on the ones that are stuck in limbo -
> the complete set can be seen at
>
> https://github.com/bavison/pixman/commits/arm-neon-release1
>
> if you're interested. Or I could just post/repost the whole lot (72 patches
> now), like I did with my 37-patch series from 2014.
>
> Ben

Let's start with the patches you wrote above and move step by step after it.

Thanks,

        Oded