From patchwork Tue Jul 18 07:26:24 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 789951 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-458371-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="MNFrIklw"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3xBWvB6JfDz9s3T for ; Tue, 18 Jul 2017 17:27:02 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=L6TfB8PWhMMyt9TKxh4PbkiY64L/CwelYpLhCU0q7yHX6F5jDZutY cqR0SHfQxVis07PR/k3BZx87YunUPCGkfYW2fX6Ysj68jzF9A9EAHpHA6l5PoBE0 7YQNxa3hi9R96Nx7zF8odBrlmxtJ7clpmjNo9DtenQjajgmf62Xi3s= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=q6ZVKeJs1ptwirxpJCe5os7ibVM=; b=MNFrIklwMCaBZ4fHqL1m xgTjT+C2SEZqu7+hWhbS0K7VRqp4YjtX6OpfaAJZuDqrXdnhhR/V48swMjB4FpnO M1QQCrmCnERokVIMxqXmzd97LQrtO0drrjt5L67/GgaZmCEKORsJmLRCjSio3Cqy dqac0tF+kxbdNMwNPy/0/g0= Received: (qmail 6551 invoked by alias); 18 Jul 2017 07:26:47 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 130863 invoked by uid 89); 18 Jul 2017 07:26:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-9.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Connect X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jul 2017 07:26:27 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 0DFA9545770; Tue, 18 Jul 2017 09:26:25 +0200 (CEST) Date: Tue, 18 Jul 2017 09:26:24 +0200 From: Jan Hubicka To: gcc-patches@gcc.gnu.org, rguenther@suse.de Subject: Fix Eh delivery in partitioned functions Message-ID: <20170718072624.GA52973@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) 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. 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 (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;