Re: [PATCH] Fix combiner issues with shifts (PR c/37924)

[Michael Meissner <meissner at linux dot vnet dot ibm dot com>]

On Mon, Oct 27, 2008 at 09:02:07PM +0100, Jakub Jelinek wrote:
> Hi!
> 
> This patch fixes a couple of combiner issues.
> 1) simplify_shift_const_1 does something only if a shift count is within
>    its range, but then can add or subtract shift counts without checking the
>    result.  This is because each such addition or subtraction is followed by
>    continue and expects next iteration to canonicalize the count. 
>    Unfortunately if complement_p is true, the loop is exited before that
>    canonicalization.  This patch moves if (complement_p) break; after
>    the shift count canonicalization.
> 2) make_compound_operation in several places didn't check if shift count is
>    in its range, and calling make_extraction with negative length results in
>    ICE.  We should leave out-of-range shift counts unmodified.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

The code looks correct to me.  However, your test code should declare a to be
signed char instead of char, since there are platforms where chars are unsigned
by default.
 
> 2008-10-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/37924
> 	* combine.c (make_compound_operation): Don't call make_extraction with
> 	non-positive length.
> 	(simplify_shift_const_1): Canonicalize count even if complement_p.
> 
> 	* gcc.c-torture/execute/pr37924.c: New test.
> 
> --- gcc/combine.c.jj	2008-10-23 13:21:41.000000000 +0200
> +++ gcc/combine.c	2008-10-27 17:05:29.000000000 +0100
> @@ -6996,7 +6996,8 @@ make_compound_operation (rtx x, enum rtx
>        if (GET_CODE (rhs) == CONST_INT
>  	  && GET_CODE (lhs) == ASHIFT
>  	  && GET_CODE (XEXP (lhs, 1)) == CONST_INT
> -	  && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1)))
> +	  && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1))
> +	  && INTVAL (rhs) < mode_width)
>  	{
>  	  new_rtx = make_compound_operation (XEXP (lhs, 0), next_code);
>  	  new_rtx = make_extraction (mode, new_rtx,
> @@ -7016,6 +7017,7 @@ make_compound_operation (rtx x, enum rtx
>  		&& (OBJECT_P (SUBREG_REG (lhs))))
>  	  && GET_CODE (rhs) == CONST_INT
>  	  && INTVAL (rhs) < HOST_BITS_PER_WIDE_INT
> +	  && INTVAL (rhs) < mode_width
>  	  && (new_rtx = extract_left_shift (lhs, INTVAL (rhs))) != 0)
>  	new_rtx = make_extraction (mode, make_compound_operation (new_rtx, next_code),
>  			       0, NULL_RTX, mode_width - INTVAL (rhs),
> @@ -9003,11 +9005,6 @@ simplify_shift_const_1 (enum rtx_code co
>        if (GET_CODE (varop) == CLOBBER)
>  	return NULL_RTX;
> 
> -      /* If we discovered we had to complement VAROP, leave.  Making a NOT
> -	 here would cause an infinite loop.  */
> -      if (complement_p)
> -	break;
> -
>        /* Convert ROTATERT to ROTATE.  */
>        if (code == ROTATERT)
>  	{
> @@ -9053,6 +9050,11 @@ simplify_shift_const_1 (enum rtx_code co
>  	    }
>  	}
> 
> +      /* If we discovered we had to complement VAROP, leave.  Making a NOT
> +	 here would cause an infinite loop.  */
> +      if (complement_p)
> +	break;
> +
>        /* An arithmetic right shift of a quantity known to be -1 or 0
>  	 is a no-op.  */
>        if (code == ASHIFTRT
> --- gcc/testsuite/gcc.c-torture/execute/pr37924.c.jj	2008-10-27 19:24:11.000000000 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr37924.c	2008-10-27 19:23:10.000000000 +0100
> @@ -0,0 +1,29 @@
> +/* PR c/37924 */
> +
> +extern void abort (void);
> +
> +char a;
> +
> +int
> +test (void)
> +{
> +  int b = -1;
> +  return ((unsigned int) (a ^ b)) >> 9;
> +}
> +
> +int
> +main (void)
> +{
> +  if (test () != (-1U >> 9))
> +    abort ();
> +  a = 0x40;
> +  if (test () != (-1U >> 9))
> +    abort ();
> +  a = 0x80;
> +  if (test () != (a < 0) ? 0 : (-1U >> 9))
> +    abort ();
> +  a = 0xff;
> +  if (test () != (a < 0) ? 0 : (-1U >> 9))
> +    abort ();
> +  return 0;
> +}
> 
> 	Jakub

-- 
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com