Fix gen_lowpart_if_possible (PR middle-end/85414)

Hi!

The following testcase FAILs, because cse_local sees
(zero_extend:TI (subreg/s/v:DI (reg:TI ...) 0))
inside of REG_EQUAL note, and simplify-rtx.c attempts to optimize it.
    case ZERO_EXTEND:
      /* Check for a zero extension of a subreg of a promoted
         variable, where the promotion is zero-extended, and the
         target mode is the same as the variable's promotion.  */
      if (GET_CODE (op) == SUBREG
          && SUBREG_PROMOTED_VAR_P (op)
          && SUBREG_PROMOTED_UNSIGNED_P (op)
          && !paradoxical_subreg_p (mode, GET_MODE (SUBREG_REG (op))))
        {
          temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
          if (temp)
            return temp;
        }
This calls gen_lowpart_common which fails, because:
      /* MODE must occupy no more of the underlying registers than X.  */
      poly_uint64 regsize = REGMODE_NATURAL_SIZE (innermode);
      unsigned int mregs, xregs;
      if (!can_div_away_from_zero_p (msize, regsize, &mregs)
          || !can_div_away_from_zero_p (xsize, regsize, &xregs)
          || mregs > xregs)
        return 0;
and so gen_lowpart_if_possible emits (subreg:TI (subreg/s/v:DI (reg:TI ...) 0) 0)
which is invalid, unsurprisingly other passes have issues with it and DF in
the middle of reload discovers use of an unsaved call saved register that
way.

The following patch fixes it by never creating a subreg of subreg, other
spots avoid it similarly.  Bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?

Perhaps in GCC9 we might consider relaxing the above gen_lowpart_common
mregs/xregs restriction, say if the inner operand is a lowpart subreg of
a register with mode having at least xregs regs, allow it; not really sure
if it is desirable though.

2018-04-16  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/85414
	* rtlhooks.c (gen_lowpart_if_possible): Don't call gen_lowpart_SUBREG
	on a SUBREG.

	* gcc.dg/pr85414.c: New test.


	Jakub

> The following testcase FAILs, because cse_local sees
> (zero_extend:TI (subreg/s/v:DI (reg:TI ...) 0))
> inside of REG_EQUAL note, and simplify-rtx.c attempts to optimize it.
>     case ZERO_EXTEND:
>       /* Check for a zero extension of a subreg of a promoted
>          variable, where the promotion is zero-extended, and the
>          target mode is the same as the variable's promotion.  */
>       if (GET_CODE (op) == SUBREG
>           && SUBREG_PROMOTED_VAR_P (op)
>           && SUBREG_PROMOTED_UNSIGNED_P (op)
>           && !paradoxical_subreg_p (mode, GET_MODE (SUBREG_REG (op))))
>         {
>           temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
>           if (temp)
>             return temp;
>         }

This code is strange though, here's the equivalent one in convert_modes:

  if (GET_CODE (x) == SUBREG
      && SUBREG_PROMOTED_VAR_P (x)
      && is_a <scalar_int_mode> (mode, &int_mode)
      && (GET_MODE_PRECISION (subreg_promoted_mode (x))
	  >= GET_MODE_PRECISION (int_mode))
      && SUBREG_CHECK_PROMOTED_SIGN (x, unsignedp))
    x = gen_lowpart (int_mode, SUBREG_REG (x));

so can't we just pass SUBREG_REG (op) here?  Same for SIGN_EXTEND above.

On Tue, Apr 17, 2018 at 09:32:31AM +0200, Eric Botcazou wrote:
> > The following testcase FAILs, because cse_local sees
> > (zero_extend:TI (subreg/s/v:DI (reg:TI ...) 0))
> > inside of REG_EQUAL note, and simplify-rtx.c attempts to optimize it.
> >     case ZERO_EXTEND:
> >       /* Check for a zero extension of a subreg of a promoted
> >          variable, where the promotion is zero-extended, and the
> >          target mode is the same as the variable's promotion.  */
> >       if (GET_CODE (op) == SUBREG
> >           && SUBREG_PROMOTED_VAR_P (op)
> >           && SUBREG_PROMOTED_UNSIGNED_P (op)
> >           && !paradoxical_subreg_p (mode, GET_MODE (SUBREG_REG (op))))
> >         {
> >           temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> >           if (temp)
> >             return temp;
> >         }
> 
> This code is strange though, here's the equivalent one in convert_modes:
> 
>   if (GET_CODE (x) == SUBREG
>       && SUBREG_PROMOTED_VAR_P (x)
>       && is_a <scalar_int_mode> (mode, &int_mode)
>       && (GET_MODE_PRECISION (subreg_promoted_mode (x))
> 	  >= GET_MODE_PRECISION (int_mode))
>       && SUBREG_CHECK_PROMOTED_SIGN (x, unsignedp))
>     x = gen_lowpart (int_mode, SUBREG_REG (x));

Yes, convert_modes does this, on the other side e.g. convert_move a few
hundred lines above it doesn't:
  if (GET_CODE (from) == SUBREG
      && SUBREG_PROMOTED_VAR_P (from)
      && is_a <scalar_int_mode> (to_mode, &to_int_mode)
      && (GET_MODE_PRECISION (subreg_promoted_mode (from))
          >= GET_MODE_PRECISION (to_int_mode))
      && SUBREG_CHECK_PROMOTED_SIGN (from, unsignedp))
    from = gen_lowpart (to_int_mode, from), from_mode = to_int_mode;

> so can't we just pass SUBREG_REG (op) here?  Same for SIGN_EXTEND above.

The following patch works for the testcase too (even with the rtlhooks.c
hunk removed, but I think we want to keep it anyway, even when we lose the
only known testcase where it matters).  We generate the same assembly,
though it behaves differently.  With the previous patch we have:
(zero_extend:TI (subreg/s/v:DI (reg:TI 94) 0))
in the REG_EQUAL note, then subreg2 makes (zero_extend:TI (reg:DI 146))
out of it and in the end nothing uses the REG_EQUAL note.
With this patch, we have instead (reg:TI 94) in the REG_EQUAL note and
subreg2 pass drops the REG_EQUAL note, so nothing can use it even if it
wanted.

Anyway, if this is what is preferred, ok for trunk if it passes
bootstrap/regtest?

2018-04-17  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/85414
	* simplify-rtx.c (simplify_unary_operation_1) <case SIGN_EXTEND,
	case ZERO_EXTEND>: Pass SUBREG_REG (op) rather than op to
	gen_lowpart_no_emit.
	* rtlhooks.c (gen_lowpart_if_possible): Don't call gen_lowpart_SUBREG
	on a SUBREG.

	* gcc.dg/pr85414.c: New test.

--- gcc/simplify-rtx.c.jj	2018-04-13 21:38:39.924432794 +0200
+++ gcc/simplify-rtx.c	2018-04-17 10:37:57.559193098 +0200
@@ -1484,7 +1484,7 @@ simplify_unary_operation_1 (enum rtx_cod
 	  && SUBREG_PROMOTED_SIGNED_P (op)
 	  && !paradoxical_subreg_p (mode, GET_MODE (SUBREG_REG (op))))
 	{
-	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
+	  temp = rtl_hooks.gen_lowpart_no_emit (mode, SUBREG_REG (op));
 	  if (temp)
 	    return temp;
 	}
@@ -1567,7 +1567,7 @@ simplify_unary_operation_1 (enum rtx_cod
 	  && SUBREG_PROMOTED_UNSIGNED_P (op)
 	  && !paradoxical_subreg_p (mode, GET_MODE (SUBREG_REG (op))))
 	{
-	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
+	  temp = rtl_hooks.gen_lowpart_no_emit (mode, SUBREG_REG (op));
 	  if (temp)
 	    return temp;
 	}
--- gcc/rtlhooks.c.jj	2018-04-16 18:11:54.791378162 +0200
+++ gcc/rtlhooks.c	2018-04-17 10:39:43.263246431 +0200
@@ -123,9 +123,9 @@ gen_lowpart_if_possible (machine_mode mo
 
       return new_rtx;
     }
-  else if (mode != GET_MODE (x) && GET_MODE (x) != VOIDmode
+  else if (mode != GET_MODE (x) && GET_MODE (x) != VOIDmode && !SUBREG_P (x)
 	   && validate_subreg (mode, GET_MODE (x), x,
-			        subreg_lowpart_offset (mode, GET_MODE (x))))
+			       subreg_lowpart_offset (mode, GET_MODE (x))))
     return gen_lowpart_SUBREG (mode, x);
   else
     return 0;
--- gcc/testsuite/gcc.dg/pr85414.c.jj	2018-04-17 10:21:55.531391008 +0200
+++ gcc/testsuite/gcc.dg/pr85414.c	2018-04-17 10:21:55.531391008 +0200
@@ -0,0 +1,10 @@
+/* PR middle-end/85414 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-Og -fgcse -Wno-uninitialized" } */
+
+int
+foo (void)
+{
+  unsigned __int128 c;
+  return __builtin_mul_overflow_p (59, -c, (short) 0);
+}


	Jakub

> Yes, convert_modes does this, on the other side e.g. convert_move a few
> hundred lines above it doesn't:
>   if (GET_CODE (from) == SUBREG
>       && SUBREG_PROMOTED_VAR_P (from)
>       && is_a <scalar_int_mode> (to_mode, &to_int_mode)
>       && (GET_MODE_PRECISION (subreg_promoted_mode (from))
> 
>           >= GET_MODE_PRECISION (to_int_mode))
> 
>       && SUBREG_CHECK_PROMOTED_SIGN (from, unsignedp))
>     from = gen_lowpart (to_int_mode, from), from_mode = to_int_mode;

Right, they were originally alike, but someone you know very well changed it:

2013-12-17  Jakub Jelinek  <jakub@redhat.com>

	* expr.c (convert_modes): For SUBREG_PROMOTED_VAR_P use SUBREG_REG (x)
	instead of x as last gen_lowpart argument.

> 2018-04-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/85414
> 	* simplify-rtx.c (simplify_unary_operation_1) <case SIGN_EXTEND,
> 	case ZERO_EXTEND>: Pass SUBREG_REG (op) rather than op to
> 	gen_lowpart_no_emit.
> 	* rtlhooks.c (gen_lowpart_if_possible): Don't call gen_lowpart_SUBREG
> 	on a SUBREG.
> 
> 	* gcc.dg/pr85414.c: New test.

I'd say, either we change both convert_move and simplify-rtx.c for GCC 8 or we 
change none of them.  You're the RM so it's more of your call than mine.

On Tue, Apr 17, 2018 at 11:00:35AM +0200, Eric Botcazou wrote:
> Right, they were originally alike, but someone you know very well changed it:
> 
> 2013-12-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* expr.c (convert_modes): For SUBREG_PROMOTED_VAR_P use SUBREG_REG (x)
> 	instead of x as last gen_lowpart argument.

Heh, https://gcc.gnu.org/ml/gcc-patches/2013-12/msg01455.html

> 
> > 2018-04-17  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR middle-end/85414
> > 	* simplify-rtx.c (simplify_unary_operation_1) <case SIGN_EXTEND,
> > 	case ZERO_EXTEND>: Pass SUBREG_REG (op) rather than op to
> > 	gen_lowpart_no_emit.
> > 	* rtlhooks.c (gen_lowpart_if_possible): Don't call gen_lowpart_SUBREG
> > 	on a SUBREG.
> > 
> > 	* gcc.dg/pr85414.c: New test.
> 
> I'd say, either we change both convert_move and simplify-rtx.c for GCC 8 or we 
> change none of them.  You're the RM so it's more of your call than mine.

Ok, I'll commit the first patch then and change all 3 spots (expr.c and 2x
simplify-rtx.c) for GCC 9 then.

	Jakub