On Wed, Mar 14, 2018 at 01:54:39PM +0100, Martin Liška wrote:
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -3651,13 +3651,24 @@ expand_builtin_memory_copy_args (tree dest, tree src,
> tree len,
> src_mem = get_memory_rtx (src, len);
> set_mem_align (src_mem, src_align);
>
> + bool is_move_done;
> +
> /* Copy word part most expediently. */
This comment supposedly belongs right above the emit_block_move_hints call.
> + bool bail_out_libcall = endp == 1
> + && targetm.libc_func_speed ((int)BUILT_IN_MEMPCPY) == FAST_SPEED;
Formatting. && belongs below endp. So either:
bool bail_out_libcall
= (endp == 1
&& ...);
or
bool bail_out_libcall = false;
if (endp == 1
&& ...)
bail_out_libcall = true;
?
The variable is not named very well, shouldn't it be avoid_libcall or
something similar? Perhaps the variable should have a comment describing
what it is. Do you need separate argument for that bool and
is_move_done, rather than just the flag being that some pointer to bool is
non-NULL?
> dest_addr = emit_block_move_hints (dest_mem, src_mem, len_rtx,
> CALL_EXPR_TAILCALL (exp)
> && (endp == 0 || target == const0_rtx)
> ? BLOCK_OP_TAILCALL : BLOCK_OP_NORMAL,
> expected_align, expected_size,
> - min_size, max_size, probable_max_size);
> + min_size, max_size, probable_max_size,
> + bail_out_libcall, &is_move_done);
> +
> + /* Bail out when a mempcpy call would be expanded as libcall and when
> + we have a target that provides a fast implementation
> + of mempcpy routine. */
> + if (!is_move_done)
> + return NULL_RTX;
>
> if (dest_addr == 0)
> {
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -2733,6 +2733,23 @@ ix86_using_red_zone (void)
> && (!cfun->machine->has_local_indirect_jump
> || cfun->machine->indirect_branch_type == indirect_branch_keep));
> }
> +
Missing function comment here. For target hooks, usually there is a copy of
the target.def comment, perhaps with further details on why it is overridden
and what it does.
> --- a/gcc/coretypes.h
> +++ b/gcc/coretypes.h
> @@ -384,6 +384,13 @@ enum excess_precision_type
> EXCESS_PRECISION_TYPE_FAST
> };
>
Missing comment describing what it is, plus it the enumerators are too
generic, if it is libc_speed enum, perhaps LIBC_FAST_SPEED etc.?
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1642,6 +1642,12 @@ no_c99_libc_has_function (enum function_class fn_class
> ATTRIBUTE_UNUSED)
> return false;
> }
>
Again, missing function comment.
> +enum libc_speed
> +default_libc_func_speed (int)
> +{
> + return UNKNOWN_SPEED;
> +}
> +
> --- a/gcc/testsuite/gcc.dg/string-opt-1.c
> +++ b/gcc/testsuite/gcc.dg/string-opt-1.c
> @@ -48,5 +48,5 @@ main (void)
> return 0;
> }
>
> -/* { dg-final { scan-assembler-not "\<mempcpy\>" } } */
> -/* { dg-final { scan-assembler "memcpy" } } */
> +/* { dg-final { scan-assembler-not "\<mempcpy\>" { target { i?86-*-*
> x86_64-*-* } } } } */
> +/* { dg-final { scan-assembler "memcpy" { target { ! { i?86-*-* x86_64-*-*
> } } } } } */
First of all, I don't really understand this, I'd expect both the
two old dg-final lines to be used as is for non-x86 targets and another two
for x86_64, and more importantly, the target hook is only for glibc, not for
musl/bionic etc., nor for non-linux, so you probably need some effective
target for it for whether it is glibc rather than musl/bionic, and use
...-*-linux* and ...-*-gnu* etc. rather than ...-*-*. Or tweak the dg-fianl
patterns so that it succeeds on both variants of the target hook, but then
does the test test anything at all?
Jakub
