diff mbox series

[66/77] Use scalar_mode for constant integers

Message ID 878tjsd81e.fsf@linaro.org
State New
Headers show
Series Add wrapper classes for machine_modes | expand

Commit Message

Richard Sandiford July 13, 2017, 9:02 a.m. UTC
This patch treats the mode associated with an integer constant as a
scalar_mode.  We can't use the more natural-sounding scalar_int_mode
because we also use (const_int 0) for bounds-checking modes.  (It might
be worth adding a bounds-specific code instead, but that's for another
day.)

This exposes a latent bug in simplify_immed_subreg, which for
vectors of CONST_WIDE_INTs would pass the vector mode rather than
the element mode to rtx_mode_t.

I think the:

		  /* We can get a 0 for an error mark.  */
		  || GET_MODE_CLASS (mode) == MODE_VECTOR_INT
		  || GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT

in immed_double_const is dead.  trunc_int_mode (via gen_int_mode)
would go on to ICE if the mode fitted in a HWI, and surely plenty
of other code would be confused to see a const_int be interpreted
as a vector.  We should instead be using CONST0_RTX (mode) if we
need a safe constant for a particular mode.

We didn't try to make these functions take scalar_mode arguments
because in many cases that would be too invasive at this stage.
Maybe it would become feasible in future.  Also, the long-term
direction should probably be to add modes to constant integers
rather than have then as VOIDmode odd-ones-out.  That would remove
the need for rtx_mode_t and thus remove the question whether they
should use scalar_int_mode, scalar_mode or machine_mode.

The patch also uses scalar_mode for the CONST_DOUBLE handling
in loc_descriptor.  In that case the mode can legitimately be
either floating-point or integral.

2017-07-13  Richard Sandiford  <richard.sandiford@linaro.org>
	    Alan Hayward  <alan.hayward@arm.com>
	    David Sherwood  <david.sherwood@arm.com>

gcc/
	* emit-rtl.c (immed_double_const): Use is_a <scalar_mode> instead
	of separate mode class checks.  Do not allow vector modes here.
	(immed_wide_int_const): Use as_a <scalar_mode>.
	* explow.c (trunc_int_for_mode): Likewise.
	* rtl.h (wi::int_traits<rtx_mode_t>::get_precision): Likewise.
	(wi::shwi): Likewise.
	(wi::min_value): Likewise.
	(wi::max_value): Likewise.
	* dwarf2out.c (loc_descriptor): Likewise.
	* simplify-rtx.c (simplify_immed_subreg): Fix rtx_mode_t argument
	for CONST_WIDE_INT.

Comments

Jeff Law Aug. 25, 2017, 4:56 a.m. UTC | #1
On 07/13/2017 03:02 AM, Richard Sandiford wrote:
> This patch treats the mode associated with an integer constant as a

> scalar_mode.  We can't use the more natural-sounding scalar_int_mode

> because we also use (const_int 0) for bounds-checking modes.  (It might

> be worth adding a bounds-specific code instead, but that's for another

> day.)

Is that the only reason why we can't use scalar_int_mode here -- the
bounds checking stuff?  What if it were to just magically disappear?


> 

> This exposes a latent bug in simplify_immed_subreg, which for

> vectors of CONST_WIDE_INTs would pass the vector mode rather than

> the element mode to rtx_mode_t.

> 

> I think the:

> 

> 		  /* We can get a 0 for an error mark.  */

> 		  || GET_MODE_CLASS (mode) == MODE_VECTOR_INT

> 		  || GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT

An interesting nugget.   It appears Aldy's commit message, but not in
the approved patch from June 2002.
> 

> in immed_double_const is dead.  trunc_int_mode (via gen_int_mode)

> would go on to ICE if the mode fitted in a HWI, and surely plenty

> of other code would be confused to see a const_int be interpreted

> as a vector.  We should instead be using CONST0_RTX (mode) if we

> need a safe constant for a particular mode.

Yea, if we wanted to use 0 as an error mark, we should be using it via
CONST0_RTX (mode).
> 

> We didn't try to make these functions take scalar_mode arguments

> because in many cases that would be too invasive at this stage.

> Maybe it would become feasible in future.  Also, the long-term

> direction should probably be to add modes to constant integers

> rather than have then as VOIDmode odd-ones-out.  That would remove

> the need for rtx_mode_t and thus remove the question whether they

> should use scalar_int_mode, scalar_mode or machine_mode.

THe lack of a mode on CONST_INTs is a long standing wart.  It's been
eons since we really thought about how to fix it.    I'd have to dig
real deep to remember why we've let the wart stand so long

> 

> The patch also uses scalar_mode for the CONST_DOUBLE handling

> in loc_descriptor.  In that case the mode can legitimately be

> either floating-point or integral.

> 

> 2017-07-13  Richard Sandiford  <richard.sandiford@linaro.org>

> 	    Alan Hayward  <alan.hayward@arm.com>

> 	    David Sherwood  <david.sherwood@arm.com>

> 

> gcc/

> 	* emit-rtl.c (immed_double_const): Use is_a <scalar_mode> instead

> 	of separate mode class checks.  Do not allow vector modes here.

> 	(immed_wide_int_const): Use as_a <scalar_mode>.

> 	* explow.c (trunc_int_for_mode): Likewise.

> 	* rtl.h (wi::int_traits<rtx_mode_t>::get_precision): Likewise.

> 	(wi::shwi): Likewise.

> 	(wi::min_value): Likewise.

> 	(wi::max_value): Likewise.

> 	* dwarf2out.c (loc_descriptor): Likewise.

> 	* simplify-rtx.c (simplify_immed_subreg): Fix rtx_mode_t argument

> 	for CONST_WIDE_INT.

OK.
jeff
Richard Sandiford Aug. 30, 2017, 12:29 p.m. UTC | #2
Jeff Law <law@redhat.com> writes:
> On 07/13/2017 03:02 AM, Richard Sandiford wrote:

>> This patch treats the mode associated with an integer constant as a

>> scalar_mode.  We can't use the more natural-sounding scalar_int_mode

>> because we also use (const_int 0) for bounds-checking modes.  (It might

>> be worth adding a bounds-specific code instead, but that's for another

>> day.)

> Is that the only reason why we can't use scalar_int_mode here -- the

> bounds checking stuff?  What if it were to just magically disappear?


:-)  I *think* that was the only case, but it's possible that once
we hit it, we didn't look much further.  

[...]

>> We didn't try to make these functions take scalar_mode arguments

>> because in many cases that would be too invasive at this stage.

>> Maybe it would become feasible in future.  Also, the long-term

>> direction should probably be to add modes to constant integers

>> rather than have then as VOIDmode odd-ones-out.  That would remove

>> the need for rtx_mode_t and thus remove the question whether they

>> should use scalar_int_mode, scalar_mode or machine_mode.

> THe lack of a mode on CONST_INTs is a long standing wart.  It's been

> eons since we really thought about how to fix it.    I'd have to dig

> real deep to remember why we've let the wart stand so long


When I last looked at the history, I got the impression it was just
lack of time.  I have a vague plan for how we could transition to
integers with modes, but then I've had the same plan for a while now
and no realistic chance of getting time to do it.

Thanks,
Richard
diff mbox series

Patch

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	2017-07-13 09:18:51.646771977 +0100
+++ gcc/emit-rtl.c	2017-07-13 09:18:54.682546579 +0100
@@ -599,7 +599,8 @@  lookup_const_wide_int (rtx wint)
 immed_wide_int_const (const wide_int_ref &v, machine_mode mode)
 {
   unsigned int len = v.get_len ();
-  unsigned int prec = GET_MODE_PRECISION (mode);
+  /* Not scalar_int_mode because we also allow pointer bound modes.  */
+  unsigned int prec = GET_MODE_PRECISION (as_a <scalar_mode> (mode));
 
   /* Allow truncation but not extension since we do not know if the
      number is signed or unsigned.  */
@@ -659,18 +660,10 @@  immed_double_const (HOST_WIDE_INT i0, HO
         (i.e., i1 consists only from copies of the sign bit, and sign
 	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
-  if (mode != VOIDmode)
-    {
-      gcc_assert (GET_MODE_CLASS (mode) == MODE_INT
-		  || GET_MODE_CLASS (mode) == MODE_PARTIAL_INT
-		  /* We can get a 0 for an error mark.  */
-		  || GET_MODE_CLASS (mode) == MODE_VECTOR_INT
-		  || GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT
-		  || GET_MODE_CLASS (mode) == MODE_POINTER_BOUNDS);
-
-      if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
-	return gen_int_mode (i0, mode);
-    }
+  scalar_mode smode;
+  if (is_a <scalar_mode> (mode, &smode)
+      && GET_MODE_BITSIZE (smode) <= HOST_BITS_PER_WIDE_INT)
+    return gen_int_mode (i0, mode);
 
   /* If this integer fits in one word, return a CONST_INT.  */
   if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0))
Index: gcc/explow.c
===================================================================
--- gcc/explow.c	2017-07-13 09:18:51.647771901 +0100
+++ gcc/explow.c	2017-07-13 09:18:54.682546579 +0100
@@ -49,14 +49,16 @@  static rtx break_out_memory_refs (rtx);
 HOST_WIDE_INT
 trunc_int_for_mode (HOST_WIDE_INT c, machine_mode mode)
 {
-  int width = GET_MODE_PRECISION (mode);
+  /* Not scalar_int_mode because we also allow pointer bound modes.  */
+  scalar_mode smode = as_a <scalar_mode> (mode);
+  int width = GET_MODE_PRECISION (smode);
 
   /* You want to truncate to a _what_?  */
   gcc_assert (SCALAR_INT_MODE_P (mode)
 	      || POINTER_BOUNDS_MODE_P (mode));
 
   /* Canonicalize BImode to 0 and STORE_FLAG_VALUE.  */
-  if (mode == BImode)
+  if (smode == BImode)
     return c & 1 ? STORE_FLAG_VALUE : 0;
 
   /* Sign-extend for the requested mode.  */
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	2017-07-13 09:18:51.662770770 +0100
+++ gcc/rtl.h	2017-07-13 09:18:54.682546579 +0100
@@ -2120,8 +2120,7 @@  typedef std::pair <rtx, machine_mode> rt
 inline unsigned int
 wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
 {
-  gcc_checking_assert (x.second != BLKmode && x.second != VOIDmode);
-  return GET_MODE_PRECISION (x.second);
+  return GET_MODE_PRECISION (as_a <scalar_mode> (x.second));
 }
 
 inline wi::storage_ref
@@ -2166,7 +2165,7 @@  wi::int_traits <rtx_mode_t>::decompose (
 inline wi::hwi_with_prec
 wi::shwi (HOST_WIDE_INT val, machine_mode mode)
 {
-  return shwi (val, GET_MODE_PRECISION (mode));
+  return shwi (val, GET_MODE_PRECISION (as_a <scalar_mode> (mode)));
 }
 
 /* Produce the smallest number that is represented in MODE.  The precision
@@ -2174,7 +2173,7 @@  wi::shwi (HOST_WIDE_INT val, machine_mod
 inline wide_int
 wi::min_value (machine_mode mode, signop sgn)
 {
-  return min_value (GET_MODE_PRECISION (mode), sgn);
+  return min_value (GET_MODE_PRECISION (as_a <scalar_mode> (mode)), sgn);
 }
 
 /* Produce the largest number that is represented in MODE.  The precision
@@ -2182,7 +2181,7 @@  wi::min_value (machine_mode mode, signop
 inline wide_int
 wi::max_value (machine_mode mode, signop sgn)
 {
-  return max_value (GET_MODE_PRECISION (mode), sgn);
+  return max_value (GET_MODE_PRECISION (as_a <scalar_mode> (mode)), sgn);
 }
 
 extern void init_rtlanal (void);
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	2017-07-13 09:18:51.644772127 +0100
+++ gcc/dwarf2out.c	2017-07-13 09:18:54.681546653 +0100
@@ -15781,10 +15781,11 @@  loc_descriptor (rtx rtl, machine_mode mo
 	     or a floating-point constant.  A CONST_DOUBLE is used whenever
 	     the constant requires more than one word in order to be
 	     adequately represented.  We output CONST_DOUBLEs as blocks.  */
+	  scalar_mode smode = as_a <scalar_mode> (mode);
 	  loc_result = new_loc_descr (DW_OP_implicit_value,
-				      GET_MODE_SIZE (mode), 0);
+				      GET_MODE_SIZE (smode), 0);
 #if TARGET_SUPPORTS_WIDE_INT == 0
-	  if (!SCALAR_FLOAT_MODE_P (mode))
+	  if (!SCALAR_FLOAT_MODE_P (smode))
 	    {
 	      loc_result->dw_loc_oprnd2.val_class = dw_val_class_const_double;
 	      loc_result->dw_loc_oprnd2.v.val_double
@@ -15793,7 +15794,7 @@  loc_descriptor (rtx rtl, machine_mode mo
 	  else
 #endif
 	    {
-	      unsigned int length = GET_MODE_SIZE (mode);
+	      unsigned int length = GET_MODE_SIZE (smode);
 	      unsigned char *array = ggc_vec_alloc<unsigned char> (length);
 
 	      insert_float (rtl, array);
Index: gcc/simplify-rtx.c
===================================================================
--- gcc/simplify-rtx.c	2017-07-13 09:18:53.276650175 +0100
+++ gcc/simplify-rtx.c	2017-07-13 09:18:54.683546506 +0100
@@ -5785,7 +5785,7 @@  simplify_immed_subreg (machine_mode oute
 
 	case CONST_WIDE_INT:
 	  {
-	    rtx_mode_t val = rtx_mode_t (el, innermode);
+	    rtx_mode_t val = rtx_mode_t (el, GET_MODE_INNER (innermode));
 	    unsigned char extend = wi::sign_mask (val);
 	    int prec = wi::get_precision (val);