Hi Aaron,
On Thu, Dec 15, 2016 at 10:31:05AM -0600, Aaron Sawdey wrote:
> This patch adds a cmpstrnsi pattern for rs6000 target to provide
> builtin expansion of strncmp(). Perf tests on a power8 system show that
> it is 3-10x faster than the glibc strncmp on RHEL7 for lengths under 64
> bytes.
>
> Bootstrap/regtest has passed on powerpc64le, in progress for powerpc64,
> ok for trunk if no new regressions?
Some trivial things...
> +static void
> +expand_strncmp_align_check (rtx strncmp_label, rtx src, HOST_WIDE_INT bytes)
> +{
> + rtx lab_ref = gen_rtx_LABEL_REF (VOIDmode, strncmp_label);
> + rtx src_check = copy_addr_to_reg (XEXP (src, 0));
> + if (GET_MODE (src_check) == SImode)
> + emit_insn (gen_andsi3_mask (src_check, src_check, GEN_INT(0xfff)));
Space before (, also for macros.
> + else
> + emit_insn (gen_anddi3_mask (src_check, src_check, GEN_INT(0xfff)));
You can also just do gen_anddi3 (..); sometimes it can optimise better that
way, you can easier be sure you match all constraints, and it requires less
thinking. Just for expanders of course, code that runs later needs to do
more by hand to get good results. (There's no need to change it here, just
a FYI).
> + rtx cond = gen_reg_rtx (CCmode);
> + emit_move_insn (cond, gen_rtx_COMPARE (CCmode, src_check,
> + GEN_INT (4096-bytes)));
Spaces around -.
>
You put the page break in the middle of a function. Let's not :-)
> +/* Expand a string compare operation with length, and return
> + true if successful. Return false if we should let the
These two lines have trailing spaces. There is more of this; please
check everything.
> + if (align1 < 8)
> + expand_strncmp_align_check (strncmp_label, src1, bytes);
> + if (align2 < 8)
> + expand_strncmp_align_check (strncmp_label, src2, bytes);
Does this work out if (say) bytes >= 4096? It doesn't have to be very
sane code, but it should work correctly. Or you need to limit the maximum
size some other way.
> + /* Now generate the following sequence:
> + - branch to begin_compare
> + - strncmp_label
> + - call to strncmp
> + - branch to final_label
> + - begin_compare_label */
Wrong indent on that last line.
> + /* -m32 -mpowerpc64 results in word_mode being DImode even
> + though otherwise it is 32-bit. The length arg to strncmp
> + is a size_t which will be the same size as pointers. */
> + rtx len_rtx;
> + if (TARGET_64BIT)
> + len_rtx = gen_reg_rtx(DImode);
> + else
> + len_rtx = gen_reg_rtx(SImode);
> +
Wrong tab. Some more tab vs. spaces indent (and white space at the end
of line) follows, please fix all.
> + /* We must always left-align the data we read, and
> + clear any bytes to the right that are beyond the string.
> + Otherwise the cmpb sequence won't produce the correct
> + results. The beginning of the compare will be done
> + with word_mode so will not have any extra shifts or
> + clear rights. */
> +
> + if ( load_mode_size < word_mode_size )
No space after ( or before ).
> + rtx mask = GEN_INT (~(((HOST_WIDE_INT)1 << mb) - 1));
rtx mask = GEN_INT (HOST_WIDE_INT_M1U << mb);
Please repost. Thanks,
Segher
