Fix combine's simplify_shift_const_1 (PR rtl-optimization/70222)

On Tue, Mar 15, 2016 at 01:08:50PM +0100, Bernd Schmidt wrote:
> This looks really specialized, and I'd be worrying about whether it really
> is the right condition. Where exactly was the constant shifted by 31 and
> count set to 0? Must be here, right?

Yes, it is that spot.
> 
>    /* If we have (A << B << C) for any shift, we can convert this to
>       (A << C << B).  This wins if A is a constant.  Only try this if
>       B is not a constant.  */
> 
>    else if (GET_CODE (varop) == code
>             && CONST_INT_P (XEXP (varop, 0))
>             && !CONST_INT_P (XEXP (varop, 1)))
>     {
>       rtx new_rtx = simplify_const_binary_operation (code, mode,
>                                                     XEXP (varop, 0),
>                                                     GEN_INT (count));
>       varop = gen_rtx_fmt_ee (code, mode, new_rtx, XEXP (varop, 1));
>       count = 0;
>       continue;
>     }
> 
> I think it might be clearer to notice and fix the problem here (or set a
> need_mask flag).

So do you prefer this instead?

2016-03-15  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/70222
	* combine.c (simplify_shift_const_1): For A >> B >> C LSHIFTRT
	optimization if mode is different from result_mode, queue up masking
	of the result in outer_op.  Formatting fix.

	* gcc.c-torture/execute/pr70222-1.c: New test.
	* gcc.c-torture/execute/pr70222-2.c: New test.



	Jakub

On Tue, Mar 15, 2016 at 02:18:33PM +0100, Jakub Jelinek wrote:
> So do you prefer this instead?
> 
> 2016-03-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/70222
> 	* combine.c (simplify_shift_const_1): For A >> B >> C LSHIFTRT
> 	optimization if mode is different from result_mode, queue up masking
> 	of the result in outer_op.  Formatting fix.
> 
> 	* gcc.c-torture/execute/pr70222-1.c: New test.
> 	* gcc.c-torture/execute/pr70222-2.c: New test.

This one looks fine, too (if it works ;-) )


Segher


> --- gcc/combine.c.jj	2016-03-14 23:18:37.958408627 +0100
> +++ gcc/combine.c	2016-03-15 14:08:34.754434506 +0100
> @@ -10524,9 +10524,19 @@ simplify_shift_const_1 (enum rtx_code co
>  		   && CONST_INT_P (XEXP (varop, 0))
>  		   && !CONST_INT_P (XEXP (varop, 1)))
>  	    {
> +	      /* For ((unsigned) (cstULL >> count)) >> cst2 we have to make
> +		 sure the result will be masked.  See PR70222.  */
> +	      if (code == LSHIFTRT
> +		  && mode != result_mode
> +		  && !merge_outer_ops (&outer_op, &outer_const, AND,
> +				       GET_MODE_MASK (result_mode)
> +				       >> orig_count, result_mode,
> +				       &complement_p))
> +		break;
> +
>  	      rtx new_rtx = simplify_const_binary_operation (code, mode,
> -							 XEXP (varop, 0),
> -							 GEN_INT (count));
> +							     XEXP (varop, 0),
> +							     GEN_INT (count));
>  	      varop = gen_rtx_fmt_ee (code, mode, new_rtx, XEXP (varop, 1));
>  	      count = 0;
>  	      continue;
> --- gcc/testsuite/gcc.c-torture/execute/pr70222-1.c.jj	2016-03-15 11:30:41.657000384 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr70222-1.c	2016-03-15 11:30:41.657000384 +0100
> @@ -0,0 +1,30 @@
> +/* PR rtl-optimization/70222 */
> +
> +int a = 1;
> +unsigned int b = 2;
> +int c = 0;
> +int d = 0;
> +
> +void
> +foo ()
> +{
> +  int e = ((-(c >= c)) < b) > ((int) (-1ULL >> ((a / a) * 15)));
> +  d = -e;
> +}
> +
> +__attribute__((noinline, noclone)) void
> +bar (int x)
> +{
> +  if (x != -1)
> +    __builtin_abort ();
> +}
> +
> +int
> +main ()
> +{
> +#if __CHAR_BIT__ == 8 && __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8
> +  foo ();
> +  bar (d);
> +#endif
> +  return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr70222-2.c.jj	2016-03-15 11:36:13.273366841 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr70222-2.c	2016-03-15 11:36:18.156298614 +0100
> @@ -0,0 +1,20 @@
> +/* PR rtl-optimization/70222 */
> +
> +#if __CHAR_BIT__ == 8 && __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8
> +__attribute__((noinline, noclone)) unsigned int
> +foo (int x)
> +{
> +  unsigned long long y = -1ULL >> x;
> +  return (unsigned int) y >> 31;
> +}
> +#endif
> +
> +int
> +main ()
> +{
> +#if __CHAR_BIT__ == 8 && __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8
> +  if (foo (15) != 1 || foo (32) != 1 || foo (33) != 0)
> +    __builtin_abort ();
> +#endif
> +  return 0;
> +}

On Tue, Mar 15, 2016 at 09:50:44AM -0500, Segher Boessenkool wrote:
> On Tue, Mar 15, 2016 at 02:18:33PM +0100, Jakub Jelinek wrote:
> > So do you prefer this instead?
> > 
> > 2016-03-15  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/70222
> > 	* combine.c (simplify_shift_const_1): For A >> B >> C LSHIFTRT
> > 	optimization if mode is different from result_mode, queue up masking
> > 	of the result in outer_op.  Formatting fix.
> > 
> > 	* gcc.c-torture/execute/pr70222-1.c: New test.
> > 	* gcc.c-torture/execute/pr70222-2.c: New test.
> 
> This one looks fine, too (if it works ;-) )

Passed bootstrap/regtest on x86_64-linux and i686-linux, and triggers solely
on the new testcases, nothing else.  Committed now.

	Jakub