[Pixman] [PATCH 02/14] ARMv6: Minor optimisation

[Siarhei Siamashka]

Hi Ben,

First a nitpick, which applies to some other patches too. "Minor
optimisation" is not a very good commit summary because it is way
too generic and does not say much about the patch itself.

The https://www.kernel.org/doc/Documentation/SubmittingPatches
guide from the Linux kernel provides a good rationale for having
descriptive commit summaries:

  Bear in mind that the "summary phrase" of your email becomes a
  globally-unique identifier for that patch.  It propagates all the way
  into the git changelog.  The "summary phrase" may later be used in
  developer discussions which refer to the patch.  People will want to
  google for the "summary phrase" to read discussion regarding that
  patch.  It will also be the only thing that people may quickly see
  when, two or three months later, they are going through perhaps
  thousands of patches using tools such as "gitk" or "git log
  --oneline".

  For these reasons, the "summary" must be no more than 70-75
  characters, and it must describe both what the patch changes, as well
  as why the patch might be necessary.  It is challenging to be both
  succinct and descriptive, but that is what a well-written summary
  should do.

Even taking "knock off one instruction per row" from the commit
message and moving it into the summary string would be a lot more
descriptive.

On Wed,  2 Oct 2013 00:00:22 +0100
Ben Avison <bavison at riscosopen.org> wrote:

> This knocks off one instruction per row. The effect is probably too small to
> be measurable, but might as well be included. The second occurrence of this
> sequence doesn't actually benefit at all, but is changed for consistency.
> ---
>  pixman/pixman-arm-simd-asm.h |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Simplification of the code and the reduction of the number of source
lines is good. Thanks.

> diff --git a/pixman/pixman-arm-simd-asm.h b/pixman/pixman-arm-simd-asm.h
> index 74400c1..3a2c250 100644
> --- a/pixman/pixman-arm-simd-asm.h
> +++ b/pixman/pixman-arm-simd-asm.h
> @@ -741,12 +741,9 @@ fname:
>          preload_leading_step1  mask_bpp, WK2, MASK
>          preload_leading_step1  dst_r_bpp, WK3, DST
>          
> -        tst     DST, #15
> +        ands    WK0, DST, #15
>          beq     154f
> -        rsb     WK0, DST, #0 /* bits 0-3 = number of leading bytes until destination aligned */
> -  .if (src_bpp != 0 && src_bpp != 2*dst_w_bpp) || (mask_bpp != 0 && mask_bpp != 2*dst_w_bpp)
> -        PF  and,    WK0, WK0, #15
> -  .endif
> +        rsb     WK0, WK0, #16 /* bits 0-3 = number of leading bytes until destination aligned */

Maybe it is better to also update the comment here? Because now it
applies not only to just bits 0-3, but to the whole WK0 register.

Also could you remind me about what was special about the removed .if
check and why it was there earlier?

>          preload_leading_step2  src_bpp, src_bpp_shift, WK1, SRC
>          preload_leading_step2  mask_bpp, mask_bpp_shift, WK2, MASK
> @@ -787,9 +784,9 @@ fname:
>          preload_line 0, dst_r_bpp, dst_bpp_shift, DST
>          
>          sub     X, X, #128/dst_w_bpp     /* simplifies inner loop termination */
> -        tst     DST, #15
> +        ands    WK0, DST, #15
>          beq     164f
> -        rsb     WK0, DST, #0 /* bits 0-3 = number of leading bytes until destination aligned */
> +        rsb     WK0, WK0, #16 /* bits 0-3 = number of leading bytes until destination aligned */
>          
>          leading_15bytes  process_head, process_tail


-- 
Best regards,
Siarhei Siamashka