Re: [PATCH, bugfix] builtin expansion of strcmp for rs6000

[Segher Boessenkool <segher at kernel dot crashing dot org>]

On Mon, Jan 16, 2017 at 03:09:35PM -0600, Aaron Sawdey wrote:
> Tulio noted that glibc's strncmp test was failing. This turned out to
> be the use of signed HOST_WIDE_INT for handling strncmp length. The
> glibc test calls strncmp with length 2^64-1, presumably to provoke
> exactly this type of bug. Fixing the issue required changing
> select_block_compare_mode() and expand_block_compare() as well.
> 
> The other change is if we must emit a runtime check for the 4k
> crossing, then we might as well set base_align to 8 and emit the best
> possible code.

Some nits...

> --- gcc/config/rs6000/rs6000.c	(revision 244503)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -19310,7 +19310,8 @@
>     WORD_MODE_OK indicates using WORD_MODE is allowed, else SImode is
>     the largest allowable mode.  */
>  static machine_mode
> -select_block_compare_mode (HOST_WIDE_INT offset, HOST_WIDE_INT bytes,
> +select_block_compare_mode (unsigned HOST_WIDE_INT offset,
> +			   unsigned HOST_WIDE_INT bytes,
>  			   HOST_WIDE_INT align, bool word_mode_ok)

"align" should probably be unsigned as well?

> @@ -19410,8 +19411,8 @@
>    gcc_assert (GET_MODE (target) == SImode);
>  
>    /* Anything to move? */
> -  HOST_WIDE_INT bytes = INTVAL (bytes_rtx);
> -  if (bytes <= 0)
> +  unsigned HOST_WIDE_INT bytes = INTVAL (bytes_rtx);
> +  if (bytes == 0)
>      return true;

UINTVAL?  Please check the rest of the patch for this, too.

>    /* We don't want to generate too much code.  */
>    if (ROUND_UP (bytes, load_mode_size) / load_mode_size
> -      > rs6000_block_compare_inline_limit)
> +      > (unsigned HOST_WIDE_INT)rs6000_block_compare_inline_limit)
>      return false;

Space after cast operator.  Why do you need a cast at all?  It already
is unsigned.

> +	  /* -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);

Space before opening paren in function calls.

> +  while (bytes_to_compare > 0)
>      {
>        /* Compare sequence:
>           check each 8B with: ld/ld cmpd bne
> +	 If equal, use rldicr/cmpb to check for zero byte.
>           cleanup code at end:
>           cmpb          get byte that differs
>           cmpb          look for zero byte

Mixed spaces/tabs indent.

> @@ -19919,37 +19968,45 @@
>  	    }
>  	}
>  
> -      int remain = bytes - cmp_bytes;
> +      /* Cases to handle.  A and B are chunks of the two strings.
> +         1: Not end of comparison:
> +	   A != B: branch to cleanup code to compute result.
> +           A == B: check for 0 byte, next block if not found.
> +         2: End of the inline comparison:
> +	   A != B: branch to cleanup code to compute result.
> +           A == B: check for 0 byte, call strcmp/strncmp
> +	 3: compared requested N bytes:
> +	   A == B: branch to result 0.
> +           A != B: cleanup code to compute result.  */

And here.

> @@ -19957,10 +20014,102 @@
>        JUMP_LABEL (j) = dst_label;
>        LABEL_NUSES (dst_label) += 1;
>  
> +      if (remain > 0 || equality_compare_rest)
> +	{
> +	  /* Generate a cmpb to test for a 0 byte and branch
> +	     to final result if found.  */
> +	  rtx cmpb_zero = gen_reg_rtx (word_mode);
> +	  rtx lab_ref_fin = gen_rtx_LABEL_REF (VOIDmode, final_move_label);
> +	  rtx condz = gen_reg_rtx (CCmode);
> +	  rtx zero_reg = gen_reg_rtx (word_mode);
> +	  if (word_mode == SImode)
> +	    {
> +	      emit_insn (gen_movsi (zero_reg, GEN_INT(0)));

Space before (0).

> +	      emit_insn (gen_cmpbsi3 (cmpb_zero, tmp_reg_src1, zero_reg));
> +	      if ( cmp_bytes < word_mode_size )

No spaces inside the parens.

> +		{ /* Don't want to look at zero bytes past end.  */

Put the comment on a separate line please.

> +	      emit_insn (gen_movdi (zero_reg, GEN_INT(0)));
> +	      emit_insn (gen_cmpbdi3 (cmpb_zero, tmp_reg_src1, zero_reg));
> +	      if ( cmp_bytes < word_mode_size )
> +		{ /* Don't want to look at zero bytes past end.  */

Again and again and again :-)

Looks good otherwise.


Segher