From: Richard Biener <rguenther@suse.de>
To: Jan Hubicka <hubicka@ucw.cz>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: Fix Eh delivery in partitioned functions
Date: Tue, 18 Jul 2017 07:34:00 -0000 [thread overview]
Message-ID: <alpine.LSU.2.20.1707180932550.10808@zhemvz.fhfr.qr> (raw)
In-Reply-To: <20170718072624.GA52973@kam.mff.cuni.cz>
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?
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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
next prev parent reply other threads:[~2017-07-18 7:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-18 7:26 Jan Hubicka
2017-07-18 7:34 ` Richard Biener [this message]
2017-07-18 7:44 ` Jan Hubicka
2017-07-18 7:46 ` Richard Biener
2017-07-19 12:52 ` Jan Hubicka
2017-07-19 13:57 ` Richard Biener
2017-07-19 14:01 ` Jan Hubicka
2017-07-19 14:04 ` Richard Biener
2017-07-19 14:18 ` Jan Hubicka
2017-07-19 14:20 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LSU.2.20.1707180932550.10808@zhemvz.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).