diff mbox series

[RS6000] num_insns_constant ICE

Message ID 20181120073740.GG21617@bubble.grove.modra.org
State New
Headers show
Series [RS6000] num_insns_constant ICE | expand

Commit Message

Alan Modra Nov. 20, 2018, 7:37 a.m. UTC 
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.
The num_insns_constant ICE was introduced with git commit f337168d97.
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.

However, the code was buggy before that.  When const_double no longer
was used for integers 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.

I also looked at all places (*) where num_insns_constant is called.
That lead to finding two cases where the "G" and "H" constraints were
used incorrectly.  Their purpose is calculating insn lengths.  Thus it
never makes sense to put "GH" together or with "F" in an insn
alternative.  movdi_internal32 used "GHF" in an alternative (and
always selected 64 byte insn length) so I replaced that with "F".
The FMOVE128 version of mov<mode>_softfloat also had "GHF" in an
alternative but from what I see in rs6000_emit_move, I believe TFmode
insns loading constants will be splt.  So this insn doesn't need to
handle FP constants (it also doesn't need an iterator as only TFmode
will be selected - not fixed with this patch).

*) Besides the G and H constraint use of num_insns_constant, there
are three uses of num_insns_constant in rs6000.md each only dealing
with const_int or const_wide_int operands.  The rs6000.c call in
rs6000_emit_move only deals with const_int, so no const_double code is
touched.  easy_fp_constant in predicates.md does appear to need a
num_insns_constant that handles const_double in DImode.
easy_fp_constant is called from easy_vector_constant, input_operand
and vector.md mov pattern but not with a DImode operand.  In rs6000.c,
the first call in rs6000_emit_move is also not DImode.  The second
call might indeed match, so there's one possible use.  The
easy_fp_constant call in rs6000_legitimate_constant_p also might call
num_insns_constant, but only in 32-bit mode.

Bootstrapped and regression tested powerpc64le-linux and
powerpc64-linux.  I'm running the powerpc64-linux tests again as I
forgot to test -m32 on the first run.  OK for trunk, assuming the -m32
tests don't regress?

	* config/rs6000/rs6000.c (num_insns_constant_wide): Make static.
	(num_insns_constant <CONST_DOUBLE>): Don't ICE on call to
	rs6000_is_valid_and_mask.  Formatting.  Simplify and extract
	code common to both CONST_INT and CONST_DOUBLE.  Handle
	floating point constants in SImode/DImode.  Add gcc_unreachable
	for unhandled const_double modes.
	* config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.
	* config/rs6000/rs6000.md (movdi_internal32): Remove "GH" from
	alternative handling "F".
	(mov<mode>_softfloat <FMOVE128>): Delete "GHF" from alternative.
	* config/rs6000/contraints.md (G, H): Comment.

Comments

Alan Modra Nov. 22, 2018, 1:08 p.m. UTC | #1 
On Tue, Nov 20, 2018 at 06:07:41PM +1030, Alan Modra wrote:
[snip]
> alternative.  movdi_internal32 used "GHF" in an alternative (and
> always selected 64 byte insn length) so I replaced that with "F".

Huh, no, the insn didn't have length attributes.

> The FMOVE128 version of mov<mode>_softfloat also had "GHF" in an
> alternative but from what I see in rs6000_emit_move, I believe TFmode
> insns loading constants will be splt.

This isn't true either.  The split happens depending on altivec, and
in fact when the split occurs we get poor code.

>  So this insn doesn't need to
> handle FP constants (it also doesn't need an iterator as only TFmode
> will be selected - not fixed with this patch).

Which means this is only half true.  By removing "F", I effectively
forced all soft-float long double constants to memory.

The patch has been revised and broken into a number of pieces.  I'll
post tomorrow.
diff mbox series

Patch

diff --git a/gcc/config/rs6000/constraints.md b/gcc/config/rs6000/constraints.md
index 90aba1e77d3..21c7a808919 100644
--- a/gcc/config/rs6000/constraints.md
+++ b/gcc/config/rs6000/constraints.md
@@ -252,7 +252,8 @@  (define_constraint "P"
   (and (match_code "const_int")
        (match_test "((- (unsigned HOST_WIDE_INT) ival) + 0x8000) < 0x10000")))
 
-;; Floating-point constraints
+;; Floating-point constraints.  These two are defined so that insn
+;; length attributes can be calculated exactly.
 
 (define_constraint "G"
   "Constant that can be copied into GPR with two insns for DF/DI
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 479dae547fa..1eabf9cd612 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5818,7 +5818,7 @@  direct_return (void)
 /* Return the number of instructions it takes to form a constant in an
    integer register.  */
 
-int
+static int
 num_insns_constant_wide (HOST_WIDE_INT value)
 {
   /* signed constant loadable with addi */
@@ -5856,16 +5856,13 @@  num_insns_constant_wide (HOST_WIDE_INT value)
 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:
       {
@@ -5876,50 +5873,62 @@  num_insns_constant (rtx op, machine_mode mode)
 	return ins;
       }
 
-      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);
+
+	    HOST_WIDE_INT high = l[WORDS_BIG_ENDIAN == 0];
+	    HOST_WIDE_INT low  = l[WORDS_BIG_ENDIAN != 0];
+
+	    if (TARGET_32BIT)
+	      /* We'll be loading the double into two registers.
+		 See the second define_split in rs6000.md handling a
+		 const_double_operand.  */
+	      return (num_insns_constant_wide (low)
+		      + num_insns_constant_wide (high));
+
+	    /* See the third define_split in rs6000.md handling a
+	       const_double_operand.  */
+	    val = (low & 0xffffffffUL) | (high << 32);
+	    mode = DImode;
 	  }
+	else
+	  gcc_unreachable ();
+	op = NULL_RTX;
+      }
+      break;
 
     default:
       gcc_unreachable ();
     }
+
+  int ins = num_insns_constant_wide (val);
+  if (ins > 2
+      && rs6000_is_valid_and_mask (op ? op : GEN_INT (val), mode))
+    ins =  2;
+  return ins;
 }
 
 /* Interpret element ELT of the CONST_VECTOR OP as an integer value.
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 8e50f5b822b..0a16b1f5d41 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -7713,7 +7713,7 @@  (define_insn_and_split "*mov<mode>_32bit"
 
 (define_insn_and_split "*mov<mode>_softfloat"
   [(set (match_operand:FMOVE128 0 "nonimmediate_operand" "=Y,r,r")
-	(match_operand:FMOVE128 1 "input_operand" "r,YGHF,r"))]
+	(match_operand:FMOVE128 1 "input_operand" "r,Y,r"))]
   "TARGET_SOFT_FLOAT
    && (gpc_reg_operand (operands[0], <MODE>mode)
        || gpc_reg_operand (operands[1], <MODE>mode))"
@@ -8670,7 +8670,7 @@  (define_insn "*movdi_internal32"
 
 	(match_operand:DI 1 "input_operand"
          "r,         Y,         r,         ^d,        m,          ^d,
-          IJKnGHF,   ^wb,       $wv,       wY,        Z,          ^wi,
+          IJKnF,     ^wb,       $wv,       wY,        Z,          ^wi,
           Oj,        wM,        OjwM,      Oj,        wM,         wS,
           wB"))]