Closed Bug 817153 Opened 8 years ago Closed 8 years ago

AltiVec acceleration for qcms

Categories

(Core :: GFX: Color Management, defect)

PowerPC
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: tobias.netzel, Assigned: tobias.netzel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch VMX acceleration for qcms (obsolete) — Splinter Review
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.
Attachment #687279 - Attachment is patch: true
Attachment #687279 - Flags: review?
Blocks: 817033
Attached patch VMX acceleration for qcms (obsolete) — Splinter Review
Fixed an unneeded define.
Attachment #687279 - Attachment is obsolete: true
Attachment #687279 - Flags: review?
Attachment #687280 - Flags: review?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #687280 - Flags: review? → review?(jmuizelaar)
As a heads up, I'm planing on replacing the floating point code with integer code as shown in bug 802109
Do you have numbers for how much this improves performance? (Timing calls to qcms_transform_data())
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.
Attachment #687280 - Flags: review?(jmuizelaar) → review-
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.
Summary: VMX acceleration for qcms → 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.
Attachment #687280 - Attachment is obsolete: true
Attachment #698275 - Flags: review?(jmuizelaar)
Attachment #698275 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/fc851f15ebd9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.