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:tho...@codesourcery.com] > Sent: 05 August 2015 11:46 > To: David Sherwood > Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Kirill Yukhin; > nat...@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.sherw...@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 <iver...@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 <iver...@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
