[PATCH] Prefer mempcpy to memcpy on x86_64 target (PR middle-end/81657).

Martin Liška mliska@suse.cz
Wed Mar 14 14:04:00 GMT 2018


On 03/14/2018 02:07 PM, Jakub Jelinek wrote:
> 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?

Can you please explain me how to replace the 2 new arguments?

> 
>>    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?

I fixed that by preserving the old 2 old-finals and then I added a new for x86_64 target.
Apart from the comment above I've fixed all nits and mempcpy is not used when LHS == NULL.

Martin

> 
> 	Jakub
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Introduce-new-libc_func_speed-target-hook-PR-middle-.patch
Type: text/x-patch
Size: 11018 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180314/32425f28/attachment.bin>


More information about the Gcc-patches mailing list