From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28433 invoked by alias); 8 Oct 2012 03:53:12 -0000 Received: (qmail 28420 invoked by uid 22791); 8 Oct 2012 03:53:11 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,MSGID_MULTIPLE_AT,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 08 Oct 2012 03:53:05 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Mon, 08 Oct 2012 04:53:04 +0100 Received: from Binsh02 ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.0); Mon, 8 Oct 2012 04:53:02 +0100 From: "Bin Cheng" To: "'Jeff Law'" Cc: "'Steven Bosscher'" , References: <50655073.e54c420a.651e.ffffac0fSMTPIN_ADDED@mx.google.com> <00b401cd9e0c$d7982140$86c863c0$@cheng@arm.com> <506AE4F1.5030807@redhat.com> In-Reply-To: <506AE4F1.5030807@redhat.com> Subject: RE: [PATCH RFA] Implement register pressure directed hoist pass Date: Mon, 08 Oct 2012 03:53:00 -0000 Message-ID: <000101cda507$bebb6a40$3c323ec0$@cheng@arm.com> MIME-Version: 1.0 X-MC-Unique: 112100804530400201 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-10/txt/msg00709.txt.bz2 Hi Jeff, Thanks for reviewing and sorry for this delayed message. >=20 > > + /* Only decrease distance if bb has high register pressure or EXPR > > + is const expr, otherwise EXPR can be hoisted through bb without > > + cost. */ > ?!? This comment makes no sense to me. To accurately know how hoisting > an expression affects pressure you have to look at the inputs and output > and see how their lifetime has changed. >=20 > In general: >=20 > For inputs, hoisting *may* reduce pressure. You really have to look at > how the life of the input changes based on the new location of the insn. > For example, if the input's lifetime is unchanged (say perhaps because > it was live after the insn we want to hoist), then hoisting will have no > impact. Otherwise the input's life is shortened, but to know by how much > you have to determine whether the new death of the input occurs (it may > still die in the hoisted insn or it may die elsewhere). >=20 > For an output, hoisting will (effectively) always extend the lifetime. >=20 > I've speculated that the right way to deal with register pressure in > code motion is to actually build the dependency graph and use that to > guide the code motions. I've never cobbled together any real code to do > this though. Yes, that's exactly what I mean "simulate register pressure change of each basic block accurately" in the patch. The current patch works in the way it only extend the lifetime of output registers of hoisted expressions, while does not consider the possibility of decreasing of register pressure because of input registers. The rationales are: 1. Calculation live range info and maintain of the info during hoisting process are very costly. 2. From the observation during the work, I found most of expression hoisted are constants and base-addresses, means the possibility of decreasing register pressure in hoisting expression is relatively low. This patch does not decrease distance if the basic block's register pressure is low, making the expression can be hoisted up in flow graph longer. When register pressure is high, the patch retreats to original algorithm. Also I observed lots of size regressions by hoisting constant expressions aggressively along with other expressions, so the patch uses original heuristic for constant expressions. > > @@ -2863,7 +2909,8 @@ static int > > if (visited =3D=3D NULL) > > { > > visited_allocated_locally =3D 1; > > - visited =3D XCNEWVEC (char, last_basic_block); > > + visited =3D sbitmap_alloc (last_basic_block); > > + sbitmap_zero (visited); > > } > What's the purpose behind changing visited from a simple array to a > sbitmap? I'm not objecting, but would like to hear the rationale behind > that change. I'll also note it wasn't mentioned in the ChangeLog. Because I have to iterate over visited basic blocks to update register pressure information, the operation is less effective on integer array. Maybe I should submit this as a separate patch as suggested by Steven. >=20 > Similarly what's the rationale behind passing the expression itself > rather than just its index? I don't see where we need to use anything > other than the index in this code. And again, this change isn't > mentioned in the ChangeLog. The expression itself is needed to check "CONST_INT_P (expr->expr)". >=20 > > + /* Considered invariant insns have only one set. */ > > + gcc_assert (set !=3D NULL_RTX); > > + reg =3D SET_DEST (set); > > + if (GET_CODE (reg) =3D=3D SUBREG) > > + reg =3D SUBREG_REG (reg); > > + if (MEM_P (reg)) > > + { > > + *nregs =3D 0; > > + pressure_class =3D NO_REGS; > > + } > Don't you need to look at the addresses within the MEM? This function is a copy from register pressure calculation code in loop-invariant.c I think MEM should be ignored here, because as in function hash_scan_set, we won't honor stores in memory when flag_gcse_las is not enabled.=20 >=20 > > Index: gcc/config/arm/arm.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > --- gcc/config/arm/arm.c (revision 191816) > > +++ gcc/config/arm/arm.c (working copy) > > @@ -2021,6 +2021,11 @@ arm_option_override (void) > > && current_tune->num_prefetch_slots > 0) > > flag_prefetch_loop_arrays =3D 1; > > > > + /* Enable register pressure hoist when optimizing for size on Thumb1 set. > */ > > + if (TARGET_THUMB1 && optimize_function_for_size_p (cfun) > > + && flag_ira_hoist_pressure =3D=3D -1) > > + flag_ira_hoist_pressure =3D 1; > I'd rather see this enabled for all targets when optimizing for size; > that guarantees more testing. Even if it doesn't help a particular > target, as long as it isn't hurting we're better off enabling it. I will collect size data for more targets. >=20 > I don't see any testsuite additions -- it'd really help long term to > have some tests for this stuff. It's some kind of difficult to create stable cases for hoist, especially having register pressure information incorporated. I will try it anyway. Could you give me some advices on this? >=20 > You should address the issues noted above and repost for final review > before likely integration. >=20 All other comments are accepted. I will work out an updated patch addressing all your/Steven's concerns, if you agree my explanations in this message. Thanks.