On Wed, 26 Aug 2015 12:06:59 +0100 "Ben Avison" <bavi...@riscosopen.org> wrote:
> On Wed, 26 Aug 2015 09:46:49 +0100, Pekka Paalanen <ppaala...@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
pgp3WQH2oz_Fl.pgp
Description: OpenPGP digital signature
_______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman