[Pixman] [PATCH 1/4] pixman-fast-path: Add over_n_8888 fast path (disabled)

[Pekka Paalanen]

On Wed, 26 Aug 2015 12:06:59 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> On Wed, 26 Aug 2015 09:46:49 +0100, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > It's clearly controversial to add C fast paths, because it affects all
> > targets that don't have an asm fast path for the same, and we cannot
> > tell by just review whether it is going to be for better or (much)
> > worse.
> 
> Yes, it's always going to be a risk changing the cross-platform
> functions. Few developers are going to be able to test all the supported
> platforms, so we're always going to need help checking performance.
> Should that be a reason not to even try C fast paths though?

This is very much IMHO, but (to not even try):
- yes, if one is going to have a platform-specific fast path anyway
- yes, if one can reasonably write a platform-specific fast path, that
  is, you are working on just one or maybe two platforms
- ??, if it's something new that rarely any platform accelerates

If Oded hadn't benchmarked this, we would likely have landed this C
fast path. Then no-one would have realized it's actually slower than
the generic path afterwards.

Whether this is an isolated case or not, I don't know. But since we
tend to be more safe than sorry with pixman, I would err on the side of
not writing C fast paths. Hmm, assuming you can beat the compiler with
hand-crafted asm...?

Or, we could say we want N amount of checking on different platforms
before we can land a new C fast path, but I suspect that is as good as
just dropping it, considering how much reviews we get on patches.


It would be *really* nice if we could somehow use a benchmark mode
where we could run an operation with every possible implementation and
compare them. I wonder, can we already do that with PIXMAN_DISABLE?

I see the code recognizes these names for PIXMAN_DISABLE:

pixman/pixman-arm.c:210:    if (!_pixman_disabled ("arm-simd") && have_feature (ARM_V6))
pixman/pixman-arm.c:215:    if (!_pixman_disabled ("arm-iwmmxt") && have_feature (ARM_IWMMXT))
pixman/pixman-arm.c:220:    if (!_pixman_disabled ("arm-neon") && have_feature (ARM_NEON))
pixman/pixman-implementation.c:390:    if (!_pixman_disabled ("fast"))
pixman/pixman-mips.c:73:    if (!_pixman_disabled ("loongson-mmi") && have_feature ("Loongson"))
pixman/pixman-mips.c:78:    if (!_pixman_disabled ("mips-dspr2"))
pixman/pixman-ppc.c:150:    if (!_pixman_disabled ("vmx") && pixman_have_vmx ())
pixman/pixman-x86.c:233:    if (!_pixman_disabled ("mmx") && have_feature (MMX_BITS))
pixman/pixman-x86.c:238:    if (!_pixman_disabled ("sse2") && have_feature (SSE2_BITS))
pixman/pixman-x86.c:243:    if (!_pixman_disabled ("ssse3") && have_feature (SSSE3_BITS))

Would that be everything we need to control to be able to test every
possible path on a platform?

This could be the way to test whether Pixman is choosing the optimal
implementation of what it already has. Some heavy scripting around
lowlevel-blt-bench could produce a nice table of results.

Just an idea. Of course it doesn't remove the need to actually try
things on the various machines.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20150827/aa0819f8/attachment.sig>