Regression in target MIC compiler

Wed Aug 5 14:10:00 GMT 2015

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