[cairo] [PATCH] Added MIPS32R2 and MIPS DSP ASE optimized functions

[Soeren Sandmann]

Georgi Beloev <gb at beloev.net> writes:

> From 118b1f5596f72be7fed85ba408ff2961b3308038 Mon Sep 17 00:00:00 2001
> From: Georgi Beloev <gb at beloev.net>
> Date: Wed, 8 Sep 2010 17:34:22 -0700
> Subject: [PATCH] Added MIPS32R2 and MIPS DSP ASE optimized functions.
> 
> The following functions were implemented for MIPS32R2:
>   - pixman_fill32()
>   - fast_composite_over_n_8_8888()
> 
> The following functions were implemented for MIPS DSP ASE:
>   - combine_over_u()
>   - fast_composite_over_n_8_8888()
> 
> Additionally, MIPS DSP ASE uses the MIPS32R2 pixman_fill32() function.
> 
> Use configure commands similar to the ones below to select the target
> processor and, correspondingly, the target instruction set:

Thanks for the patch - new CPU specific fast paths are always
welcome. As Dmitri mentioned, please CC

    pixman at lists.freedesktop.org 

on Pixman patches. I have pushed it to a git branch here:

    http://cgit.freedesktop.org/~sandmann/pixman/commit/?h=mips&id=8beceb87436c222a9581d48154075db4cc7e8234

Some comments:

- Does "make check" pass? I'm a little suspicious about these:

+       addu            $t3, $t3, $t9   // can't overflow; rev2: addu_s.ph
+       addu            $t4, $t4, $t9   // can't overflow; rev2: addu_s.ph

  because overflow definitely can happen if the source has color
  components that are larger than the alpha channel (which is legal,
  though unusual). But I don't know the DSP ASE instruction set, so I
  could be wrong about this.

- I think this needs to be split into (at least) two commits, one that
  adds the mips32r2 part, and one that adds the DSP ASE 1 part. It
  would probably also be useful to enable the combiner and the
  n_8_8888 fast paths separately so that they can be bisected.

- Is there a reason to not do runtime checking? I realize that most
  people using MIPS will likely do so on an embedded system where they
  know ahead of time what the CPU supports, but we do have runtime
  checking for the other CPU specific implementations.

- This:

>  pixman_implementation_t *
>  _pixman_choose_implementation (void)
>  {
> @@ -593,6 +604,19 @@ _pixman_choose_implementation (void)
>  	return _pixman_implementation_create_vmx ();
>  #endif
>  
> +#ifdef USE_MIPS32R2
> +	pixman_implementation_t *imp = NULL;
> +    if (pixman_have_mips32r2 ())
> +		imp = _pixman_implementation_create_mips32r2 (imp);
> +
> +#ifdef USE_MIPS_DSPASE1
> +	if (pixman_have_mips_dspase1 ())
> +		imp = _pixman_implementation_create_mips_dspase1 (imp);
> +#endif // USE_MIPS_DSPASE1
> +
> +	return imp;
> +#endif //  USE_MIPS32R2
> +
>      return _pixman_implementation_create_fast_path ();
>  }

  doesn't look quite right to me. If both have_mips32r2() and
  have_dspase1() return FALSE, then this will return a NULL
  implementation. 

  Also, if it is intentional that DSPASE1 is only used when MIPS32R2
  is used, then it seems that this:

> +pixman_implementation_t *
> +_pixman_implementation_create_mips_dspase1 (pixman_implementation_t *delegate)
> +{
> +	if (delegate == NULL)
> +		delegate = _pixman_implementation_create_fast_path ();
> +
> +    pixman_implementation_t *imp =
> +		_pixman_implementation_create (delegate, mips_dspase1_fast_paths);
> +
> +	imp->combine_32[PIXMAN_OP_OVER] = mips_dspase1_combine_over_u;
> +
> +    return imp;
> +}
    
  doesn't need to check for NULL. (Also, please avoid variable
  declaration in the middle of code; that applies to the mips32r2
  version too).

- There are some style issues (see CODING_STYLE)

  - Use /* */ comments instead of //

  - Indents are four spaces

  - Put a space before parentheses

  - Don't leave in commented-out code like this:

     //              b = _pixman_implementation_fill(imp->delegate, bits, stride, bpp, x, y, width, height, xor);

I'm hoping someone with more experience with MIPS than I have can give
more detailed comments on the assembly.


Thanks,
Soren