[RS6000] num_insns_constant ICE

This patch came about from investigating an ICE that appeared when I
was retesting an old half-baked patch of mine to rs6000_rtx_costs.
If a const_double is fed to rs6000_is_valid_and_mask and from there to
rs6000_is_valid_mask where INTVAL is used, gcc will ICE.

The num_insns_constant ICE was introduced with git commit f337168d97.
However, the code was buggy before that.  There was no point in
testing for a mask since the mask predicates only handle const_int.
In fact, I don't think the function ever handled floating point
constants that might match a load of minus one and mask.  It does now.
I've added a few comments regarding splitters so the next person
looking at this code can see how this works.

The patch also extracts code out of num_insns_constant that needed to
handle multiple gprs for DFmode constants in 32-bit mode, to a
function that handles multiple gprs a little more generally.  I don't
think there is any need for anything but the 32-bit DFmode case
currently, but this allows for possible future uses.  The
CONST_WIDE_INT case is also not used currently, and needed fixing.
Adding CONST_WIDE_INT_NUNITS - 1 only makes sense if the elements of
the array were being shifted into a register of size larger than the
element size (which is 64-bits).

Boostrapped etc. powerpc64le-linux and powerpc64-linux.

	* config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from
	num_insns_constant_wide.  Make static.  Revise comment.
	(num_insns_constant_multi): New function.
	(num_insns_constant): Formatting.  Correct CONST_WIDE_INT
	calculation.  Simplify and extract code common to both
	CONST_INT and CONST_DOUBLE.  Add gcc_unreachable for unhandled
	const_double modes.
	* config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.

Hi!

On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote:
> This patch came about from investigating an ICE that appeared when I
> was retesting an old half-baked patch of mine to rs6000_rtx_costs.
> If a const_double is fed to rs6000_is_valid_and_mask and from there to
> rs6000_is_valid_mask where INTVAL is used, gcc will ICE.
> 
> The num_insns_constant ICE was introduced with git commit f337168d97.
> However, the code was buggy before that.  There was no point in
> testing for a mask since the mask predicates only handle const_int.
> In fact, I don't think the function ever handled floating point
> constants that might match a load of minus one and mask.  It does now.
> I've added a few comments regarding splitters so the next person
> looking at this code can see how this works.
> 
> The patch also extracts code out of num_insns_constant that needed to
> handle multiple gprs for DFmode constants in 32-bit mode, to a
> function that handles multiple gprs a little more generally.  I don't
> think there is any need for anything but the 32-bit DFmode case
> currently, but this allows for possible future uses.  The
> CONST_WIDE_INT case is also not used currently, and needed fixing.
> Adding CONST_WIDE_INT_NUNITS - 1 only makes sense if the elements of
> the array were being shifted into a register of size larger than the
> element size (which is 64-bits).
> 
> Boostrapped etc. powerpc64le-linux and powerpc64-linux.
> 
> 	* config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from
> 	num_insns_constant_wide.  Make static.  Revise comment.
> 	(num_insns_constant_multi): New function.
> 	(num_insns_constant): Formatting.  Correct CONST_WIDE_INT
> 	calculation.  Simplify and extract code common to both
> 	CONST_INT and CONST_DOUBLE.  Add gcc_unreachable for unhandled
> 	const_double modes.
> 	* config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.


> +/* Helper for num_insn_constant.  Allow constants formed by the
> +   num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm,
> +   and handle modes that require multiple gprs.  */
> +
> +static int
> +num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode)
> +{
> +  int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
> +  int total = 0;
> +  while (nregs-- > 0)
> +    {
> +      int ins = num_insns_constant_gpr (value);

You probably should mask "value" here so that it is only one GPR.
Alternatively, make num_insns_constant_gpr handle that.  Consider what
happens for a 64-bit number with 32-bit registers here?

Also, s/ins/insns/ please.

> +      if (ins > 2
> +	  /* We won't get more than 2 from num_insns_constant_gpr
> +	     except when TARGET_POWERPC64 and mode is DImode or
> +	     wider, so the register mode must be DImode.  */
> +	  && rs6000_is_valid_and_mask (GEN_INT (value), DImode))
> +	ins = 2;
> +      total += ins;

> +      value >>= 32;
> +      if (TARGET_POWERPC64)
> +	value >>= 32;

That's just

      value >>= BITS_PER_WORD;

> +	    long l[2];

> +	    val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32;

This doesn't work, as in the other patch: long can be 32 bit.

Looks good otherwise.


Segher

On Thu, Nov 29, 2018 at 01:52:34PM -0600, Segher Boessenkool wrote:
> On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote:
> > +  while (nregs-- > 0)
> > +    {
> > +      int ins = num_insns_constant_gpr (value);
> 
> You probably should mask "value" here so that it is only one GPR.
> Alternatively, make num_insns_constant_gpr handle that.  Consider what
> happens for a 64-bit number with 32-bit registers here?

Oh, right, the low part will always give an answer of 2 if the high
parts isn't merely a sign extension.

> > +	    val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32;
> 
> This doesn't work, as in the other patch: long can be 32 bit.

Huh, I originally had "high" and "low" HOST_WIDE_INTs which avoided
this sort of problem on 32-bit hosts.

Revised patch follows.  Bootstrapped and regression tested
powerpc64le-linux.  Hopefully this has removed all the stupidity.

	* config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from
	num_insns_constant_wide.  Make static.  Revise comment.
	(num_insns_constant_multi): New function.
	(num_insns_constant): Formatting.  Correct CONST_WIDE_INT
	calculation.  Simplify and extract code common to both
	CONST_INT and CONST_DOUBLE.  Add gcc_unreachable for unhandled
	const_double modes.
	* config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.

diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h
index 1dfe7995ff1..dfee1f28aa9 100644
--- a/gcc/config/rs6000/rs6000-protos.h
+++ b/gcc/config/rs6000/rs6000-protos.h
@@ -36,7 +36,6 @@ extern int vspltis_shifted (rtx);
 extern HOST_WIDE_INT const_vector_elt_as_int (rtx, unsigned int);
 extern bool macho_lo_sum_memory_operand (rtx, machine_mode);
 extern int num_insns_constant (rtx, machine_mode);
-extern int num_insns_constant_wide (HOST_WIDE_INT);
 extern int small_data_operand (rtx, machine_mode);
 extern bool mem_operand_gpr (rtx, machine_mode);
 extern bool mem_operand_ds_form (rtx, machine_mode);
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 02e69c103ec..c438fdc64fe 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5821,11 +5821,12 @@ direct_return (void)
   return 0;
 }
 
-/* Return the number of instructions it takes to form a constant in an
-   integer register.  */
+/* Helper for num_insns_constant.  Calculate number of instructions to
+   load VALUE to a single gpr using combinations of addi, addis, ori,
+   oris and sldi instructions.  */
 
-int
-num_insns_constant_wide (HOST_WIDE_INT value)
+static int
+num_insns_constant_gpr (HOST_WIDE_INT value)
 {
   /* signed constant loadable with addi */
   if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000)
@@ -5847,85 +5848,108 @@ num_insns_constant_wide (HOST_WIDE_INT value)
       high >>= 1;
 
       if (low == 0)
-	return num_insns_constant_wide (high) + 1;
+	return num_insns_constant_gpr (high) + 1;
       else if (high == 0)
-	return num_insns_constant_wide (low) + 1;
+	return num_insns_constant_gpr (low) + 1;
       else
-	return (num_insns_constant_wide (high)
-		+ num_insns_constant_wide (low) + 1);
+	return (num_insns_constant_gpr (high)
+		+ num_insns_constant_gpr (low) + 1);
     }
 
   else
     return 2;
 }
 
+/* Helper for num_insns_constant.  Allow constants formed by the
+   num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm,
+   and handle modes that require multiple gprs.  */
+
+static int
+num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode)
+{
+  int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
+  int total = 0;
+  while (nregs-- > 0)
+    {
+      HOST_WIDE_INT low = sext_hwi (value, BITS_PER_WORD);
+      int insns = num_insns_constant_gpr (low);
+      if (insns > 2
+	  /* We won't get more than 2 from num_insns_constant_gpr
+	     except when TARGET_POWERPC64 and mode is DImode or
+	     wider, so the register mode must be DImode.  */
+	  && rs6000_is_valid_and_mask (GEN_INT (low), DImode))
+	insns = 2;
+      total += insns;
+      value >>= BITS_PER_WORD;
+    }
+  return total;
+}
+
+/* Return the number of instructions it takes to form a constant in as
+   many gprs are needed for MODE.  */
+
 int
 num_insns_constant (rtx op, machine_mode mode)
 {
-  HOST_WIDE_INT low, high;
+  HOST_WIDE_INT val;
 
   switch (GET_CODE (op))
     {
     case CONST_INT:
-      if ((INTVAL (op) >> 31) != 0 && (INTVAL (op) >> 31) != -1
-	  && rs6000_is_valid_and_mask (op, mode))
-	return 2;
-      else
-	return num_insns_constant_wide (INTVAL (op));
+      val = INTVAL (op);
+      break;
 
     case CONST_WIDE_INT:
       {
-	int i;
-	int ins = CONST_WIDE_INT_NUNITS (op) - 1;
-	for (i = 0; i < CONST_WIDE_INT_NUNITS (op); i++)
-	  ins += num_insns_constant_wide (CONST_WIDE_INT_ELT (op, i));
-	return ins;
+	int insns = 0;
+	for (int i = 0; i < CONST_WIDE_INT_NUNITS (op); i++)
+	  insns += num_insns_constant_multi (CONST_WIDE_INT_ELT (op, i),
+					     DImode);
+	return insns;
       }
 
-      case CONST_DOUBLE:
+    case CONST_DOUBLE:
+      {
+	const struct real_value *rv = CONST_DOUBLE_REAL_VALUE (op);
+
 	if (mode == SFmode || mode == SDmode)
 	  {
 	    long l;
 
-	    if (DECIMAL_FLOAT_MODE_P (mode))
-	      REAL_VALUE_TO_TARGET_DECIMAL32
-		(*CONST_DOUBLE_REAL_VALUE (op), l);
+	    if (mode == SDmode)
+	      REAL_VALUE_TO_TARGET_DECIMAL32 (*rv, l);
 	    else
-	      REAL_VALUE_TO_TARGET_SINGLE (*CONST_DOUBLE_REAL_VALUE (op), l);
-	    return num_insns_constant_wide ((HOST_WIDE_INT) l);
+	      REAL_VALUE_TO_TARGET_SINGLE (*rv, l);
+	    /* See the first define_split in rs6000.md handling a
+	       const_double_operand.  */
+	    val = l;
+	    mode = SImode;
 	  }
-
-	long l[2];
-	if (DECIMAL_FLOAT_MODE_P (mode))
-	  REAL_VALUE_TO_TARGET_DECIMAL64 (*CONST_DOUBLE_REAL_VALUE (op), l);
-	else
-	  REAL_VALUE_TO_TARGET_DOUBLE (*CONST_DOUBLE_REAL_VALUE (op), l);
-	high = l[WORDS_BIG_ENDIAN == 0];
-	low  = l[WORDS_BIG_ENDIAN != 0];
-
-	if (TARGET_32BIT)
-	  return (num_insns_constant_wide (low)
-		  + num_insns_constant_wide (high));
-	else
+	else if (mode == DFmode || mode == DDmode)
 	  {
-	    if ((high == 0 && low >= 0)
-		|| (high == -1 && low < 0))
-	      return num_insns_constant_wide (low);
-
-	    else if (rs6000_is_valid_and_mask (op, mode))
-	      return 2;
-
-	    else if (low == 0)
-	      return num_insns_constant_wide (high) + 1;
+	    long l[2];
 
+	    if (mode == DDmode)
+	      REAL_VALUE_TO_TARGET_DECIMAL64 (*rv, l);
 	    else
-	      return (num_insns_constant_wide (high)
-		      + num_insns_constant_wide (low) + 1);
+	      REAL_VALUE_TO_TARGET_DOUBLE (*rv, l);
+
+	    /* See the second (32-bit) and third (64-bit) define_split
+	       in rs6000.md handling a const_double_operand.  */
+	    val = (unsigned HOST_WIDE_INT) l[WORDS_BIG_ENDIAN ? 0 : 1] << 32;
+	    val |= l[WORDS_BIG_ENDIAN ? 1 : 0] & 0xffffffffUL;
+	    mode = DImode;
 	  }
+	else
+	  gcc_unreachable ();
+      }
+      break;
 
     default:
       gcc_unreachable ();
     }
+
+  return num_insns_constant_multi (val, mode);
 }
 
 /* Interpret element ELT of the CONST_VECTOR OP as an integer value.

On Fri, Nov 30, 2018 at 06:16:40PM +1030, Alan Modra wrote:
> On Thu, Nov 29, 2018 at 01:52:34PM -0600, Segher Boessenkool wrote:
> > On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote:
> > > +  while (nregs-- > 0)
> > > +    {
> > > +      int ins = num_insns_constant_gpr (value);
> > 
> > You probably should mask "value" here so that it is only one GPR.
> > Alternatively, make num_insns_constant_gpr handle that.  Consider what
> > happens for a 64-bit number with 32-bit registers here?
> 
> Oh, right, the low part will always give an answer of 2 if the high
> parts isn't merely a sign extension.
> 
> > > +	    val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32;
> > 
> > This doesn't work, as in the other patch: long can be 32 bit.
> 
> Huh, I originally had "high" and "low" HOST_WIDE_INTs which avoided
> this sort of problem on 32-bit hosts.
> 
> Revised patch follows.  Bootstrapped and regression tested
> powerpc64le-linux.  Hopefully this has removed all the stupidity.

I think it is fine like this, or certainly an improvement :-)  Okay for
trunk.  Thanks!


Segher


> 	* config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from
> 	num_insns_constant_wide.  Make static.  Revise comment.
> 	(num_insns_constant_multi): New function.
> 	(num_insns_constant): Formatting.  Correct CONST_WIDE_INT
> 	calculation.  Simplify and extract code common to both
> 	CONST_INT and CONST_DOUBLE.  Add gcc_unreachable for unhandled
> 	const_double modes.
> 	* config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.