817153 - AltiVec acceleration for qcms

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Tobias Netzel]

Attachment: VMX acceleration for qcms

This implements VMX (Altivec) acceleration for qcms, inspired by the SSE2 implementation.
The newly added file follows the mozilla style guide while the other modifications follow the style of the rest of the code in that changed files.

Comment 1

[Tobias Netzel]

Attachment: VMX acceleration for qcms

Fixed an unneeded define.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

As a heads up, I'm planing on replacing the floating point code with integer code as shown in bug 802109

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Do you have numbers for how much this improves performance? (Timing calls to qcms_transform_data())

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 687280 [details] [diff] [review]
VMX acceleration for qcms

Review of attachment 687280 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/qcms/transform-vmx.c
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This code should use the same license as the rest of qcms and the same formatting.

@@ +5,5 @@
> +#include <altivec.h>
> +
> +#include "qcmsint.h"
> +
> +#undef COPY_CONSTANTS_TO_ALIGNED_LOCATION

Can you add a comment about why you chose this option?

@@ +49,5 @@
> +  const uint8_t* otdata_g = &transform->output_table_g->data[0];
> +  const uint8_t* otdata_b = &transform->output_table_b->data[0];
> +
> +  /* input matrix values never change */
> +  register const vector float mat0 = vec_ldl(0, (vector float*)mat[0]);

Does using "register" do anything?

::: gfx/qcms/transform.c
@@ +52,5 @@
> +static inline int have_altivec() {
> +	static int available = -1;
> +	int new_avail = 0;
> +	char fname[64];
> +	unsigned long buf[64];

Instead of a untyped buffer it might be better to read chunks of
size Elf32_auxv_t

@@ +59,5 @@
> +
> +	if (available != -1)
> +		return available;
> +
> +	snprintf(fname, sizeof(fname)-1, "/proc/self/auxv");

No point in using sprintf here with a constant string. Just open "/proc/self/auxv" directly.

@@ +86,5 @@
> +out:
> +	available = new_avail;
> +	return available;
> +}
> +#elif defined(XP_MACOSX)

This should use defined(__APPLE__) && defined(__MACH__) to not depend on the mozilla defines.

@@ +100,5 @@
> +	size_t len = sizeof(has_vu);
> +	int err;
> +
> +	if (has_vu != -1)
> +		return has_vu;

It might be good to have some naming consistency between this function and the linux one above. (available vs. has_vu)

@@ +102,5 @@
> +
> +	if (has_vu != -1)
> +		return has_vu;
> +
> +	err = sysctl(sels, 2, &has_vu, &len, NULL, 0);   

trailing space.

Comment 5

[Tobias Netzel]

I measured performance some time ago using a modified version of your lcms-compare.c and the vectorized version was 35% faster on a low end G4 500MHz machine.

Comment 6

[Tobias Netzel]

Attachment: AltiVec acceleration for qcms

glandium told me to call it AltiVec instead of VMX.
COPY_CONSTANTS_TO_ALIGNED_LOCATION code path was removed as it was a workaround that is not needed anymore.
'register' declaration didn't anything indeed.
Linux AltiVec detection was rewritten using ElfW(auxv_t) buffers. I can't test it though because I don't currently have a linux install to test it with. It seems to compile, though I don't know what the requirements are to have ElfW(auxv_t) defined in the system headers.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc851f15ebd9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fc851f15ebd9