From patchwork Fri Dec 15 11:25:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 849095 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-469327-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="wiKFvAqh"; 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 3yyp5s1skhz9t2W for ; Fri, 15 Dec 2017 22:26:08 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:to :references:from:subject:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=dPmQLf9c7iKRq43Rg t3RSxOTSlfWBA7TCmS/IvHFsjPhaki4dfUr4s2A/AHD2x60uaJz2t9m73LqnNWn0 aIQKrFO3/Om3WMg6cQqGd4Hgls2LPTl1SKsy5n7LSfpq0fUqvFyJ1un1F4qVdVN7 OGHe1PsSBkJUFL+Os/wEdzV38Q= 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:to :references:from:subject:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=Z9qzX7K181jecrsNLoo1daD TQj8=; b=wiKFvAqh4nd3ZWBp8LFrK0GLLHQxYMvZDMUELVWKLca7VBS3DTubNRE B+JGQEXRZNb6LHFZydnnqzA5rFuTsYbgONnGkVa5fMgAijb3vp/XFMT2Tgksf/pL VTz6w8wMo1VaD+Wu/n+lHHWiAWuKWrUXXzImMV+otTmRX1WUPTH0= Received: (qmail 69259 invoked by alias); 15 Dec 2017 11:26:00 -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 69245 invoked by uid 89); 15 Dec 2017 11:25:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 15 Dec 2017 11:25:56 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-MBX-04.mgc.mentorg.com) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1ePo7k-0005O9-Nn from Tom_deVries@mentor.com ; Fri, 15 Dec 2017 03:25:52 -0800 Received: from [172.30.72.115] (137.202.0.87) by SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Fri, 15 Dec 2017 11:25:49 +0000 To: Vladimir Makarov , "gcc-patches@gcc.gnu.org" References: <5794f39e-42ef-66da-e7f5-270ebc54cdf2@redhat.com> <98df25f2-0282-7eca-f6d9-c320c023ec0d@mentor.com> <5ae37149-3559-8221-fc74-68f4e90ff4de@redhat.com> From: Tom de Vries Subject: [PATCH, PR83327] Fix liveness analysis in lra for spilled-into hard regs Message-ID: <12e6eeea-ba32-bdcf-1e42-6480b0bbad32@mentor.com> Date: Fri, 15 Dec 2017 12:25:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <5ae37149-3559-8221-fc74-68f4e90ff4de@redhat.com> X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) [ was: Re: patch to fix PR82353 ] On 12/14/2017 06:01 PM, Vladimir Makarov wrote: > > > On 12/13/2017 07:34 AM, Tom de Vries wrote: >> On 10/16/2017 10:38 PM, Vladimir Makarov wrote: >>> This is another version of the patch to fix >>> >>>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82353 >>> >>> The patch was successfully bootstrapped on x86-64 with Go and Ada. >>> >>> Committed as rev. 253796. >> >> Hi Vladimir, >> >> AFAIU this bit of the patch makes sure that the flags register show up >> in the bb_livein of the bb in which it's used (and not defined before >> the use), but not in the bb_liveout of the predecessors of that bb. >> >> I wonder if that's a compile-speed optimization, or an oversight. >> > Hi, Tom.  It was just a minimal fix.  I prefer minimal fixes for LRA > because even for me it is hard to predict in many cases how the patch > will affect all the targets.  Therefore many LRA patches have a few > iterations before to be final. > I see, thanks for the explanation. > I remember that I had some serious problems in the past when I tried to > implement fixed hard reg liveness propagation in LRA.  It was long ago > so we could try it again.  If you send patch you mentioned to gcc > mailing list, I'll review and approve it. Here it is. It applies cleanly to trunk (and to gcc-7-branch if you first backport r253796, the fix for PR82353). I have not tested this on trunk sofar, only on the internal branch for gcn based on gcc 7.1 with the gcc testsuite, where it fixes a wrong-code bug in gcc.dg/vect/no-scevccp-outer-10.c and causes no regressions. -------------------------------------------------------------------------- The problem on a minimal version of the test-case looks as follows: I. At ira, we have a def and use of 'reg:BI 605', the def in bb2 and the use in bb3: ... (note 44 32 33 2 [bb 2] NOTE_INSN_BASIC_BLOCK) ... (insn 269 54 64 2 (set (reg:BI 605) (le:BI (reg/v:SI 491 [ n ]) (const_int 0 [0]))) 23 {cstoresi4} (nil)) .... (code_label 250 228 56 3 7 (nil) [1 uses]) (note 56 250 58 3 [bb 3] NOTE_INSN_BASIC_BLOCK) ... (jump_insn 62 60 63 3 (set (pc) (if_then_else (ne:BI (reg:BI 605) (const_int 0 [0])) (label_ref 242) (pc))) "no-scevccp-outer-10.c":19 21 {cjump} (int_list:REG_BR_PROB 1500 (nil)) -> 242) (note 63 62 66 4 [bb 4] NOTE_INSN_BASIC_BLOCK) ... And in lra, we decide to spill it into a hard register: ... Spill r605 into hr95 ... Resulting in this code: ... (insn 385 386 64 2 (set (reg:BI 95 s95) (reg:BI 18 s18 [605])) 3 {*movbi} (nil)) ... (insn 404 60 405 3 (set (reg:BI 18 s18 [605]) (reg:BI 95 s95)) "no-scevccp-outer-10.c":19 3 {*movbi} (nil)) ... II. However, a bit later in lra we decide to assign r94,r95 to DImode pseudo 833: ... Assign 94 to reload r833 (freq=60) ... Resulting in this code: ... (insn 629 378 390 2 (set (reg:DI 94 s94 [833]) (plus:DI (reg/f:DI 16 s16) (const_int -8 [0xfffffffffffffff8]))) 35 {addptrdi3} (nil)) ... This clobbers the def of s95 in insn 385. III. Consequently, the insn is removed in the dce in the jump pass: ... DCE: Deleting insn 385 deleting insn with uid = 385. ... -------------------------------------------------------------------------- Analysis: The decision to assign r94,r95 to DImode pseudo 833 happens because r95 is not marked in the conflict_hard_regs of lra_reg_info[833] during liveness analysis. There's code in make_hard_regno_born to set the reg in the conflict_hard_regs of live pseudos, but at the point that r95 becomes live, the r833 pseudo is not live. Then there's code in mark_pseudo_live to set the hard_regs_live in the conflict_hard_regs of the pseudo, but at the point that r833 becomes live, r95 is not set in hard_regs_live, due to the fact that it's not set in df_get_live_out (bb2). In other words, the root cause is that hard reg liveness propagation is not done. -------------------------------------------------------------------------- Proposed Solution: The patch addresses the problem, by: - marking the hard regs that have been used in lra_spill in hard_regs_spilled_into - using hard_regs_spilled_into in lra_create_live_ranges to make sure those registers are marked in the conflict_hard_regs of pseudos that overlap with the spill register usage [ I've also tried an approach where I didn't use hard_regs_spilled_into, but tried to propagate all hard regs. I figured out that I needed to mask out eliminable_regset. Also I needed to masked out lra_no_alloc_regs, but that could be due to gcn-specific problems (pointers take 2 hard regs), I'm not yet sure. Anyway, in the submitted patch I tried to avoid these problems and went for the more minimal approach. ] In order to get the patch accepted for trunk, I think we need: - bootstrap and reg-test on x86_64 - build and reg-test on mips (the only primary platform that has the spill_class hook enabled) Any comments? > But we need to be ready to > revert it if some problems occur again. > Understood. Thanks, - Tom Fix liveness analysis in lra for spilled-into hard regs 2017-12-12 Tom de Vries PR rtl-optimization/83327 * lra-int.h (hard_regs_spilled_into): Declare. * lra.c (hard_regs_spilled_into): Define. (init_reg_info): Init hard_regs_spilled_into. * lra-spills.c (assign_spill_hard_regs): Update hard_regs_spilled_into. * lra-lives.c (make_hard_regno_born, make_hard_regno_dead) (process_bb_lives): Handle hard_regs_spilled_into. (lra_create_live_ranges_1): Before doing liveness propagation, clear regs in all_hard_regs_bitmap if set in hard_regs_spilled_into. --- gcc/lra-int.h | 2 ++ gcc/lra-lives.c | 28 ++++++++++++++++++++++++++-- gcc/lra-spills.c | 2 ++ gcc/lra.c | 3 +++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/gcc/lra-int.h b/gcc/lra-int.h index 5a519b0..064961a 100644 --- a/gcc/lra-int.h +++ b/gcc/lra-int.h @@ -122,6 +122,8 @@ struct lra_reg /* References to the common info about each register. */ extern struct lra_reg *lra_reg_info; +extern HARD_REG_SET hard_regs_spilled_into; + /* Static info about each insn operand (common for all insns with the same ICODE). Warning: if the structure definition is changed, the initializer for debug_operand_data in lra.c should be changed diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c index 03dfec2..85da626 100644 --- a/gcc/lra-lives.c +++ b/gcc/lra-lives.c @@ -245,7 +245,7 @@ make_hard_regno_born (int regno, bool check_pic_pseudo_p ATTRIBUTE_UNUSED) || i != REGNO (pic_offset_table_rtx)) #endif SET_HARD_REG_BIT (lra_reg_info[i].conflict_hard_regs, regno); - if (fixed_regs[regno]) + if (fixed_regs[regno] || TEST_HARD_REG_BIT (hard_regs_spilled_into, regno)) bitmap_set_bit (bb_gen_pseudos, regno); } @@ -259,7 +259,7 @@ make_hard_regno_dead (int regno) return; sparseset_set_bit (start_dying, regno); CLEAR_HARD_REG_BIT (hard_regs_live, regno); - if (fixed_regs[regno]) + if (fixed_regs[regno] || TEST_HARD_REG_BIT (hard_regs_spilled_into, regno)) { bitmap_clear_bit (bb_gen_pseudos, regno); bitmap_set_bit (bb_killed_pseudos, regno); @@ -1051,6 +1051,25 @@ process_bb_lives (basic_block bb, int &curr_point, bool dead_insn_p) check_pseudos_live_through_calls (j, last_call_used_reg_set); } + for (i = 0; i < FIRST_PSEUDO_REGISTER; ++i) + { + if (!TEST_HARD_REG_BIT (hard_regs_live, i)) + continue; + + if (!TEST_HARD_REG_BIT (hard_regs_spilled_into, i)) + continue; + + if (bitmap_bit_p (df_get_live_in (bb), i)) + continue; + + live_change_p = true; + if (lra_dump_file) + fprintf (lra_dump_file, + " hard reg r%d is added to live at bb%d start\n", i, + bb->index); + bitmap_set_bit (df_get_live_in (bb), i); + } + if (need_curr_point_incr) next_program_point (curr_point, freq); @@ -1320,6 +1339,11 @@ lra_create_live_ranges_1 (bool all_p, bool dead_insn_p) } /* As we did not change CFG since LRA start we can use DF-infrastructure solver to solve live data flow problem. */ + for (int i = 0; i < FIRST_PSEUDO_REGISTER; ++i) + { + if (TEST_HARD_REG_BIT (hard_regs_spilled_into, i)) + bitmap_clear_bit (&all_hard_regs_bitmap, i); + } df_simple_dataflow (DF_BACKWARD, NULL, live_con_fun_0, live_con_fun_n, live_trans_fun, &all_blocks, diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c index af11b3d..ab13b21 100644 --- a/gcc/lra-spills.c +++ b/gcc/lra-spills.c @@ -291,6 +291,8 @@ assign_spill_hard_regs (int *pseudo_regnos, int n) } if (lra_dump_file != NULL) fprintf (lra_dump_file, " Spill r%d into hr%d\n", regno, hard_regno); + add_to_hard_reg_set (&hard_regs_spilled_into, + lra_reg_info[regno].biggest_mode, hard_regno); /* Update reserved_hard_regs. */ for (r = lra_reg_info[regno].live_ranges; r != NULL; r = r->next) for (p = r->start; p <= r->finish; p++) diff --git a/gcc/lra.c b/gcc/lra.c index fec2383..047e64f 100644 --- a/gcc/lra.c +++ b/gcc/lra.c @@ -1270,6 +1270,8 @@ static int reg_info_size; /* Common info about each register. */ struct lra_reg *lra_reg_info; +HARD_REG_SET hard_regs_spilled_into; + /* Last register value. */ static int last_reg_value; @@ -1319,6 +1321,7 @@ init_reg_info (void) for (i = 0; i < reg_info_size; i++) initialize_lra_reg_info_element (i); copy_vec.truncate (0); + CLEAR_HARD_REG_SET (hard_regs_spilled_into); }