On Tue, 18 Jul 2017, Jan Hubicka wrote:
> > On Tue, 18 Jul 2017, Jan Hubicka wrote:
> >
> > > Hi,
> > > this patch fixes wrong code issue with BB partitioning where sometimes EH
> > > is not delivered. This is very old issue that affect all release branches
> > > with -fprofile-use, but recently surfaced as libstdc++ testsuite
> > > regression
> > > because we now partition functions based on static profile prediction.
> > >
> > > The problem is that EH tables are stored as offsets from start of
> > > functions.
> > > In the record however value 0 is special and means that there is no
> > > landing
> > > pad for a given region. Normally this is safe because landing pads never
> > > appear as very first label in function. This is however no longer true
> > > with
> > > partitining where cold partition is actually quite likely to start by
> > > landing
> > > pad.
> > >
> > > The change in except.c adds sanity check that no EH landing pads are very
> > > first
> > > in the insn stream. The change in bb-reorder makes reorder to chose
> > > non-landing-pad BB as first trace for the cold partition. Such BB always
> > > exists
> > > because landing pads must be in same partition as the instruction
> > > throwing them
> > > and we never make BB both landing pad and reachable by normal control
> > > folow.
> > > However I am not thrilled by the fix as it is bit fragile in case some
> > > optimization happends after bb partitioning and code is moved away. Also
> > > the
> > > logic can be confused by asm statement which may result in no code (again
> > > however the BB reachable from outside world should contain something that
> > > produce EH that is a real instruction).
> > >
> > > Ideas for better fix would be welcome then. If the assert I added
> > > triggers
> > > for valid reasons, we may just end up adding a NOP in the rare case we do
> > > not suceed arranging cold partition to not start with landing pad.
> >
> > Yeah, I'd rather pad the function start with a nop if it starts with a
> > landing pad. How difficult would it be to arrange for this? I suppose
> > we'd need to touch each and every target to accomplish this? Or end up
> > using gen_nop in generic code?
>
> I think we could just output from generic code - I think it can be done by
> final_scan_insn. I don't know however if we have a way to tell if the section
> starts with a landing pad?
Not sure either -- some insn note / bb note? Some flag on the label?
At least the latter should be easy to add if it's not there already.
Richard.
> Honza
> >
> > Richard.
> >
> > > Bootstrapped/regtested x86_64-linux, looks sane?
> > >
> > > Honza
> > >
> > > PR middle-end/81331
> > > * except.c (first_in_partition): New function.
> > > (dw2_output_call_site_table): Sanity check that landing pads are not
> > > very first in the partition.
> > > * bb-reorder.c (ok_to_be_first): New function.
> > > (connect_traces): Avoid traces that are !ok_to_be_first to start
> > > partitions.
> > > Index: except.c
> > > ===================================================================
> > > --- except.c (revision 250226)
> > > +++ except.c (working copy)
> > > @@ -2724,6 +2724,23 @@ sjlj_size_of_call_site_table (void)
> > > return size;
> > > }
> > >
> > > +/* Return true if L will appear as very first in its partition. */
> > > +
> > > +bool
> > > +first_in_partition (rtx_insn *l)
> > > +{
> > > + while (l != NULL_RTX)
> > > + {
> > > + if (active_insn_p (l))
> > > + return false;
> > > + else if (GET_CODE (l) == NOTE
> > > + && NOTE_KIND (l) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
> > > + return true;
> > > + l = PREV_INSN (l);
> > > + }
> > > + return true;
> > > +}
> > > +
> > > static void
> > > dw2_output_call_site_table (int cs_format, int section)
> > > {
> > > @@ -2749,8 +2766,14 @@ dw2_output_call_site_table (int cs_forma
> > > ASM_GENERATE_INTERNAL_LABEL (reg_end_lab, "LEHE", call_site_base +
> > > i);
> > >
> > > if (cs->landing_pad)
> > > - ASM_GENERATE_INTERNAL_LABEL (landing_pad_lab, "L",
> > > - CODE_LABEL_NUMBER (cs->landing_pad));
> > > + {
> > > + ASM_GENERATE_INTERNAL_LABEL (landing_pad_lab, "L",
> > > + CODE_LABEL_NUMBER (cs->landing_pad));
> > > + /* Be sure that the offset will not be 0 as that would make EH
> > > + delivery code to think that there is no landing pad. */
> > > + gcc_checking_assert (!first_in_partition
> > > + (as_a <rtx_insn *> (cs->landing_pad)));
> > > + }
> > >
> > > /* ??? Perhaps use insn length scaling if the assembler supports
> > > generic arithmetic. */
> > > Index: bb-reorder.c
> > > ===================================================================
> > > --- bb-reorder.c (revision 250226)
> > > +++ bb-reorder.c (working copy)
> > > @@ -1066,6 +1066,21 @@ connect_better_edge_p (const_edge e, boo
> > > return is_better_edge;
> > > }
> > >
> > > +/* If we place EH landing pad as very first BB in the partition, its
> > > offset
> > > + from start of function is 0 which is special cased by the eh table to
> > > mean
> > > + no landing pad. For this reason such BBs can not appear as very
> > > first in
> > > + the partition. */
> > > +static bool
> > > +ok_to_be_first (struct trace *t)
> > > +{
> > > + edge e;
> > > + edge_iterator ei;
> > > + FOR_EACH_EDGE (e, ei, t->first->preds)
> > > + if (e->flags & EDGE_EH)
> > > + return false;
> > > + return true;
> > > +}
> > > +
> > > /* Connect traces in array TRACES, N_TRACES is the count of traces. */
> > >
> > > static void
> > > @@ -1080,6 +1095,7 @@ connect_traces (int n_traces, struct tra
> > > int freq_threshold;
> > > gcov_type count_threshold;
> > > bool for_size = optimize_function_for_size_p (cfun);
> > > + bool first_in_partition;
> > >
> > > freq_threshold = max_entry_frequency * DUPLICATION_THRESHOLD / 1000;
> > > if (max_entry_count.to_gcov_type () < INT_MAX / 1000)
> > > @@ -1092,6 +1108,7 @@ connect_traces (int n_traces, struct tra
> > > current_pass = 1;
> > > current_partition = BB_PARTITION (traces[0].first);
> > > two_passes = false;
> > > + first_in_partition = true;
> > >
> > > if (crtl->has_bb_partition)
> > > for (i = 0; i < n_traces && !two_passes; i++)
> > > @@ -1116,6 +1133,7 @@ connect_traces (int n_traces, struct tra
> > > current_partition = BB_COLD_PARTITION;
> > > else
> > > current_partition = BB_HOT_PARTITION;
> > > + first_in_partition = true;
> > > }
> > >
> > > if (connected[t])
> > > @@ -1125,6 +1143,9 @@ connect_traces (int n_traces, struct tra
> > > && BB_PARTITION (traces[t].first) != current_partition)
> > > continue;
> > >
> > > + if (first_in_partition && !ok_to_be_first (&traces[t]))
> > > + continue;
> > > +
> > > connected[t] = true;
> > >
> > > /* Find the predecessor traces. */
> > > @@ -1143,7 +1164,9 @@ connect_traces (int n_traces, struct tra
> > > && bbd[si].end_of_trace >= 0
> > > && !connected[bbd[si].end_of_trace]
> > > && (BB_PARTITION (e->src) == current_partition)
> > > - && connect_better_edge_p (e, true, best_len, best, traces))
> > > + && connect_better_edge_p (e, true, best_len, best, traces)
> > > + && (!first_in_partition
> > > + || ok_to_be_first (&traces[bbd[si].end_of_trace])))
> > > {
> > > best = e;
> > > best_len = traces[bbd[si].end_of_trace].length;
> > > @@ -1151,6 +1174,8 @@ connect_traces (int n_traces, struct tra
> > > }
> > > if (best)
> > > {
> > > + gcc_checking_assert (BB_PARTITION (best->src)
> > > + == BB_PARTITION (best->dest));
> > > best->src->aux = best->dest;
> > > t2 = bbd[best->src->index].end_of_trace;
> > > connected[t2] = true;
> > > @@ -1166,8 +1191,13 @@ connect_traces (int n_traces, struct tra
> > > }
> > >
> > > if (last_trace >= 0)
> > > - traces[last_trace].last->aux = traces[t2].first;
> > > + {
> > > + gcc_checking_assert (!first_in_partition
> > > + || ok_to_be_first (&traces[t2]));
> > > + traces[last_trace].last->aux = traces[t2].first;
> > > + }
> > > last_trace = t;
> > > + first_in_partition = false;
> > >
> > > /* Find the successor traces. */
> > > while (1)
> > > @@ -1226,6 +1256,8 @@ connect_traces (int n_traces, struct tra
> > > fprintf (dump_file, "Connection: %d %d\n",
> > > best->src->index, best->dest->index);
> > >
> > > + gcc_checking_assert (BB_PARTITION (best->dest)
> > > + == BB_PARTITION (best->src));
> > > t = bbd[best->dest->index].start_of_trace;
> > > traces[last_trace].last->aux = traces[t].first;
> > > connected[t] = true;
> > > @@ -1238,6 +1270,8 @@ connect_traces (int n_traces, struct tra
> > > fprintf (dump_file, "Connection: %d %d\n",
> > > best->src->index, best->dest->index);
> > > }
> > > + gcc_checking_assert (BB_PARTITION (best->dest)
> > > + == BB_PARTITION (best->src));
> > > t = bbd[best->dest->index].start_of_trace;
> > > traces[last_trace].last->aux = traces[t].first;
> > > connected[t] = true;
> > >
> > >
> >
> > --
> > Richard Biener <[email protected]>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
> > 21284 (AG Nuernberg)
>
>
--
Richard Biener <[email protected]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
