[PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[David Sherwood]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

Hi,

Part 1 of this change is a clean-up. I have changed calls to GET_MODE_INNER (m)
so that it returns m in cases where there is no inner mode. This simplifies some
of the calling code by removing the need to check for VOIDmode and allows
calling it unconditionally. I also removed element_precision () as it was only
called in one place and thought it neater to call GET_MODE_PRECISION explicitly.

Parts 2-4 will include further tidy-ups and optimisations based on [1/N].

Good to go?

Regards,
David Sherwood.

2015-07-17  David Sherwood  <david.sherwood@arm.com>

    gcc/
        * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
        GET_MODE_INNER unconditionally.
        * config/spu/spu.c (arith_immediate_p): Likewise.
        * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
        * expmed.c (synth_mult): Remove check for VOIDmode result from
        GET_MODE_INNER.
        (expand_mult_const): Likewise.
        * fold-const.c (): Replace call to element_precision with call to
        GET_MODE_PRECISION.
        * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
        m->name.
        (emit_mode_inner): Likewise.
        * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
        result check.
        * machmode.h (GET_MODE_UNIT_SIZE): Simplify.
        (GET_MODE_UNIT_PRECISION): Likewise.
        * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
        * simplify-rtx.c (simplify_immed_subreg): Likewise.
        * stor-layout.c (bitwise_type_for_mode): Update assert.
        (element_precision): Remove.


[-- Attachment #2: mode_inner1.patch --]
[-- Type: application/octet-stream, Size: 9575 bytes --]

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e1bc727..ab96824 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -12234,18 +12234,16 @@ neon_valid_immediate (rtx op, machine_mode mode, int inverse,
   bool vector = GET_CODE (op) == CONST_VECTOR;
 
   if (vector)
-    {
-      n_elts = CONST_VECTOR_NUNITS (op);
-      innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
-    }
+    n_elts = CONST_VECTOR_NUNITS (op);
   else
     {
       n_elts = 1;
       if (mode == VOIDmode)
 	mode = DImode;
-      innersize = GET_MODE_SIZE (mode);
     }
 
+  innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
+
   /* Vectors of float constants.  */
   if (GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT)
     {
@@ -12821,10 +12819,7 @@ neon_const_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high)
 HOST_WIDE_INT
 neon_element_bits (machine_mode mode)
 {
-  if (mode == DImode)
-    return GET_MODE_BITSIZE (mode);
-  else
-    return GET_MODE_BITSIZE (GET_MODE_INNER (mode));
+  return GET_MODE_BITSIZE (GET_MODE_INNER (mode));
 }
 
 \f
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 55e1e2d..c68e284 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -19626,7 +19626,6 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
     case V8SFmode:
     case V4SFmode:
       vec_mode = mode;
-      mode = GET_MODE_INNER (mode);
       imode = SImode;
       break;
 
@@ -19637,7 +19636,6 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
     case V4DFmode:
     case V2DFmode:
       vec_mode = mode;
-      mode = GET_MODE_INNER (mode);
       imode = DImode;
       break;
 
@@ -19651,17 +19649,18 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
       gcc_unreachable ();
     }
 
-  w = wi::set_bit_in_zero (GET_MODE_BITSIZE (mode) - 1,
-			   GET_MODE_BITSIZE (mode));
+  machine_mode inner_mode = GET_MODE_INNER (mode);
+  w = wi::set_bit_in_zero (GET_MODE_BITSIZE (inner_mode) - 1,
+			   GET_MODE_BITSIZE (inner_mode));
   if (invert)
     w = wi::bit_not (w);
 
   /* Force this value into the low part of a fp vector constant.  */
   mask = immed_wide_int_const (w, imode);
-  mask = gen_lowpart (mode, mask);
+  mask = gen_lowpart (inner_mode, mask);
 
   if (vec_mode == VOIDmode)
-    return force_reg (mode, mask);
+    return force_reg (inner_mode, mask);
 
   v = ix86_build_const_vector (vec_mode, vect, mask);
   return force_reg (vec_mode, v);
diff --git a/gcc/config/spu/spu.c b/gcc/config/spu/spu.c
index 99efb67..68840f5 100644
--- a/gcc/config/spu/spu.c
+++ b/gcc/config/spu/spu.c
@@ -3391,9 +3391,7 @@ arith_immediate_p (rtx op, machine_mode mode,
 
   constant_to_array (mode, op, arr);
 
-  if (VECTOR_MODE_P (mode))
-    mode = GET_MODE_INNER (mode);
-
+  mode = GET_MODE_INNER (mode);
   bytes = GET_MODE_SIZE (mode);
   mode = mode_for_size (GET_MODE_BITSIZE (mode), MODE_INT, 0);
 
diff --git a/gcc/expmed.c b/gcc/expmed.c
index 557da33..3c70cde 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -2427,8 +2427,6 @@ synth_mult (struct algorithm *alg_out, unsigned HOST_WIDE_INT t,
 
   /* Be prepared for vector modes.  */
   imode = GET_MODE_INNER (mode);
-  if (imode == VOIDmode)
-    imode = mode;
 
   maxm = MIN (BITS_PER_WORD, GET_MODE_BITSIZE (imode));
 
@@ -3088,8 +3086,6 @@ expand_mult_const (machine_mode mode, rtx op0, HOST_WIDE_INT val,
   /* Compare only the bits of val and val_so_far that are significant
      in the result mode, to avoid sign-/zero-extension confusion.  */
   nmode = GET_MODE_INNER (mode);
-  if (nmode == VOIDmode)
-    nmode = mode;
   val &= GET_MODE_MASK (nmode);
   val_so_far &= GET_MODE_MASK (nmode);
   gcc_assert (val == val_so_far);
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 93dd29d..12b15f8 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -9503,7 +9503,7 @@ fold_binary_loc (location_t loc,
 	    /* Only create rotates in complete modes.  Other cases are not
 	       expanded properly.  */
 	    && (element_precision (rtype)
-		== element_precision (TYPE_MODE (rtype))))
+		== GET_MODE_PRECISION (GET_MODE_INNER (TYPE_MODE (rtype)))))
 	  {
 	    tree tree01, tree11;
 	    enum tree_code code01, code11;
diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8e9337c..f4db427 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1039,7 +1039,7 @@ mode_inner_inline (machine_mode mode)\n\
   for_all_modes (c, m)
     printf ("    case %smode: return %smode;\n", m->name,
 	    c != MODE_PARTIAL_INT && m->component
-	    ? m->component->name : void_mode->name);
+	    ? m->component->name : m->name);
 
   puts ("\
     default: return mode_inner[mode];\n\
@@ -1338,7 +1338,7 @@ emit_mode_inner (void)
   for_all_modes (c, m)
     tagged_printf ("%smode",
 		   c != MODE_PARTIAL_INT && m->component
-		   ? m->component->name : void_mode->name,
+		   ? m->component->name : m->name,
 		   m->name);
 
   print_closer ();
diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index 2d048e8..1b88115 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -2682,7 +2682,7 @@ lto_write_mode_table (void)
     if (streamer_mode_table[i])
       {
 	machine_mode m = (machine_mode) i;
-	if (GET_MODE_INNER (m) != VOIDmode)
+	if (GET_MODE_INNER (m) != m)
 	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
       }
   /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
@@ -2692,7 +2692,7 @@ lto_write_mode_table (void)
       if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
 	{
 	  machine_mode m = (machine_mode) i;
-	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
+	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
 	    continue;
 	  bp_pack_value (&bp, m, 8);
 	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
diff --git a/gcc/machmode.h b/gcc/machmode.h
index 5ab7eeb..c6b4064 100644
--- a/gcc/machmode.h
+++ b/gcc/machmode.h
@@ -223,18 +223,12 @@ extern const unsigned char mode_inner[NUM_MACHINE_MODES];
 /* Get the size in bytes or bites of the basic parts of an
    object of mode MODE.  */
 
-#define GET_MODE_UNIT_SIZE(MODE)		\
-  (GET_MODE_INNER (MODE) == VOIDmode		\
-   ? GET_MODE_SIZE (MODE)			\
-   : GET_MODE_SIZE (GET_MODE_INNER (MODE)))
+#define GET_MODE_UNIT_SIZE(MODE) GET_MODE_SIZE (GET_MODE_INNER (MODE))
 
 #define GET_MODE_UNIT_BITSIZE(MODE) \
   ((unsigned short) (GET_MODE_UNIT_SIZE (MODE) * BITS_PER_UNIT))
 
-#define GET_MODE_UNIT_PRECISION(MODE)		\
-  (GET_MODE_INNER (MODE) == VOIDmode		\
-   ? GET_MODE_PRECISION (MODE)			\
-   : GET_MODE_PRECISION (GET_MODE_INNER (MODE)))
+#define GET_MODE_UNIT_PRECISION(MODE) GET_MODE_PRECISION (GET_MODE_INNER (MODE))
 
 /* Get the number of units in the object.  */
 
@@ -320,10 +314,6 @@ extern unsigned get_mode_alignment (machine_mode);
 
 #define GET_MODE_ALIGNMENT(MODE) get_mode_alignment (MODE)
 
-/* Get the precision of the mode or its inner mode if it has one.  */
-
-extern unsigned int element_precision (machine_mode);
-
 /* For each class, get the narrowest mode in that class.  */
 
 extern const unsigned char class_narrowest_mode[MAX_MODE_CLASS];
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index 0bba878..4269a71 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -3571,10 +3571,7 @@ subreg_get_info (unsigned int xregno, machine_mode xmode,
       machine_mode xmode_unit;
 
       nregs_xmode = HARD_REGNO_NREGS_WITH_PADDING (xregno, xmode);
-      if (GET_MODE_INNER (xmode) == VOIDmode)
-	xmode_unit = xmode;
-      else
-	xmode_unit = GET_MODE_INNER (xmode);
+      xmode_unit = GET_MODE_INNER (xmode);
       gcc_assert (HARD_REGNO_NREGS_HAS_PADDING (xregno, xmode_unit));
       gcc_assert (nregs_xmode
 		  == (GET_MODE_NUNITS (xmode)
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index fde9944..70c1308 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5622,21 +5622,17 @@ simplify_immed_subreg (machine_mode outermode, rtx op,
   value_start = byte * (BITS_PER_UNIT / value_bit);
 
   /* Re-pack the value.  */
+  num_elem = GET_MODE_NUNITS (outermode);
 
   if (VECTOR_MODE_P (outermode))
     {
-      num_elem = GET_MODE_NUNITS (outermode);
       result_v = rtvec_alloc (num_elem);
       elems = &RTVEC_ELT (result_v, 0);
-      outer_submode = GET_MODE_INNER (outermode);
     }
   else
-    {
-      num_elem = 1;
-      elems = &result_s;
-      outer_submode = outermode;
-    }
+    elems = &result_s;
 
+  outer_submode = GET_MODE_INNER (outermode);
   outer_class = GET_MODE_CLASS (outer_submode);
   elem_bitsize = GET_MODE_BITSIZE (outer_submode);
 
diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
index 0d4f4a4..9757777 100644
--- a/gcc/stor-layout.c
+++ b/gcc/stor-layout.c
@@ -491,7 +491,7 @@ bitwise_type_for_mode (machine_mode mode)
   if (COMPLEX_MODE_P (mode))
     return build_complex_type (inner_type);
 
-  gcc_checking_assert (GET_MODE_INNER (mode) == VOIDmode);
+  gcc_checking_assert (GET_MODE_INNER (mode) == mode);
   return inner_type;
 }
 
@@ -548,18 +548,6 @@ get_mode_alignment (machine_mode mode)
   return MIN (BIGGEST_ALIGNMENT, MAX (1, mode_base_align[mode]*BITS_PER_UNIT));
 }
 
-/* Return the precision of the mode, or for a complex or vector mode the
-   precision of the mode of its elements.  */
-
-unsigned int
-element_precision (machine_mode mode)
-{
-  if (COMPLEX_MODE_P (mode) || VECTOR_MODE_P (mode))
-    mode = GET_MODE_INNER (mode);
-
-  return GET_MODE_PRECISION (mode);
-}
-
 /* Return the natural mode of an array, given that it is SIZE bytes in
    total and has elements of type ELEM_TYPE.  */
 

Re: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[Jeff Law]

On 07/27/2015 04:25 AM, David Sherwood wrote:
> Hi,
>
> Part 1 of this change is a clean-up. I have changed calls to GET_MODE_INNER (m)
> so that it returns m in cases where there is no inner mode. This simplifies some
> of the calling code by removing the need to check for VOIDmode and allows
> calling it unconditionally. I also removed element_precision () as it was only
> called in one place and thought it neater to call GET_MODE_PRECISION explicitly.
>
> Parts 2-4 will include further tidy-ups and optimisations based on [1/N].
>
> Good to go?
>
> Regards,
> David Sherwood.
>
> 2015-07-17  David Sherwood<david.sherwood@arm.com>
>
>      gcc/
>          * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
>          GET_MODE_INNER unconditionally.
>          * config/spu/spu.c (arith_immediate_p): Likewise.
>          * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
>          * expmed.c (synth_mult): Remove check for VOIDmode result from
>          GET_MODE_INNER.
>          (expand_mult_const): Likewise.
>          * fold-const.c (): Replace call to element_precision with call to
>          GET_MODE_PRECISION.
>          * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
>          m->name.
>          (emit_mode_inner): Likewise.
>          * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
>          result check.
>          * machmode.h (GET_MODE_UNIT_SIZE): Simplify.
>          (GET_MODE_UNIT_PRECISION): Likewise.
>          * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
>          * simplify-rtx.c (simplify_immed_subreg): Likewise.
>          * stor-layout.c (bitwise_type_for_mode): Update assert.
>          (element_precision): Remove.
Somehow my brain kept translating INNER into NARROWER.  Naturally I was 
having considerable trouble seeing how the patch could be correct ;-) 
Looking at insn-modes.h cleared things up quickly.

In a lot of ways this makes GET_INNER_MODE act more like 
GET_MODE_NUNITS, which is probably good.

You need to update the comment for GET_MODE_INNER in machmode.h to 
reflect the change in its return value for non-vector modes.

With that update, this patch is fine.

jeff

RE: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[David Sherwood]

> 
> On 07/27/2015 04:25 AM, David Sherwood wrote:
> > Hi,
> >
> > Part 1 of this change is a clean-up. I have changed calls to GET_MODE_INNER (m)
> > so that it returns m in cases where there is no inner mode. This simplifies some
> > of the calling code by removing the need to check for VOIDmode and allows
> > calling it unconditionally. I also removed element_precision () as it was only
> > called in one place and thought it neater to call GET_MODE_PRECISION explicitly.
> >
> > Parts 2-4 will include further tidy-ups and optimisations based on [1/N].
> >
> > Good to go?
> >
> > Regards,
> > David Sherwood.
> >
> > 2015-07-17  David Sherwood<david.sherwood@arm.com>
> >
> >      gcc/
> >          * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
> >          GET_MODE_INNER unconditionally.
> >          * config/spu/spu.c (arith_immediate_p): Likewise.
> >          * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
> >          * expmed.c (synth_mult): Remove check for VOIDmode result from
> >          GET_MODE_INNER.
> >          (expand_mult_const): Likewise.
> >          * fold-const.c (): Replace call to element_precision with call to
> >          GET_MODE_PRECISION.
> >          * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
> >          m->name.
> >          (emit_mode_inner): Likewise.
> >          * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
> >          result check.
> >          * machmode.h (GET_MODE_UNIT_SIZE): Simplify.
> >          (GET_MODE_UNIT_PRECISION): Likewise.
> >          * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
> >          * simplify-rtx.c (simplify_immed_subreg): Likewise.
> >          * stor-layout.c (bitwise_type_for_mode): Update assert.
> >          (element_precision): Remove.
> Somehow my brain kept translating INNER into NARROWER.  Naturally I was
> having considerable trouble seeing how the patch could be correct ;-)
> Looking at insn-modes.h cleared things up quickly.
> 
> In a lot of ways this makes GET_INNER_MODE act more like
> GET_MODE_NUNITS, which is probably good.
> 
> You need to update the comment for GET_MODE_INNER in machmode.h to
> reflect the change in its return value for non-vector modes.
Thanks for the quick response! Before I post a new patch, does this new
comment seem ok?

/* Where MODE represents a vector return the mode of the inner elements,
otherwise just return MODE.  */

Dave.

> 
> With that update, this patch is fine.
> 
> jeff

RE: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[David Sherwood]

[-- Attachment #1: Type: text/plain, Size: 4122 bytes --]

Hi,

I have updated the comment above GET_MODE_INNER and while there I have
fixed a spelling mistake in the comment above GET_MODE_UNIT_SIZE.

Tested:
aarch64 and aarch64_be - no regressions in gcc testsuite
x86_64 - bootstrap build, no testsuite regressions
arm-none-eabi - no regressions in gcc testsuite
Run contrib/config-list.mk - only build failures are ones that fail anyway with
warnings being treated as errors.

Hope this is ok.

Cheers,
Dave.

2015-07-28  David Sherwood  <david.sherwood@arm.com>

    gcc/
        * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
        GET_MODE_INNER unconditionally.
        * config/spu/spu.c (arith_immediate_p): Likewise.
        * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
        * expmed.c (synth_mult): Remove check for VOIDmode result from
        GET_MODE_INNER.
        (expand_mult_const): Likewise.
        * fold-const.c (): Replace call to element_precision with call to
        GET_MODE_PRECISION.
        * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
        m->name.
        (emit_mode_inner): Likewise.
        * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
        result check.
        * machmode.h (GET_MODE_INNER): Update comment.
        (GET_MODE_UNIT_SIZE): Simplify and fix spelling mistake in comment.
        (GET_MODE_UNIT_PRECISION): Simplify.
        (element_precision): Remove.
        * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
        * simplify-rtx.c (simplify_immed_subreg): Likewise.
        * stor-layout.c (bitwise_type_for_mode): Update assert.
        (element_precision): Remove.

> 
> On 07/27/2015 04:25 AM, David Sherwood wrote:
> > Hi,
> >
> > Part 1 of this change is a clean-up. I have changed calls to GET_MODE_INNER (m)
> > so that it returns m in cases where there is no inner mode. This simplifies some
> > of the calling code by removing the need to check for VOIDmode and allows
> > calling it unconditionally. I also removed element_precision () as it was only
> > called in one place and thought it neater to call GET_MODE_PRECISION explicitly.
> >
> > Parts 2-4 will include further tidy-ups and optimisations based on [1/N].
> >
> > Good to go?
> >
> > Regards,
> > David Sherwood.
> >
> > 2015-07-17  David Sherwood<david.sherwood@arm.com>
> >
> >      gcc/
> >          * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
> >          GET_MODE_INNER unconditionally.
> >          * config/spu/spu.c (arith_immediate_p): Likewise.
> >          * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
> >          * expmed.c (synth_mult): Remove check for VOIDmode result from
> >          GET_MODE_INNER.
> >          (expand_mult_const): Likewise.
> >          * fold-const.c (): Replace call to element_precision with call to
> >          GET_MODE_PRECISION.
> >          * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
> >          m->name.
> >          (emit_mode_inner): Likewise.
> >          * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
> >          result check.
> >          * machmode.h (GET_MODE_UNIT_SIZE): Simplify.
> >          (GET_MODE_UNIT_PRECISION): Likewise.
> >          * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
> >          * simplify-rtx.c (simplify_immed_subreg): Likewise.
> >          * stor-layout.c (bitwise_type_for_mode): Update assert.
> >          (element_precision): Remove.
> Somehow my brain kept translating INNER into NARROWER.  Naturally I was
> having considerable trouble seeing how the patch could be correct ;-)
> Looking at insn-modes.h cleared things up quickly.
> 
> In a lot of ways this makes GET_INNER_MODE act more like
> GET_MODE_NUNITS, which is probably good.
> 
> You need to update the comment for GET_MODE_INNER in machmode.h to
> reflect the change in its return value for non-vector modes.
> 
> With that update, this patch is fine.
> 
> jeff


[-- Attachment #2: mode_inner1.patch --]
[-- Type: application/octet-stream, Size: 10222 bytes --]

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e1bc727..ab96824 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -12234,18 +12234,16 @@ neon_valid_immediate (rtx op, machine_mode mode, int inverse,
   bool vector = GET_CODE (op) == CONST_VECTOR;
 
   if (vector)
-    {
-      n_elts = CONST_VECTOR_NUNITS (op);
-      innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
-    }
+    n_elts = CONST_VECTOR_NUNITS (op);
   else
     {
       n_elts = 1;
       if (mode == VOIDmode)
 	mode = DImode;
-      innersize = GET_MODE_SIZE (mode);
     }
 
+  innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
+
   /* Vectors of float constants.  */
   if (GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT)
     {
@@ -12821,10 +12819,7 @@ neon_const_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high)
 HOST_WIDE_INT
 neon_element_bits (machine_mode mode)
 {
-  if (mode == DImode)
-    return GET_MODE_BITSIZE (mode);
-  else
-    return GET_MODE_BITSIZE (GET_MODE_INNER (mode));
+  return GET_MODE_BITSIZE (GET_MODE_INNER (mode));
 }
 
 \f
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 55e1e2d..c68e284 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -19626,7 +19626,6 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
     case V8SFmode:
     case V4SFmode:
       vec_mode = mode;
-      mode = GET_MODE_INNER (mode);
       imode = SImode;
       break;
 
@@ -19637,7 +19636,6 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
     case V4DFmode:
     case V2DFmode:
       vec_mode = mode;
-      mode = GET_MODE_INNER (mode);
       imode = DImode;
       break;
 
@@ -19651,17 +19649,18 @@ ix86_build_signbit_mask (machine_mode mode, bool vect, bool invert)
       gcc_unreachable ();
     }
 
-  w = wi::set_bit_in_zero (GET_MODE_BITSIZE (mode) - 1,
-			   GET_MODE_BITSIZE (mode));
+  machine_mode inner_mode = GET_MODE_INNER (mode);
+  w = wi::set_bit_in_zero (GET_MODE_BITSIZE (inner_mode) - 1,
+			   GET_MODE_BITSIZE (inner_mode));
   if (invert)
     w = wi::bit_not (w);
 
   /* Force this value into the low part of a fp vector constant.  */
   mask = immed_wide_int_const (w, imode);
-  mask = gen_lowpart (mode, mask);
+  mask = gen_lowpart (inner_mode, mask);
 
   if (vec_mode == VOIDmode)
-    return force_reg (mode, mask);
+    return force_reg (inner_mode, mask);
 
   v = ix86_build_const_vector (vec_mode, vect, mask);
   return force_reg (vec_mode, v);
diff --git a/gcc/config/spu/spu.c b/gcc/config/spu/spu.c
index 99efb67..68840f5 100644
--- a/gcc/config/spu/spu.c
+++ b/gcc/config/spu/spu.c
@@ -3391,9 +3391,7 @@ arith_immediate_p (rtx op, machine_mode mode,
 
   constant_to_array (mode, op, arr);
 
-  if (VECTOR_MODE_P (mode))
-    mode = GET_MODE_INNER (mode);
-
+  mode = GET_MODE_INNER (mode);
   bytes = GET_MODE_SIZE (mode);
   mode = mode_for_size (GET_MODE_BITSIZE (mode), MODE_INT, 0);
 
diff --git a/gcc/expmed.c b/gcc/expmed.c
index 557da33..3c70cde 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -2427,8 +2427,6 @@ synth_mult (struct algorithm *alg_out, unsigned HOST_WIDE_INT t,
 
   /* Be prepared for vector modes.  */
   imode = GET_MODE_INNER (mode);
-  if (imode == VOIDmode)
-    imode = mode;
 
   maxm = MIN (BITS_PER_WORD, GET_MODE_BITSIZE (imode));
 
@@ -3088,8 +3086,6 @@ expand_mult_const (machine_mode mode, rtx op0, HOST_WIDE_INT val,
   /* Compare only the bits of val and val_so_far that are significant
      in the result mode, to avoid sign-/zero-extension confusion.  */
   nmode = GET_MODE_INNER (mode);
-  if (nmode == VOIDmode)
-    nmode = mode;
   val &= GET_MODE_MASK (nmode);
   val_so_far &= GET_MODE_MASK (nmode);
   gcc_assert (val == val_so_far);
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 93dd29d..12b15f8 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -9503,7 +9503,7 @@ fold_binary_loc (location_t loc,
 	    /* Only create rotates in complete modes.  Other cases are not
 	       expanded properly.  */
 	    && (element_precision (rtype)
-		== element_precision (TYPE_MODE (rtype))))
+		== GET_MODE_PRECISION (GET_MODE_INNER (TYPE_MODE (rtype)))))
 	  {
 	    tree tree01, tree11;
 	    enum tree_code code01, code11;
diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8e9337c..f4db427 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1039,7 +1039,7 @@ mode_inner_inline (machine_mode mode)\n\
   for_all_modes (c, m)
     printf ("    case %smode: return %smode;\n", m->name,
 	    c != MODE_PARTIAL_INT && m->component
-	    ? m->component->name : void_mode->name);
+	    ? m->component->name : m->name);
 
   puts ("\
     default: return mode_inner[mode];\n\
@@ -1338,7 +1338,7 @@ emit_mode_inner (void)
   for_all_modes (c, m)
     tagged_printf ("%smode",
 		   c != MODE_PARTIAL_INT && m->component
-		   ? m->component->name : void_mode->name,
+		   ? m->component->name : m->name,
 		   m->name);
 
   print_closer ();
diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index 2d048e8..1b88115 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -2682,7 +2682,7 @@ lto_write_mode_table (void)
     if (streamer_mode_table[i])
       {
 	machine_mode m = (machine_mode) i;
-	if (GET_MODE_INNER (m) != VOIDmode)
+	if (GET_MODE_INNER (m) != m)
 	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
       }
   /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
@@ -2692,7 +2692,7 @@ lto_write_mode_table (void)
       if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
 	{
 	  machine_mode m = (machine_mode) i;
-	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
+	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
 	    continue;
 	  bp_pack_value (&bp, m, 8);
 	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
diff --git a/gcc/machmode.h b/gcc/machmode.h
index 5ab7eeb..6575517 100644
--- a/gcc/machmode.h
+++ b/gcc/machmode.h
@@ -209,7 +209,9 @@ extern const unsigned HOST_WIDE_INT mode_mask_array[NUM_MACHINE_MODES];
 
 #define GET_MODE_MASK(MODE) mode_mask_array[MODE]
 
-/* Return the mode of the inner elements in a vector.  */
+/* Return the mode of the basic parts of MODE.  For vector modes this is the
+   mode of the vector elements.  For complex modes it is the mode of the real
+   and imaginary parts.  For other modes it is MODE itself.  */
 
 extern const unsigned char mode_inner[NUM_MACHINE_MODES];
 #if GCC_VERSION >= 4001
@@ -220,21 +222,15 @@ extern const unsigned char mode_inner[NUM_MACHINE_MODES];
 #define GET_MODE_INNER(MODE) ((machine_mode) mode_inner[MODE])
 #endif
 
-/* Get the size in bytes or bites of the basic parts of an
+/* Get the size in bytes or bits of the basic parts of an
    object of mode MODE.  */
 
-#define GET_MODE_UNIT_SIZE(MODE)		\
-  (GET_MODE_INNER (MODE) == VOIDmode		\
-   ? GET_MODE_SIZE (MODE)			\
-   : GET_MODE_SIZE (GET_MODE_INNER (MODE)))
+#define GET_MODE_UNIT_SIZE(MODE) GET_MODE_SIZE (GET_MODE_INNER (MODE))
 
 #define GET_MODE_UNIT_BITSIZE(MODE) \
   ((unsigned short) (GET_MODE_UNIT_SIZE (MODE) * BITS_PER_UNIT))
 
-#define GET_MODE_UNIT_PRECISION(MODE)		\
-  (GET_MODE_INNER (MODE) == VOIDmode		\
-   ? GET_MODE_PRECISION (MODE)			\
-   : GET_MODE_PRECISION (GET_MODE_INNER (MODE)))
+#define GET_MODE_UNIT_PRECISION(MODE) GET_MODE_PRECISION (GET_MODE_INNER (MODE))
 
 /* Get the number of units in the object.  */
 
@@ -320,10 +316,6 @@ extern unsigned get_mode_alignment (machine_mode);
 
 #define GET_MODE_ALIGNMENT(MODE) get_mode_alignment (MODE)
 
-/* Get the precision of the mode or its inner mode if it has one.  */
-
-extern unsigned int element_precision (machine_mode);
-
 /* For each class, get the narrowest mode in that class.  */
 
 extern const unsigned char class_narrowest_mode[MAX_MODE_CLASS];
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index 0bba878..4269a71 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -3571,10 +3571,7 @@ subreg_get_info (unsigned int xregno, machine_mode xmode,
       machine_mode xmode_unit;
 
       nregs_xmode = HARD_REGNO_NREGS_WITH_PADDING (xregno, xmode);
-      if (GET_MODE_INNER (xmode) == VOIDmode)
-	xmode_unit = xmode;
-      else
-	xmode_unit = GET_MODE_INNER (xmode);
+      xmode_unit = GET_MODE_INNER (xmode);
       gcc_assert (HARD_REGNO_NREGS_HAS_PADDING (xregno, xmode_unit));
       gcc_assert (nregs_xmode
 		  == (GET_MODE_NUNITS (xmode)
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index fde9944..70c1308 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5622,21 +5622,17 @@ simplify_immed_subreg (machine_mode outermode, rtx op,
   value_start = byte * (BITS_PER_UNIT / value_bit);
 
   /* Re-pack the value.  */
+  num_elem = GET_MODE_NUNITS (outermode);
 
   if (VECTOR_MODE_P (outermode))
     {
-      num_elem = GET_MODE_NUNITS (outermode);
       result_v = rtvec_alloc (num_elem);
       elems = &RTVEC_ELT (result_v, 0);
-      outer_submode = GET_MODE_INNER (outermode);
     }
   else
-    {
-      num_elem = 1;
-      elems = &result_s;
-      outer_submode = outermode;
-    }
+    elems = &result_s;
 
+  outer_submode = GET_MODE_INNER (outermode);
   outer_class = GET_MODE_CLASS (outer_submode);
   elem_bitsize = GET_MODE_BITSIZE (outer_submode);
 
diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
index 0d4f4a4..9757777 100644
--- a/gcc/stor-layout.c
+++ b/gcc/stor-layout.c
@@ -491,7 +491,7 @@ bitwise_type_for_mode (machine_mode mode)
   if (COMPLEX_MODE_P (mode))
     return build_complex_type (inner_type);
 
-  gcc_checking_assert (GET_MODE_INNER (mode) == VOIDmode);
+  gcc_checking_assert (GET_MODE_INNER (mode) == mode);
   return inner_type;
 }
 
@@ -548,18 +548,6 @@ get_mode_alignment (machine_mode mode)
   return MIN (BIGGEST_ALIGNMENT, MAX (1, mode_base_align[mode]*BITS_PER_UNIT));
 }
 
-/* Return the precision of the mode, or for a complex or vector mode the
-   precision of the mode of its elements.  */
-
-unsigned int
-element_precision (machine_mode mode)
-{
-  if (COMPLEX_MODE_P (mode) || VECTOR_MODE_P (mode))
-    mode = GET_MODE_INNER (mode);
-
-  return GET_MODE_PRECISION (mode);
-}
-
 /* Return the natural mode of an array, given that it is SIZE bytes in
    total and has elements of type ELEM_TYPE.  */
 

Re: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[Richard Sandiford]

"David Sherwood" <david.sherwood@arm.com> writes:
> Hi,
>
> I have updated the comment above GET_MODE_INNER and while there I have
> fixed a spelling mistake in the comment above GET_MODE_UNIT_SIZE.
>
> Tested:
> aarch64 and aarch64_be - no regressions in gcc testsuite
> x86_64 - bootstrap build, no testsuite regressions
> arm-none-eabi - no regressions in gcc testsuite
> Run contrib/config-list.mk - only build failures are ones that fail anyway with
> warnings being treated as errors.
>
> Hope this is ok.
>
> Cheers,
> Dave.

Since Jeff conditionally approved the patch, I went ahead and applied it.

Thanks,
Richard

Regression in target MIC compiler (was: nvptx offloading patches [3/n], RFD)

[Ilya Verbin]

Hi!

I've noticed that target MIC compiler from trunk hangs forever in
lto_input_mode_table in this loop, even on simple testcases.

On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
+      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
+	 if not found, fallback to all modes.  */
+      int pass;
+      for (pass = 0; pass < 2; pass++)
+	for (machine_mode mr = pass ? VOIDmode
+				    : GET_CLASS_NARROWEST_MODE (mclass);
+	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
+	     pass ? mr = (machine_mode) (m + 1)
+		  : mr = GET_MODE_WIDER_MODE (mr))
+	  if (GET_MODE_CLASS (mr) != mclass
+	      || GET_MODE_SIZE (mr) != size
+	      || GET_MODE_PRECISION (mr) != prec
+	      || GET_MODE_INNER (mr) != inner
+	      || GET_MODE_IBIT (mr) != ibit
+	      || GET_MODE_FBIT (mr) != fbit
+	      || GET_MODE_NUNITS (mr) != nunits)
+	    continue;

Given that gomp-4_1-branch works ok, the problem was introduced somewhere
between 9 and 31 Jul.  I'll try to find the revision.

  -- Ilya

Re: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[Jeff Law]

On 07/28/2015 03:47 AM, David Sherwood wrote:
> Sorry, the comment should reflect complex types too. How about this?
>
> /* Return the mode of the basic parts of MODE.  For vector modes this is the
>      mode of the vector elements.  For complex modes it is the mode of the real
>      and imaginary parts.  For other modes it is MODE itself.  */
Even better :-)

jeff

Re: [PATCH][1/N] Change GET_MODE_INNER to always return a non-void mode

[Jeff Law]

On 07/28/2015 03:35 AM, David Sherwood wrote:
>>
>> On 07/27/2015 04:25 AM, David Sherwood wrote:
>>> Hi,
>>>
>>> Part 1 of this change is a clean-up. I have changed calls to GET_MODE_INNER (m)
>>> so that it returns m in cases where there is no inner mode. This simplifies some
>>> of the calling code by removing the need to check for VOIDmode and allows
>>> calling it unconditionally. I also removed element_precision () as it was only
>>> called in one place and thought it neater to call GET_MODE_PRECISION explicitly.
>>>
>>> Parts 2-4 will include further tidy-ups and optimisations based on [1/N].
>>>
>>> Good to go?
>>>
>>> Regards,
>>> David Sherwood.
>>>
>>> 2015-07-17  David Sherwood<david.sherwood@arm.com>
>>>
>>>       gcc/
>>>           * config/arm/arm.c (neon_element_bits, neon_valid_immediate): Call
>>>           GET_MODE_INNER unconditionally.
>>>           * config/spu/spu.c (arith_immediate_p): Likewise.
>>>           * config/i386/i386.c (ix86_build_signbit_mask): Likewise.  New variable.
>>>           * expmed.c (synth_mult): Remove check for VOIDmode result from
>>>           GET_MODE_INNER.
>>>           (expand_mult_const): Likewise.
>>>           * fold-const.c (): Replace call to element_precision with call to
>>>           GET_MODE_PRECISION.
>>>           * genmodes.c (emit_mode_inner_inline): Replace void_mode->name with
>>>           m->name.
>>>           (emit_mode_inner): Likewise.
>>>           * lto-streamer-out.c (lto_write_mode_table): Update GET_MODE_INNER
>>>           result check.
>>>           * machmode.h (GET_MODE_UNIT_SIZE): Simplify.
>>>           (GET_MODE_UNIT_PRECISION): Likewise.
>>>           * rtlanal.c (subreg_get_info): Call GET_MODE_INNER unconditionally.
>>>           * simplify-rtx.c (simplify_immed_subreg): Likewise.
>>>           * stor-layout.c (bitwise_type_for_mode): Update assert.
>>>           (element_precision): Remove.
>> Somehow my brain kept translating INNER into NARROWER.  Naturally I was
>> having considerable trouble seeing how the patch could be correct ;-)
>> Looking at insn-modes.h cleared things up quickly.
>>
>> In a lot of ways this makes GET_INNER_MODE act more like
>> GET_MODE_NUNITS, which is probably good.
>>
>> You need to update the comment for GET_MODE_INNER in machmode.h to
>> reflect the change in its return value for non-vector modes.
> Thanks for the quick response! Before I post a new patch, does this new
> comment seem ok?
>
> /* Where MODE represents a vector return the mode of the inner elements,
> otherwise just return MODE.  */
Yes, that's fine.

Thanks,
jeff

Re: Regression in target MIC compiler (was: nvptx offloading patches [3/n], RFD)

[Ilya Verbin]

On Fri, Jul 31, 2015 at 19:27:58 +0300, Ilya Verbin wrote:
> I've noticed that target MIC compiler from trunk hangs forever in
> lto_input_mode_table in this loop, even on simple testcases.
> 
> On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> +	 if not found, fallback to all modes.  */
> +      int pass;
> +      for (pass = 0; pass < 2; pass++)
> +	for (machine_mode mr = pass ? VOIDmode
> +				    : GET_CLASS_NARROWEST_MODE (mclass);
> +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> +	     pass ? mr = (machine_mode) (m + 1)
> +		  : mr = GET_MODE_WIDER_MODE (mr))
> +	  if (GET_MODE_CLASS (mr) != mclass
> +	      || GET_MODE_SIZE (mr) != size
> +	      || GET_MODE_PRECISION (mr) != prec
> +	      || GET_MODE_INNER (mr) != inner
> +	      || GET_MODE_IBIT (mr) != ibit
> +	      || GET_MODE_FBIT (mr) != fbit
> +	      || GET_MODE_NUNITS (mr) != nunits)
> +	    continue;
> 
> Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> between 9 and 31 Jul.  I'll try to find the revision.

Shouldn't 'mr' be here instead of 'm'?

> mr = (machine_mode) (m + 1)

  -- Ilya

Re: Regression in target MIC compiler (was: nvptx offloading patches [3/n], RFD)

[Jakub Jelinek]

On Fri, Jul 31, 2015 at 07:53:16PM +0300, Ilya Verbin wrote:
> On Fri, Jul 31, 2015 at 19:27:58 +0300, Ilya Verbin wrote:
> > I've noticed that target MIC compiler from trunk hangs forever in
> > lto_input_mode_table in this loop, even on simple testcases.
> > 
> > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > +	 if not found, fallback to all modes.  */
> > +      int pass;
> > +      for (pass = 0; pass < 2; pass++)
> > +	for (machine_mode mr = pass ? VOIDmode
> > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > +	     pass ? mr = (machine_mode) (m + 1)
> > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > +	  if (GET_MODE_CLASS (mr) != mclass
> > +	      || GET_MODE_SIZE (mr) != size
> > +	      || GET_MODE_PRECISION (mr) != prec
> > +	      || GET_MODE_INNER (mr) != inner
> > +	      || GET_MODE_IBIT (mr) != ibit
> > +	      || GET_MODE_FBIT (mr) != fbit
> > +	      || GET_MODE_NUNITS (mr) != nunits)
> > +	    continue;
> > 
> > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > between 9 and 31 Jul.  I'll try to find the revision.
> 
> Shouldn't 'mr' be here instead of 'm'?

I think so.  If it works, patch preapproved.
But wonder what changed that we haven't been triggering it before.
What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?

	Jakub

Re: Regression in target MIC compiler (was: nvptx offloading patches [3/n], RFD)

[Ilya Verbin]

On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> On Fri, Jul 31, 2015 at 07:53:16PM +0300, Ilya Verbin wrote:
> > On Fri, Jul 31, 2015 at 19:27:58 +0300, Ilya Verbin wrote:
> > > I've noticed that target MIC compiler from trunk hangs forever in
> > > lto_input_mode_table in this loop, even on simple testcases.
> > > 
> > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > +	 if not found, fallback to all modes.  */
> > > +      int pass;
> > > +      for (pass = 0; pass < 2; pass++)
> > > +	for (machine_mode mr = pass ? VOIDmode
> > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > +	     pass ? mr = (machine_mode) (m + 1)
> > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > +	      || GET_MODE_SIZE (mr) != size
> > > +	      || GET_MODE_PRECISION (mr) != prec
> > > +	      || GET_MODE_INNER (mr) != inner
> > > +	      || GET_MODE_IBIT (mr) != ibit
> > > +	      || GET_MODE_FBIT (mr) != fbit
> > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > +	    continue;
> > > 
> > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > between 9 and 31 Jul.  I'll try to find the revision.
> > 
> > Shouldn't 'mr' be here instead of 'm'?
> 
> I think so.  If it works, patch preapproved.

It fixes the infinite loop, but causes an error:
lto1: fatal error: unsupported mode QI

> But wonder what changed that we haven't been triggering it before.
> What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?

When in hangs, mr is HImode.

  -- Ilya

Re: Regression in target MIC compiler

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

Hi!

Testing some offloading patches for trunk, I'm encountering the same
problem already reported here:

On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > On Fri, Jul 31, 2015 at 07:53:16PM +0300, Ilya Verbin wrote:
> > > On Fri, Jul 31, 2015 at 19:27:58 +0300, Ilya Verbin wrote:
> > > > I've noticed that target MIC compiler from trunk hangs forever in
> > > > lto_input_mode_table in this loop, even on simple testcases.

Confirmed.

> > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > +	 if not found, fallback to all modes.  */
> > > > +      int pass;
> > > > +      for (pass = 0; pass < 2; pass++)
> > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > +	      || GET_MODE_SIZE (mr) != size
> > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > +	      || GET_MODE_INNER (mr) != inner
> > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > +	    continue;
> > > > 
> > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > 
> > > Shouldn't 'mr' be here instead of 'm'?
> > 
> > I think so.  If it works, patch preapproved.
> 
> It fixes the infinite loop, but causes an error:
> lto1: fatal error: unsupported mode QI

Confirmed.

> > But wonder what changed that we haven't been triggering it before.
> > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> 
> When in hangs, mr is HImode.

Do you already have any further analysis, a workaround, or even a fix?


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: Regression in target MIC compiler

[Ilya Verbin]

On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > +	 if not found, fallback to all modes.  */
> > > > > +      int pass;
> > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > +	    continue;
> > > > > 
> > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > > 
> > > > Shouldn't 'mr' be here instead of 'm'?
> > > 
> > > I think so.  If it works, patch preapproved.
> > 
> > It fixes the infinite loop, but causes an error:
> > lto1: fatal error: unsupported mode QI
> 
> Confirmed.
> 
> > > But wonder what changed that we haven't been triggering it before.
> > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> > 
> > When in hangs, mr is HImode.
> 
> Do you already have any further analysis, a workaround, or even a fix?

Not yet.  I thought since Jakub is the author of this function, he could easily
point what is wrong here :)  Actually, intelmic doesn't require
lto_input_mode_table, so temporary workaround is just to disable it.

  -- Ilya

Re: Regression in target MIC compiler

[Richard Biener]

On Tue, Aug 4, 2015 at 3:06 PM, Ilya Verbin <iverbin@gmail.com> wrote:
> On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
>> On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
>> > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
>> > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
>> > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
>> > > > > +      if not found, fallback to all modes.  */
>> > > > > +      int pass;
>> > > > > +      for (pass = 0; pass < 2; pass++)
>> > > > > +     for (machine_mode mr = pass ? VOIDmode
>> > > > > +                                 : GET_CLASS_NARROWEST_MODE (mclass);
>> > > > > +          pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
>> > > > > +          pass ? mr = (machine_mode) (m + 1)
>> > > > > +               : mr = GET_MODE_WIDER_MODE (mr))
>> > > > > +       if (GET_MODE_CLASS (mr) != mclass
>> > > > > +           || GET_MODE_SIZE (mr) != size
>> > > > > +           || GET_MODE_PRECISION (mr) != prec
>> > > > > +           || GET_MODE_INNER (mr) != inner
>> > > > > +           || GET_MODE_IBIT (mr) != ibit
>> > > > > +           || GET_MODE_FBIT (mr) != fbit
>> > > > > +           || GET_MODE_NUNITS (mr) != nunits)
>> > > > > +         continue;
>> > > > >
>> > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
>> > > > > between 9 and 31 Jul.  I'll try to find the revision.
>> > > >
>> > > > Shouldn't 'mr' be here instead of 'm'?
>> > >
>> > > I think so.  If it works, patch preapproved.

^^^

looks like an obvious error anyway.

Richard.

>> > It fixes the infinite loop, but causes an error:
>> > lto1: fatal error: unsupported mode QI
>>
>> Confirmed.
>>
>> > > But wonder what changed that we haven't been triggering it before.
>> > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
>> >
>> > When in hangs, mr is HImode.
>>
>> Do you already have any further analysis, a workaround, or even a fix?
>
> Not yet.  I thought since Jakub is the author of this function, he could easily
> point what is wrong here :)  Actually, intelmic doesn't require
> lto_input_mode_table, so temporary workaround is just to disable it.
>
>   -- Ilya

Re: Regression in target MIC compiler

[Ilya Verbin]

On Tue, Aug 04, 2015 at 16:07:42 +0200, Richard Biener wrote:
> On Tue, Aug 4, 2015 at 3:06 PM, Ilya Verbin <iverbin@gmail.com> wrote:
> > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> >> On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> >> > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> >> > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> >> > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> >> > > > > +      if not found, fallback to all modes.  */
> >> > > > > +      int pass;
> >> > > > > +      for (pass = 0; pass < 2; pass++)
> >> > > > > +     for (machine_mode mr = pass ? VOIDmode
> >> > > > > +                                 : GET_CLASS_NARROWEST_MODE (mclass);
> >> > > > > +          pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> >> > > > > +          pass ? mr = (machine_mode) (m + 1)
> >> > > > > +               : mr = GET_MODE_WIDER_MODE (mr))
> >> > > > > +       if (GET_MODE_CLASS (mr) != mclass
> >> > > > > +           || GET_MODE_SIZE (mr) != size
> >> > > > > +           || GET_MODE_PRECISION (mr) != prec
> >> > > > > +           || GET_MODE_INNER (mr) != inner
> >> > > > > +           || GET_MODE_IBIT (mr) != ibit
> >> > > > > +           || GET_MODE_FBIT (mr) != fbit
> >> > > > > +           || GET_MODE_NUNITS (mr) != nunits)
> >> > > > > +         continue;
> >> > > > >
> >> > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> >> > > > > between 9 and 31 Jul.  I'll try to find the revision.
> >> > > >
> >> > > > Shouldn't 'mr' be here instead of 'm'?
> >> > >
> >> > > I think so.  If it works, patch preapproved.
> 
> ^^^
> 
> looks like an obvious error anyway.
> 
> Richard.

Yeah, but the fix for this typo doesn't really help, since it exposes another
error in this function.

vvv

> >> > It fixes the infinite loop, but causes an error:
> >> > lto1: fatal error: unsupported mode QI
> >>
> >> Confirmed.
> >>
> >> > > But wonder what changed that we haven't been triggering it before.
> >> > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> >> >
> >> > When in hangs, mr is HImode.
> >>
> >> Do you already have any further analysis, a workaround, or even a fix?
> >
> > Not yet.  I thought since Jakub is the author of this function, he could easily
> > point what is wrong here :)  Actually, intelmic doesn't require
> > lto_input_mode_table, so temporary workaround is just to disable it.

  -- Ilya

Re: Regression in target MIC compiler

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 4221 bytes --]

Hi!

It seems as if David's »[PATCH][1/N] Change GET_MODE_INNER to always
return a non-void mode« is relevant here:

On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > > +	 if not found, fallback to all modes.  */
> > > > > > +      int pass;
> > > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > > +	    continue;
> > > > > > 
> > > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > > > 
> > > > > Shouldn't 'mr' be here instead of 'm'?
> > > > 
> > > > I think so.  If it works, patch preapproved.
> > > 
> > > It fixes the infinite loop, but causes an error:
> > > lto1: fatal error: unsupported mode QI
> > 
> > Confirmed.
> > 
> > > > But wonder what changed that we haven't been triggering it before.
> > > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> > > 
> > > When in hangs, mr is HImode.
> > 
> > Do you already have any further analysis, a workaround, or even a fix?
> 
> Not yet.  I thought since Jakub is the author of this function, he could easily
> point what is wrong here :)  Actually, intelmic doesn't require
> lto_input_mode_table, so temporary workaround is just to disable it.

Well, avoiding lto_input_mode_table doesn't help us with nvptx
offloading...  ;-)

Anyway, I found that if I revert r226328, »[PATCH][1/N] Change
GET_MODE_INNER to always return a non-void mode«,
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02350.html>, the problem
goes away.  I'm trying, but I cannot claim yet to really understand this
mode streaming code...  But, with the producer
gcc/lto-streamer-out.c:lto_write_mode_table having been changed, does
maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also need
to be updated accordingly?

For reference, David's change to
gcc/lto-streamer-out.c:lto_write_mode_table:

@@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
   /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
      also the inner mode marked.  */
   for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
     if (streamer_mode_table[i])
       {
 	machine_mode m = (machine_mode) i;
-	if (GET_MODE_INNER (m) != VOIDmode)
+	if (GET_MODE_INNER (m) != m)
 	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
       }
   /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
      so that we can refer to them afterwards.  */
   for (int pass = 0; pass < 2; pass++)
     for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
       if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
 	{
 	  machine_mode m = (machine_mode) i;
-	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
+	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
 	    continue;
 	  bp_pack_value (&bp, m, 8);
 	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
 	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
 	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
 	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);

(Also, the source code comments need to be updated?)


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

RE: Regression in target MIC compiler

[David Sherwood]

Hi Thomas,

If this looks like my fault I am happy to look into this and fix the bug
if you can tell me how to reproduce it. I recently changed GET_MODE_INNER (m)
to return 'm' itself if there is no inner mode and I thought I'd fixed up lto,
but it seems I got it wrong. It also sounds like there is another bug in this
area too - if I want to test this do I need to apply any other patches too?

Regards,
David.

> 
> Hi!
> 
> It seems as if David's »[PATCH][1/N] Change GET_MODE_INNER to always
> return a non-void mode« is relevant here:
> 
> On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> > > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > > > +	 if not found, fallback to all modes.  */
> > > > > > > +      int pass;
> > > > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > > > +	    continue;
> > > > > > >
> > > > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > > > >
> > > > > > Shouldn't 'mr' be here instead of 'm'?
> > > > >
> > > > > I think so.  If it works, patch preapproved.
> > > >
> > > > It fixes the infinite loop, but causes an error:
> > > > lto1: fatal error: unsupported mode QI
> > >
> > > Confirmed.
> > >
> > > > > But wonder what changed that we haven't been triggering it before.
> > > > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> > > >
> > > > When in hangs, mr is HImode.
> > >
> > > Do you already have any further analysis, a workaround, or even a fix?
> >
> > Not yet.  I thought since Jakub is the author of this function, he could easily
> > point what is wrong here :)  Actually, intelmic doesn't require
> > lto_input_mode_table, so temporary workaround is just to disable it.
> 
> Well, avoiding lto_input_mode_table doesn't help us with nvptx
> offloading...  ;-)
> 
> Anyway, I found that if I revert r226328, »[PATCH][1/N] Change
> GET_MODE_INNER to always return a non-void mode«,
> <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02350.html>, the problem
> goes away.  I'm trying, but I cannot claim yet to really understand this
> mode streaming code...  But, with the producer
> gcc/lto-streamer-out.c:lto_write_mode_table having been changed, does
> maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also need
> to be updated accordingly?
> 
> For reference, David's change to
> gcc/lto-streamer-out.c:lto_write_mode_table:
> 
> @@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
>    /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
>       also the inner mode marked.  */
>    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
>      if (streamer_mode_table[i])
>        {
>  	machine_mode m = (machine_mode) i;
> -	if (GET_MODE_INNER (m) != VOIDmode)
> +	if (GET_MODE_INNER (m) != m)
>  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
>        }
>    /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
>       so that we can refer to them afterwards.  */
>    for (int pass = 0; pass < 2; pass++)
>      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
>        if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
>  	{
>  	  machine_mode m = (machine_mode) i;
> -	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
> +	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
>  	    continue;
>  	  bp_pack_value (&bp, m, 8);
>  	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
>  	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
>  	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
>  	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);
> 
> (Also, the source code comments need to be updated?)
> 
> 
> Grüße,
>  Thomas

RE: Regression in target MIC compiler

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 6020 bytes --]

Hi!

On Wed, 5 Aug 2015 11:18:32 +0100, David Sherwood <david.sherwood@arm.com> wrote:
> If this looks like my fault

Well, not necessarily your fault -- might as well just be something that
has already been lurking in gcc/lto-streamer-in.c:lto_input_mode_table,
but so far we've gotten away without tripping over it.

> I am happy to look into this and fix the bug

Thanks for helping!

> if you can tell me how to reproduce it. I recently changed GET_MODE_INNER (m)
> to return 'm' itself if there is no inner mode and I thought I'd fixed up lto,
> but it seems I got it wrong. It also sounds like there is another bug in this
> area too - if I want to test this do I need to apply any other patches too?

gcc/lto-streamer-out.c:lto_write_mode_table as well as
gcc/lto-streamer-in.c:lto_input_mode_table are not used in regular LTO,
but are only used in offloading configurations,
<https://gcc.gnu.org/wiki/Offloading>.  To reproduce this, you'd build
such a configuration (offloading to x86_64-intelmicemul-linux-gnu is
easier to build than nvptx-none),
<https://gcc.gnu.org/wiki/Offloading#How_to_try_offloading_enabled_GCC>.
You can use the build scripts I uploaded, or do the steps manually.
Running the libgomp testsuite, then observe, for example,
libgomp.c/examples-4/array_sections-3.c hang (or, fail with »unsupported
mode QI« with the mr vs. m confusion fixed, see below).

I'm happy to test any patches or also hypotheses that you suggest --
maybe something is obvious to you just from looking at the code?


For reference:

> > On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> > > > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > > > > +	 if not found, fallback to all modes.  */
> > > > > > > > +      int pass;
> > > > > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > > > > +	    continue;
> > > > > > > >
> > > > > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > > > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > > > > >
> > > > > > > Shouldn't 'mr' be here instead of 'm'?
> > > > > >
> > > > > > I think so.  If it works, patch preapproved.
> > > > >
> > > > > It fixes the infinite loop, but causes an error:
> > > > > lto1: fatal error: unsupported mode QI
> > > >
> > > > Confirmed.
> > > >
> > > > > > But wonder what changed that we haven't been triggering it before.
> > > > > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> > > > >
> > > > > When in hangs, mr is HImode.
> > > >
> > > > Do you already have any further analysis, a workaround, or even a fix?
> > >
> > > Not yet.  I thought since Jakub is the author of this function, he could easily
> > > point what is wrong here :)  Actually, intelmic doesn't require
> > > lto_input_mode_table, so temporary workaround is just to disable it.
> > 
> > Well, avoiding lto_input_mode_table doesn't help us with nvptx
> > offloading...  ;-)
> > 
> > Anyway, I found that if I revert r226328, »[PATCH][1/N] Change
> > GET_MODE_INNER to always return a non-void mode«,
> > <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02350.html>, the problem
> > goes away.  I'm trying, but I cannot claim yet to really understand this
> > mode streaming code...  But, with the producer
> > gcc/lto-streamer-out.c:lto_write_mode_table having been changed, does
> > maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also need
> > to be updated accordingly?
> > 
> > For reference, David's change to
> > gcc/lto-streamer-out.c:lto_write_mode_table:
> > 
> > @@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
> >    /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
> >       also the inner mode marked.  */
> >    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> >      if (streamer_mode_table[i])
> >        {
> >  	machine_mode m = (machine_mode) i;
> > -	if (GET_MODE_INNER (m) != VOIDmode)
> > +	if (GET_MODE_INNER (m) != m)
> >  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
> >        }
> >    /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
> >       so that we can refer to them afterwards.  */
> >    for (int pass = 0; pass < 2; pass++)
> >      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> >        if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
> >  	{
> >  	  machine_mode m = (machine_mode) i;
> > -	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
> > +	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
> >  	    continue;
> >  	  bp_pack_value (&bp, m, 8);
> >  	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
> >  	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
> >  	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
> >  	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);
> > 
> > (Also, the source code comments need to be updated?)


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

RE: Regression in target MIC compiler

[David Sherwood]

Hi Thomas,

In lto_input_mode_table there is the following line of code:

machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];

Is this right? In lto_write_mode_table this inner mode is written out explicitly
into the stream already, so do we just need this instead?

machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);

It's possible I'm misunderstanding the code somehow though ...

Regards,
David.

> -----Original Message-----
> From: Thomas Schwinge [mailto:thomas@codesourcery.com]
> Sent: 05 August 2015 11:46
> To: David Sherwood
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Kirill Yukhin; nathan@codesourcery.com; Richard Sandiford;
> Ilya Verbin; Jeff Law
> Subject: RE: Regression in target MIC compiler
> 
> Hi!
> 
> On Wed, 5 Aug 2015 11:18:32 +0100, David Sherwood <david.sherwood@arm.com> wrote:
> > If this looks like my fault
> 
> Well, not necessarily your fault -- might as well just be something that
> has already been lurking in gcc/lto-streamer-in.c:lto_input_mode_table,
> but so far we've gotten away without tripping over it.
> 
> > I am happy to look into this and fix the bug
> 
> Thanks for helping!
> 
> > if you can tell me how to reproduce it. I recently changed GET_MODE_INNER (m)
> > to return 'm' itself if there is no inner mode and I thought I'd fixed up lto,
> > but it seems I got it wrong. It also sounds like there is another bug in this
> > area too - if I want to test this do I need to apply any other patches too?
> 
> gcc/lto-streamer-out.c:lto_write_mode_table as well as
> gcc/lto-streamer-in.c:lto_input_mode_table are not used in regular LTO,
> but are only used in offloading configurations,
> <https://gcc.gnu.org/wiki/Offloading>.  To reproduce this, you'd build
> such a configuration (offloading to x86_64-intelmicemul-linux-gnu is
> easier to build than nvptx-none),
> <https://gcc.gnu.org/wiki/Offloading#How_to_try_offloading_enabled_GCC>.
> You can use the build scripts I uploaded, or do the steps manually.
> Running the libgomp testsuite, then observe, for example,
> libgomp.c/examples-4/array_sections-3.c hang (or, fail with »unsupported
> mode QI« with the mr vs. m confusion fixed, see below).
> 
> I'm happy to test any patches or also hypotheses that you suggest --
> maybe something is obvious to you just from looking at the code?
> 
> 
> For reference:
> 
> > > On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> > > > > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > > > > > +	 if not found, fallback to all modes.  */
> > > > > > > > > +      int pass;
> > > > > > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > > > > > +	    continue;
> > > > > > > > >
> > > > > > > > > Given that gomp-4_1-branch works ok, the problem was introduced somewhere
> > > > > > > > > between 9 and 31 Jul.  I'll try to find the revision.
> > > > > > > >
> > > > > > > > Shouldn't 'mr' be here instead of 'm'?
> > > > > > >
> > > > > > > I think so.  If it works, patch preapproved.
> > > > > >
> > > > > > It fixes the infinite loop, but causes an error:
> > > > > > lto1: fatal error: unsupported mode QI
> > > > >
> > > > > Confirmed.
> > > > >
> > > > > > > But wonder what changed that we haven't been triggering it before.
> > > > > > > What mode do you think it on (mclass/size/prec/inner/ibit/fbit/nunits)?
> > > > > >
> > > > > > When in hangs, mr is HImode.
> > > > >
> > > > > Do you already have any further analysis, a workaround, or even a fix?
> > > >
> > > > Not yet.  I thought since Jakub is the author of this function, he could easily
> > > > point what is wrong here :)  Actually, intelmic doesn't require
> > > > lto_input_mode_table, so temporary workaround is just to disable it.
> > >
> > > Well, avoiding lto_input_mode_table doesn't help us with nvptx
> > > offloading...  ;-)
> > >
> > > Anyway, I found that if I revert r226328, »[PATCH][1/N] Change
> > > GET_MODE_INNER to always return a non-void mode«,
> > > <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02350.html>, the problem
> > > goes away.  I'm trying, but I cannot claim yet to really understand this
> > > mode streaming code...  But, with the producer
> > > gcc/lto-streamer-out.c:lto_write_mode_table having been changed, does
> > > maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also need
> > > to be updated accordingly?
> > >
> > > For reference, David's change to
> > > gcc/lto-streamer-out.c:lto_write_mode_table:
> > >
> > > @@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
> > >    /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
> > >       also the inner mode marked.  */
> > >    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> > >      if (streamer_mode_table[i])
> > >        {
> > >  	machine_mode m = (machine_mode) i;
> > > -	if (GET_MODE_INNER (m) != VOIDmode)
> > > +	if (GET_MODE_INNER (m) != m)
> > >  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
> > >        }
> > >    /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
> > >       so that we can refer to them afterwards.  */
> > >    for (int pass = 0; pass < 2; pass++)
> > >      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> > >        if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
> > >  	{
> > >  	  machine_mode m = (machine_mode) i;
> > > -	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
> > > +	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
> > >  	    continue;
> > >  	  bp_pack_value (&bp, m, 8);
> > >  	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
> > >  	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
> > >  	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
> > >  	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);
> > >
> > > (Also, the source code comments need to be updated?)
> 
> 
> Grüße,
>  Thomas

RE: Regression in target MIC compiler

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 271 bytes --]

Hi!

On Wed, 5 Aug 2015 15:10:40 +0100, David Sherwood <david.sherwood@arm.com> wrote:
> In lto_input_mode_table there is the following line of code: [...]

Thanks!  That's not exactly it, but you put me on the right track.
Testing a patch.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Fix offloading machine mode stream reading (was: Regression in target MIC compiler)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 8667 bytes --]

Hi!

On Wed, 5 Aug 2015 15:10:40 +0100, "David Sherwood" <david.sherwood@arm.com> wrote:
> In lto_input_mode_table there is the following line of code:
> 
> machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];
> 
> Is this right? In lto_write_mode_table this inner mode is written out explicitly
> into the stream already, so do we just need this instead?
> 
> machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
> 
> It's possible I'm misunderstanding the code somehow though ...

The idea here is to translate between the machine mode IDs used by the
target and the offloading compiler(s), whence the table lookup, or table
construction in the first place.  But as I said yesterday, you gave me a
clue where to look, and the problem is that given your GET_MODE_INNER
change:

> > From: Thomas Schwinge [mailto:thomas@codesourcery.com]
> > On Wed, 5 Aug 2015 11:18:32 +0100, David Sherwood <david.sherwood@arm.com> wrote:
> > > I recently changed GET_MODE_INNER (m)
> > > to return 'm' itself if there is no inner mode

... the following code from gcc/lto-streamer-in.c:lto_input_mode_table:

> > > > On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
> > > > > > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> > > > > > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
> > > > > > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek wrote:
> > > > > > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE to wider modes,
> > > > > > > > > > +	 if not found, fallback to all modes.  */
> > > > > > > > > > +      int pass;
> > > > > > > > > > +      for (pass = 0; pass < 2; pass++)
> > > > > > > > > > +	for (machine_mode mr = pass ? VOIDmode
> > > > > > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
> > > > > > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
> > > > > > > > > > +	     pass ? mr = (machine_mode) (m + 1)
> > > > > > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
> > > > > > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
> > > > > > > > > > +	      || GET_MODE_SIZE (mr) != size
> > > > > > > > > > +	      || GET_MODE_PRECISION (mr) != prec
> > > > > > > > > > +	      || GET_MODE_INNER (mr) != inner
> > > > > > > > > > +	      || GET_MODE_IBIT (mr) != ibit
> > > > > > > > > > +	      || GET_MODE_FBIT (mr) != fbit
> > > > > > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
> > > > > > > > > > +	    continue;

... no longer does the right thing in the »GET_MODE_INNER (mr) != inner«
comparison.

> > > > I'm trying, but I cannot claim yet to really understand this
> > > > mode streaming code...

;-) Now that I do...

> > > > But, with the producer
> > > > gcc/lto-streamer-out.c:lto_write_mode_table having been changed, does
> > > > maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also need
> > > > to be updated accordingly?
> > > >
> > > > For reference, David's change to
> > > > gcc/lto-streamer-out.c:lto_write_mode_table:
> > > >
> > > > @@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
> > > >    /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
> > > >       also the inner mode marked.  */
> > > >    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> > > >      if (streamer_mode_table[i])
> > > >        {
> > > >  	machine_mode m = (machine_mode) i;
> > > > -	if (GET_MODE_INNER (m) != VOIDmode)
> > > > +	if (GET_MODE_INNER (m) != m)
> > > >  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
> > > >        }
> > > >    /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
> > > >       so that we can refer to them afterwards.  */
> > > >    for (int pass = 0; pass < 2; pass++)
> > > >      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> > > >        if (streamer_mode_table[i] && i != (int) VOIDmode && i != (int) BLKmode)
> > > >  	{
> > > >  	  machine_mode m = (machine_mode) i;
> > > > -	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
> > > > +	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
> > > >  	    continue;
> > > >  	  bp_pack_value (&bp, m, 8);
> > > >  	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m));
> > > >  	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
> > > >  	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
> > > >  	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);

... I came up with the following patch to fix the offloading machine mode
stream reading.  OK to commit?

commit 45264b009e988298fddab5417e12d36e2edeeb49
Author: Thomas Schwinge <thomas@codesourcery.com>
Date:   Thu Aug 6 12:00:01 2015 +0200

    Fix offloading machine mode stream reading
    
    ... in context of the GET_MODE_INNER changes applied in r226328.
    
    	gcc/
    	* lto-streamer-in.c (lto_input_mode_table): Adjust to
    	GET_MODE_INNER changes.
    	libgomp/
    	* testsuite/libgomp.oacc-c-c++-common/vector-type-1.c: New file.
---
 gcc/lto-streamer-in.c                              |    8 ++++---
 gcc/lto-streamer-out.c                             |    4 ++--
 .../libgomp.oacc-c-c++-common/vector-type-1.c      |   24 ++++++++++++++++++++
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git gcc/lto-streamer-in.c gcc/lto-streamer-in.c
index 299900a..2eb8051 100644
--- gcc/lto-streamer-in.c
+++ gcc/lto-streamer-in.c
@@ -1544,7 +1544,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	= bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS);
       unsigned int size = bp_unpack_value (&bp, 8);
       unsigned int prec = bp_unpack_value (&bp, 16);
-      machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];
+      machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
       unsigned int nunits = bp_unpack_value (&bp, 8);
       unsigned int ibit = 0, fbit = 0;
       unsigned int real_fmt_len = 0;
@@ -1578,7 +1578,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	  if (GET_MODE_CLASS (mr) != mclass
 	      || GET_MODE_SIZE (mr) != size
 	      || GET_MODE_PRECISION (mr) != prec
-	      || GET_MODE_INNER (mr) != inner
+	      || (inner == m
+		  ? GET_MODE_INNER (mr) != mr
+		  : GET_MODE_INNER (mr) != table[(int) inner])
 	      || GET_MODE_IBIT (mr) != ibit
 	      || GET_MODE_FBIT (mr) != fbit
 	      || GET_MODE_NUNITS (mr) != nunits)
@@ -1606,7 +1608,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	    case MODE_VECTOR_UACCUM:
 	      /* For unsupported vector modes just use BLKmode,
 		 if the scalar mode is supported.  */
-	      if (inner != VOIDmode)
+	      if (table[(int) inner] != VOIDmode)
 		{
 		  table[m] = BLKmode;
 		  break;

> > > > (Also, the source code comments need to be updated?)

diff --git gcc/lto-streamer-out.c gcc/lto-streamer-out.c
index 1b88115..3ca8855 100644
--- gcc/lto-streamer-out.c
+++ gcc/lto-streamer-out.c
@@ -2676,7 +2676,7 @@ lto_write_mode_table (void)
   ob = create_output_block (LTO_section_mode_table);
   bitpack_d bp = bitpack_create (ob->main_stream);
 
-  /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
+  /* Ensure that for GET_MODE_INNER (m) != m we have
      also the inner mode marked.  */
   for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
     if (streamer_mode_table[i])
@@ -2685,7 +2685,7 @@ lto_write_mode_table (void)
 	if (GET_MODE_INNER (m) != m)
 	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
       }
-  /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
+  /* First stream modes that have GET_MODE_INNER (m) == m,
      so that we can refer to them afterwards.  */
   for (int pass = 0; pass < 2; pass++)
     for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
diff --git libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
new file mode 100644
index 0000000..5adfcec
--- /dev/null
+++ libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
@@ -0,0 +1,24 @@
+#define vector __attribute__ ((vector_size (4 * sizeof(int))))
+
+int main(void)
+{
+  vector int vi = { 12, -34, -56, 78 };
+
+#pragma acc parallel copy(vi)
+  {
+    if (vi[0] != 12
+	|| vi[1] != -34
+	|| vi[2] != -56
+	|| vi[3] != 78)
+      __builtin_abort();
+    vector int vi_ = { -21, -43, 65, 87 };
+    vi = vi_;
+  }
+  if (vi[0] != -21
+      || vi[1] != -43
+      || vi[2] != 65
+      || vi[3] != 87)
+    __builtin_abort();
+
+  return 0;
+}


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: Fix offloading machine mode stream reading (was: Regression in target MIC compiler)

[Richard Biener]

Ok.

Richard.

On 8/6/15, Thomas Schwinge <thomas@codesourcery.com> wrote:
> Hi!
>
> On Wed, 5 Aug 2015 15:10:40 +0100, "David Sherwood" <david.sherwood@arm.com>
> wrote:
>> In lto_input_mode_table there is the following line of code:
>>
>> machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];
>>
>> Is this right? In lto_write_mode_table this inner mode is written out
>> explicitly
>> into the stream already, so do we just need this instead?
>>
>> machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
>>
>> It's possible I'm misunderstanding the code somehow though ...
>
> The idea here is to translate between the machine mode IDs used by the
> target and the offloading compiler(s), whence the table lookup, or table
> construction in the first place.  But as I said yesterday, you gave me a
> clue where to look, and the problem is that given your GET_MODE_INNER
> change:
>
>> > From: Thomas Schwinge [mailto:thomas@codesourcery.com]
>> > On Wed, 5 Aug 2015 11:18:32 +0100, David Sherwood
>> > <david.sherwood@arm.com> wrote:
>> > > I recently changed GET_MODE_INNER (m)
>> > > to return 'm' itself if there is no inner mode
>
> ... the following code from gcc/lto-streamer-in.c:lto_input_mode_table:
>
>> > > > On Tue, 4 Aug 2015 16:06:23 +0300, Ilya Verbin <iverbin@gmail.com>
>> > > > wrote:
>> > > > > On Tue, Aug 04, 2015 at 14:35:11 +0200, Thomas Schwinge wrote:
>> > > > > > On Fri, 31 Jul 2015 20:13:02 +0300, Ilya Verbin
>> > > > > > <iverbin@gmail.com> wrote:
>> > > > > > > On Fri, Jul 31, 2015 at 18:59:59 +0200, Jakub Jelinek wrote:
>> > > > > > > > > > On Wed, Feb 18, 2015 at 11:00:35 +0100, Jakub Jelinek
>> > > > > > > > > > wrote:
>> > > > > > > > > > +      /* First search just the GET_CLASS_NARROWEST_MODE
>> > > > > > > > > > to wider modes,
>> > > > > > > > > > +	 if not found, fallback to all modes.  */
>> > > > > > > > > > +      int pass;
>> > > > > > > > > > +      for (pass = 0; pass < 2; pass++)
>> > > > > > > > > > +	for (machine_mode mr = pass ? VOIDmode
>> > > > > > > > > > +				    : GET_CLASS_NARROWEST_MODE (mclass);
>> > > > > > > > > > +	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
>> > > > > > > > > > +	     pass ? mr = (machine_mode) (m + 1)
>> > > > > > > > > > +		  : mr = GET_MODE_WIDER_MODE (mr))
>> > > > > > > > > > +	  if (GET_MODE_CLASS (mr) != mclass
>> > > > > > > > > > +	      || GET_MODE_SIZE (mr) != size
>> > > > > > > > > > +	      || GET_MODE_PRECISION (mr) != prec
>> > > > > > > > > > +	      || GET_MODE_INNER (mr) != inner
>> > > > > > > > > > +	      || GET_MODE_IBIT (mr) != ibit
>> > > > > > > > > > +	      || GET_MODE_FBIT (mr) != fbit
>> > > > > > > > > > +	      || GET_MODE_NUNITS (mr) != nunits)
>> > > > > > > > > > +	    continue;
>
> ... no longer does the right thing in the »GET_MODE_INNER (mr) != inner«
> comparison.
>
>> > > > I'm trying, but I cannot claim yet to really understand this
>> > > > mode streaming code...
>
> ;-) Now that I do...
>
>> > > > But, with the producer
>> > > > gcc/lto-streamer-out.c:lto_write_mode_table having been changed,
>> > > > does
>> > > > maybe the consumer gcc/lto-streamer-in.c:lto_input_mode_table also
>> > > > need
>> > > > to be updated accordingly?
>> > > >
>> > > > For reference, David's change to
>> > > > gcc/lto-streamer-out.c:lto_write_mode_table:
>> > > >
>> > > > @@ -2679,23 +2679,23 @@ lto_write_mode_table (void)
>> > > >    /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
>> > > >       also the inner mode marked.  */
>> > > >    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
>> > > >      if (streamer_mode_table[i])
>> > > >        {
>> > > >  	machine_mode m = (machine_mode) i;
>> > > > -	if (GET_MODE_INNER (m) != VOIDmode)
>> > > > +	if (GET_MODE_INNER (m) != m)
>> > > >  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
>> > > >        }
>> > > >    /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
>> > > >       so that we can refer to them afterwards.  */
>> > > >    for (int pass = 0; pass < 2; pass++)
>> > > >      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
>> > > >        if (streamer_mode_table[i] && i != (int) VOIDmode && i !=
>> > > > (int) BLKmode)
>> > > >  	{
>> > > >  	  machine_mode m = (machine_mode) i;
>> > > > -	  if ((GET_MODE_INNER (m) == VOIDmode) ^ (pass == 0))
>> > > > +	  if ((GET_MODE_INNER (m) == m) ^ (pass == 0))
>> > > >  	    continue;
>> > > >  	  bp_pack_value (&bp, m, 8);
>> > > >  	  bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS
>> > > > (m));
>> > > >  	  bp_pack_value (&bp, GET_MODE_SIZE (m), 8);
>> > > >  	  bp_pack_value (&bp, GET_MODE_PRECISION (m), 16);
>> > > >  	  bp_pack_value (&bp, GET_MODE_INNER (m), 8);
>
> ... I came up with the following patch to fix the offloading machine mode
> stream reading.  OK to commit?
>
> commit 45264b009e988298fddab5417e12d36e2edeeb49
> Author: Thomas Schwinge <thomas@codesourcery.com>
> Date:   Thu Aug 6 12:00:01 2015 +0200
>
>     Fix offloading machine mode stream reading
>
>     ... in context of the GET_MODE_INNER changes applied in r226328.
>
>     	gcc/
>     	* lto-streamer-in.c (lto_input_mode_table): Adjust to
>     	GET_MODE_INNER changes.
>     	libgomp/
>     	* testsuite/libgomp.oacc-c-c++-common/vector-type-1.c: New file.
> ---
>  gcc/lto-streamer-in.c                              |    8 ++++---
>  gcc/lto-streamer-out.c                             |    4 ++--
>  .../libgomp.oacc-c-c++-common/vector-type-1.c      |   24
> ++++++++++++++++++++
>  3 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git gcc/lto-streamer-in.c gcc/lto-streamer-in.c
> index 299900a..2eb8051 100644
> --- gcc/lto-streamer-in.c
> +++ gcc/lto-streamer-in.c
> @@ -1544,7 +1544,7 @@ lto_input_mode_table (struct lto_file_decl_data
> *file_data)
>  	= bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS);
>        unsigned int size = bp_unpack_value (&bp, 8);
>        unsigned int prec = bp_unpack_value (&bp, 16);
> -      machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];
> +      machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
>        unsigned int nunits = bp_unpack_value (&bp, 8);
>        unsigned int ibit = 0, fbit = 0;
>        unsigned int real_fmt_len = 0;
> @@ -1578,7 +1578,9 @@ lto_input_mode_table (struct lto_file_decl_data
> *file_data)
>  	  if (GET_MODE_CLASS (mr) != mclass
>  	      || GET_MODE_SIZE (mr) != size
>  	      || GET_MODE_PRECISION (mr) != prec
> -	      || GET_MODE_INNER (mr) != inner
> +	      || (inner == m
> +		  ? GET_MODE_INNER (mr) != mr
> +		  : GET_MODE_INNER (mr) != table[(int) inner])
>  	      || GET_MODE_IBIT (mr) != ibit
>  	      || GET_MODE_FBIT (mr) != fbit
>  	      || GET_MODE_NUNITS (mr) != nunits)
> @@ -1606,7 +1608,7 @@ lto_input_mode_table (struct lto_file_decl_data
> *file_data)
>  	    case MODE_VECTOR_UACCUM:
>  	      /* For unsupported vector modes just use BLKmode,
>  		 if the scalar mode is supported.  */
> -	      if (inner != VOIDmode)
> +	      if (table[(int) inner] != VOIDmode)
>  		{
>  		  table[m] = BLKmode;
>  		  break;
>
>> > > > (Also, the source code comments need to be updated?)
>
> diff --git gcc/lto-streamer-out.c gcc/lto-streamer-out.c
> index 1b88115..3ca8855 100644
> --- gcc/lto-streamer-out.c
> +++ gcc/lto-streamer-out.c
> @@ -2676,7 +2676,7 @@ lto_write_mode_table (void)
>    ob = create_output_block (LTO_section_mode_table);
>    bitpack_d bp = bitpack_create (ob->main_stream);
>
> -  /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
> +  /* Ensure that for GET_MODE_INNER (m) != m we have
>       also the inner mode marked.  */
>    for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
>      if (streamer_mode_table[i])
> @@ -2685,7 +2685,7 @@ lto_write_mode_table (void)
>  	if (GET_MODE_INNER (m) != m)
>  	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
>        }
> -  /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
> +  /* First stream modes that have GET_MODE_INNER (m) == m,
>       so that we can refer to them afterwards.  */
>    for (int pass = 0; pass < 2; pass++)
>      for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
> diff --git libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
> libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
> new file mode 100644
> index 0000000..5adfcec
> --- /dev/null
> +++ libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
> @@ -0,0 +1,24 @@
> +#define vector __attribute__ ((vector_size (4 * sizeof(int))))
> +
> +int main(void)
> +{
> +  vector int vi = { 12, -34, -56, 78 };
> +
> +#pragma acc parallel copy(vi)
> +  {
> +    if (vi[0] != 12
> +	|| vi[1] != -34
> +	|| vi[2] != -56
> +	|| vi[3] != 78)
> +      __builtin_abort();
> +    vector int vi_ = { -21, -43, 65, 87 };
> +    vi = vi_;
> +  }
> +  if (vi[0] != -21
> +      || vi[1] != -43
> +      || vi[2] != 65
> +      || vi[3] != 87)
> +    __builtin_abort();
> +
> +  return 0;
> +}
>
>
> Grüße,
>  Thomas
>

Re: Fix offloading machine mode stream reading

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 6772 bytes --]

Hi!

On Sat, 8 Aug 2015 07:25:42 +0200, Richard Biener <richard.guenther@gmail.com> wrote:
> Ok.

Committed in r226758 and r226759:

commit 7231f6b984806cceb30cacf0e79f8f5ae7a68803
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Mon Aug 10 15:22:24 2015 +0000

    Correctly advance iterator in offloading machine mode stream reading
    
    	gcc/
    	* lto-streamer-in.c (lto_input_mode_table): Correctly advance
    	iterator.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@226758 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog         |    6 ++++++
 gcc/lto-streamer-in.c |    2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git gcc/ChangeLog gcc/ChangeLog
index f103d41..c51aaf9 100644
--- gcc/ChangeLog
+++ gcc/ChangeLog
@@ -1,3 +1,9 @@
+2015-08-10  Thomas Schwinge  <thomas@codesourcery.com>
+	    Ilya Verbin  <ilya.verbin@intel.com>
+
+	* lto-streamer-in.c (lto_input_mode_table): Correctly advance
+	iterator.
+
 2015-08-09  Manuel López-Ibáñez  <manu@gcc.gnu.org>
 
 	* doc/options.texi (EnabledBy): Document that the argument must be
diff --git gcc/lto-streamer-in.c gcc/lto-streamer-in.c
index a56d3f3..299900a 100644
--- gcc/lto-streamer-in.c
+++ gcc/lto-streamer-in.c
@@ -1573,7 +1573,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	for (machine_mode mr = pass ? VOIDmode
 				    : GET_CLASS_NARROWEST_MODE (mclass);
 	     pass ? mr < MAX_MACHINE_MODE : mr != VOIDmode;
-	     pass ? mr = (machine_mode) (m + 1)
+	     pass ? mr = (machine_mode) (mr + 1)
 		  : mr = GET_MODE_WIDER_MODE (mr))
 	  if (GET_MODE_CLASS (mr) != mclass
 	      || GET_MODE_SIZE (mr) != size

commit b308f4a0d03e67bdaf3f43416cfbd360db957a29
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Mon Aug 10 15:22:30 2015 +0000

    Fix offloading machine mode stream reading
    
    ... in context of the GET_MODE_INNER changes applied in r226328.
    
    	gcc/
    	* lto-streamer-in.c (lto_input_mode_table): Adjust to
    	GET_MODE_INNER changes.
    	libgomp/
    	* testsuite/libgomp.oacc-c-c++-common/vector-type-1.c: New file.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@226759 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog                                      |    5 ++++
 gcc/lto-streamer-in.c                              |    8 ++++---
 gcc/lto-streamer-out.c                             |    4 ++--
 libgomp/ChangeLog                                  |    4 ++++
 .../libgomp.oacc-c-c++-common/vector-type-1.c      |   24 ++++++++++++++++++++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git gcc/ChangeLog gcc/ChangeLog
index c51aaf9..f547931 100644
--- gcc/ChangeLog
+++ gcc/ChangeLog
@@ -1,4 +1,9 @@
 2015-08-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* lto-streamer-in.c (lto_input_mode_table): Adjust to
+	GET_MODE_INNER changes.
+
+2015-08-10  Thomas Schwinge  <thomas@codesourcery.com>
 	    Ilya Verbin  <ilya.verbin@intel.com>
 
 	* lto-streamer-in.c (lto_input_mode_table): Correctly advance
diff --git gcc/lto-streamer-in.c gcc/lto-streamer-in.c
index 299900a..2eb8051 100644
--- gcc/lto-streamer-in.c
+++ gcc/lto-streamer-in.c
@@ -1544,7 +1544,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	= bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS);
       unsigned int size = bp_unpack_value (&bp, 8);
       unsigned int prec = bp_unpack_value (&bp, 16);
-      machine_mode inner = (machine_mode) table[bp_unpack_value (&bp, 8)];
+      machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
       unsigned int nunits = bp_unpack_value (&bp, 8);
       unsigned int ibit = 0, fbit = 0;
       unsigned int real_fmt_len = 0;
@@ -1578,7 +1578,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	  if (GET_MODE_CLASS (mr) != mclass
 	      || GET_MODE_SIZE (mr) != size
 	      || GET_MODE_PRECISION (mr) != prec
-	      || GET_MODE_INNER (mr) != inner
+	      || (inner == m
+		  ? GET_MODE_INNER (mr) != mr
+		  : GET_MODE_INNER (mr) != table[(int) inner])
 	      || GET_MODE_IBIT (mr) != ibit
 	      || GET_MODE_FBIT (mr) != fbit
 	      || GET_MODE_NUNITS (mr) != nunits)
@@ -1606,7 +1608,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
 	    case MODE_VECTOR_UACCUM:
 	      /* For unsupported vector modes just use BLKmode,
 		 if the scalar mode is supported.  */
-	      if (inner != VOIDmode)
+	      if (table[(int) inner] != VOIDmode)
 		{
 		  table[m] = BLKmode;
 		  break;
diff --git gcc/lto-streamer-out.c gcc/lto-streamer-out.c
index 1b88115..3ca8855 100644
--- gcc/lto-streamer-out.c
+++ gcc/lto-streamer-out.c
@@ -2676,7 +2676,7 @@ lto_write_mode_table (void)
   ob = create_output_block (LTO_section_mode_table);
   bitpack_d bp = bitpack_create (ob->main_stream);
 
-  /* Ensure that for GET_MODE_INNER (m) != VOIDmode we have
+  /* Ensure that for GET_MODE_INNER (m) != m we have
      also the inner mode marked.  */
   for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
     if (streamer_mode_table[i])
@@ -2685,7 +2685,7 @@ lto_write_mode_table (void)
 	if (GET_MODE_INNER (m) != m)
 	  streamer_mode_table[(int) GET_MODE_INNER (m)] = 1;
       }
-  /* First stream modes that have GET_MODE_INNER (m) == VOIDmode,
+  /* First stream modes that have GET_MODE_INNER (m) == m,
      so that we can refer to them afterwards.  */
   for (int pass = 0; pass < 2; pass++)
     for (int i = 0; i < (int) MAX_MACHINE_MODE; i++)
diff --git libgomp/ChangeLog libgomp/ChangeLog
index ccdeaa7..3b60290 100644
--- libgomp/ChangeLog
+++ libgomp/ChangeLog
@@ -1,3 +1,7 @@
+2015-08-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* testsuite/libgomp.oacc-c-c++-common/vector-type-1.c: New file.
+
 2015-08-03  Nathan Sidwell  <nathan@codesourcery.com>
 
 	* plugin/plugin-nvptx.c: Don't include dlfcn.h.
diff --git libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
new file mode 100644
index 0000000..5adfcec
--- /dev/null
+++ libgomp/testsuite/libgomp.oacc-c-c++-common/vector-type-1.c
@@ -0,0 +1,24 @@
+#define vector __attribute__ ((vector_size (4 * sizeof(int))))
+
+int main(void)
+{
+  vector int vi = { 12, -34, -56, 78 };
+
+#pragma acc parallel copy(vi)
+  {
+    if (vi[0] != 12
+	|| vi[1] != -34
+	|| vi[2] != -56
+	|| vi[3] != 78)
+      __builtin_abort();
+    vector int vi_ = { -21, -43, 65, 87 };
+    vi = vi_;
+  }
+  if (vi[0] != -21
+      || vi[1] != -43
+      || vi[2] != 65
+      || vi[3] != 87)
+    __builtin_abort();
+
+  return 0;
+}


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]