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 <[email protected]> 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 _______________________________________________ Pixman mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pixman
