From patchwork Sun Oct 28 17:13:12 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joern Rennecke X-Patchwork-Id: 194726 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 38CF72C008B for ; Mon, 29 Oct 2012 04:13:36 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1352049217; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:To:Cc:Subject:References:In-Reply-To: MIME-Version:Content-Type:Content-Transfer-Encoding:User-Agent: Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:Sender:Delivered-To; bh=x+xgWHETq8AYL/e8HWe7 7N7RTEo=; b=WdHmCDBjvwzrLXdR5LMgEE0GtC9UXZg3qhe6qvQBbSoA7gZywxHA k4MUV3pPpML8mvq6UQ7rEbzH3Y00FT2jku8M/Iy1Y4snCIszFigbn5quUAho7I78 3Qwu2+nD0+skMEgX9MTOrRUBDjqlXGAR3o+888xabcAlmEPiomEtlDo= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Message-ID:Date:From:To:Cc:Subject:References:In-Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding:User-Agent:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=dQt0bMn+SkS6UW34u2FzZ/eAjTUOAjksA0eEbtB04TWLRDLzVeT3cqsXSzTkSV gxJ1qfrUwq2bSEvJaQDp7osHVGtFQZ3k8JPGhrij60zf1v0STZQ8sVWWr+vR9kop niTT1P9C+Ezhh535usZV8MzbcmrKm2J528cbami4MakEw=; Received: (qmail 21572 invoked by alias); 28 Oct 2012 17:13:30 -0000 Received: (qmail 21564 invoked by uid 22791); 28 Oct 2012 17:13:29 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL, BAYES_00, KHOP_THREADED, MIME_QP_LONG_LINE X-Spam-Check-By: sourceware.org Received: from c62.cesmail.net (HELO c62.cesmail.net) (216.154.195.54) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 28 Oct 2012 17:13:12 +0000 Received: from unknown (HELO epsilon2) ([192.168.1.60]) by c62.cesmail.net with ESMTP; 28 Oct 2012 13:13:12 -0400 Received: from cust213-dsl91-135-11.idnet.net (cust213-dsl91-135-11.idnet.net [91.135.11.213]) by webmail.spamcop.net (Horde MIME library) with HTTP; Sun, 28 Oct 2012 13:13:12 -0400 Message-ID: <20121028131312.poycwzqkg0kc4gc8-nzlynne@webmail.spamcop.net> Date: Sun, 28 Oct 2012 13:13:12 -0400 From: Joern Rennecke To: Richard Biener Cc: Richard Sandiford , gcc-patches@gcc.gnu.org, Eric Botcazou Subject: RFA: hookize ADJUST_INSN_LENGTH (Was: RFA: Add lock_lenth attribute to support the ARC port) References: <20121015013935.v6k1ge59foks8s88-nzlynne@webmail.spamcop.net> <87391flcjy.fsf@talisman.home> <20121016153515.0nmsft8o2soo4s40-nzlynne@webmail.spamcop.net> <20121023214204.vsnfi46iyscw4sos-nzlynne@webmail.spamcop.net> In-Reply-To: MIME-Version: 1.0 User-Agent: Internet Messaging Program (IMP) H3 (4.1.4) 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 Quoting Richard Biener : >> Thus, you can allow the length to vary downwards as well as upwards >> across iterations with suitable definitions of the @code{length} attribute >> and/or @code{ADJUST_INSN_LENGTH}. Care has to be taken that this does not >> lead to infinite loops. > > I don't see that you can shrink length with just suitable lock_length and > length attributes. I disagree there (for certain values of 'just'), but we can just agree to disagree on this point because... > What seems to be the cruical difference is that you > apply ADJUST_INSN_LENGTH after combining lock-length-max and length. > But then you _decrease_ length with ADJUST_INSN_LENGHT ... > > Maybe what you really want is ADJUST_INSN_LENGTH_AFTER which is > applied afterwards? Thus, Well, actually, I found a number of problems with ADJUST_INSN_LENGTH: - it is not applied to inner insn is a delay slot sequence. You can sort of work around this by stitching recursive calls together, but then you are faced with another problem: - You don't know what the length prior to ADJUST_INSN_LENGTH was. That was even worse with the non-unique uids where you didn't knew squat about the instructions in the delay slot. - As you said, I want the adjustment happen after the maximum calculation. Well, usually. There are actually special cases where it would be useful to have some sort of maximum calculation in place, to break up alignment-driven cycles, but only applicable with a lesser priority than for the range-driven branch offset calculations. But adding yet another macro neither does solve all these problems, nor would it align with our goal to move away from target macros. I found now an alternate way to make the ARC port termiante building its insn-attrtab.c - it involves using match_test("get_attr_length (insn) == 2") instead of eq_attr("[lock_]length" (const_int 2)) - where I really want to know if the instruction was considered short in the previous iteration. So, I have made a patch to replace the ADJUST_INSN_LENGTH macro in final.c with an adjust_insn_length hook, for which a default implementation using ADJUST_INSN_LENGTH is provided in targhooks.c to provide for an easy transition. I've looked at the existing ports that use ADJUST_INSN_LENGTH, and some seem to prefer to return an adjustment to be added to the length, while others prefer to return the new length. The latter seemed to be slightly more numerous, so I went with that. The hook has two new parameters: - a flag that tells it if the insn in question is a delay sequence. The default hook implementation skips the invocation of ADJUST_INSN_LENGTH in this case for the sake of compatibility. - a pointer to int to set the number of iteration loops till the length locking feature is supposed to apply to this instruction length when using the increasing size calculations. The pointed-to value is initialized to zero, which means that length locking is always applied (assuming final.c uses the increasing length algorithm). Setting this to a higher number effectively gives the new instruction length a lower priority to be put into uid_lock_length. > Note that > >> Care has to be taken that this does not >> lead to infinite loops. > > doesn't convince me that is properly designed ;) With the hook mechanism, it is much harder to create an infinite loop inside shorten_branches. (It would involve something like setting iteration_threshold to MAX_INT and making length decreasing when niter is at MAX_INT, then letting integer overflow of niter take its course. Making it impossible for a port maintainer to get things wrong is not a meaningful goal for GCC, but making it straightforward to get it right is.) This patch builds on: http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02527.html Bootstrapped in revision 192879 on i686-pc-linux-gnu. Tested with config-list.mk on x86_64-unknown-linux-gnu. 2012-10-28 Joern Rennecke * doc/tm.texi.in (@hook TARGET_ADJUST_INSN_LENGTH): Add. * doc/tm.texi: Regenerate. * final.c (get_attr_length_1): Use targetm.adjust_insn_length instead of ADJUST_INSN_LENGTH. (adjust_length): New function. (shorten_branches): Use adjust_length instead of ADJUST_INSN_LENGTH. * target.def (adjust_insn_length): New hook. * targhooks.c (default_adjust_insn_length): New function. * targhooks.h (default_adjust_insn_length): Declare. diff -drup gcc-192879-haveattr/doc/tm.texi gcc/doc/tm.texi --- gcc-192879-haveattr/doc/tm.texi 2012-10-28 01:07:38.463469923 +0000 +++ gcc/doc/tm.texi 2012-10-28 01:31:15.522227927 +0000 @@ -11341,3 +11341,7 @@ This value should be set if the result w @deftypevrx {Target Hook} {bool} TARGET_HAVE_ATTR_LENGTH These flags are automatically generated; you should not override them in tm.c: @end deftypevr + +@deftypefn {Target Hook} int TARGET_ADJUST_INSN_LENGTH (rtx @var{insn}, int @var{length}, bool @var{in_delay_sequence}, int *@var{iteration_threshold}) +Return an adjusted length for @var{insn}. @var{length} is the value that has been calculated using the @code{length} instruction attribute. @var{in_delay_sequence} if @var{insn} forms part of a delay sequence. *@var{iteration_threshold} specifies the number of branch shortening iterations before length decreases are inhibited. The default implementation uses @code{ADJUST_INSN_LENGTH}, if defined. +@end deftypefn diff -drup gcc-192879-haveattr/doc/tm.texi.in gcc/doc/tm.texi.in --- gcc-192879-haveattr/doc/tm.texi.in 2012-10-28 01:07:38.489470252 +0000 +++ gcc/doc/tm.texi.in 2012-10-28 01:31:55.578745701 +0000 @@ -11175,3 +11175,5 @@ memory model bits are allowed. @hook TARGET_ATOMIC_TEST_AND_SET_TRUEVAL @hook TARGET_HAVE_CC0 + +@hook TARGET_ADJUST_INSN_LENGTH diff -drup gcc-192879-haveattr/final.c gcc/final.c --- gcc-192879-haveattr/final.c 2012-10-28 01:07:38.492470288 +0000 +++ gcc/final.c 2012-10-28 15:24:49.731169660 +0000 @@ -424,10 +424,8 @@ get_attr_length_1 (rtx insn, int (*fallb break; } -#ifdef ADJUST_INSN_LENGTH - ADJUST_INSN_LENGTH (insn, length); -#endif - return length; + int dummy = -1; + return targetm.adjust_insn_length (insn, length, false, &dummy); } /* Obtain the current length of an insn. If branch shortening has been done, @@ -827,6 +825,56 @@ struct rtl_opt_pass pass_compute_alignme }; +/* Helper function for shorten_branches, to apply the adjust_insn_length + target hook, and lock in length increases in order to avoid infinite + iteration cycles. + INSN the the instruction being processed, and new_length is the length + that has been calulated for it from its length attribute. UID is + its UID. SEQ_P indicates if it is inside a delay slot sequence. + INSN_LENGTHS and UID_LOCK_LENGTH are the arrays holding the current lengths + and lockeds-in maximum lengths from the prevuious iterations, and are + updated for UID when appropriate. + *NITER contains the number of iterations since we last made a change + to uid_lock_length. + *SOMETHING_CHANGED will be set if we make a change to INSN_LENGTHS. + The return value is the new length taking the result of the + adjust_insn_length and uid_lock_length into account. + Note that when *NITER is negative, no length locking is effective, and + the new lengths are just assigned. This is used in the initial pass, + and when only making a single pass to update instruction length downward. */ +static int +adjust_length (rtx insn, int new_length, int uid, int *insn_lengths, + int *uid_lock_length, bool seq_p, int *niter, + bool *something_changed) + +{ + int iter_threshold = 0; + new_length + = targetm.adjust_insn_length (insn, new_length, seq_p, &iter_threshold); + if (new_length < 0) + fatal_insn ("negative insn length", insn); + if (iter_threshold < 0) + fatal_insn ("negative shorten_branches iteration threshold", insn); + if (new_length != insn_lengths[uid]) + { + if (new_length < uid_lock_length[uid]) + new_length = uid_lock_length[uid]; + if (new_length == insn_lengths[uid]) + ; /* done here. */ + else if (*niter < iter_threshold + || new_length > insn_lengths[uid]) + { + if (*niter >= iter_threshold) + uid_lock_length[uid] = new_length, *niter = 0; + insn_lengths[uid] = new_length; + *something_changed = true; + } + else + new_length = insn_lengths[uid]; + } + return new_length; +} + /* Make a pass over all insns and compute their actual lengths by shortening any branches of variable length if possible. */ @@ -848,7 +896,7 @@ shorten_branches (rtx first) int max_skip; #define MAX_CODE_ALIGN 16 rtx seq; - int something_changed = 1; + bool something_changed = true; char *varying_length; rtx body; int uid; @@ -964,7 +1012,8 @@ shorten_branches (rtx first) return; /* Allocate the rest of the arrays. */ - insn_lengths = XNEWVEC (int, max_uid); + insn_lengths = XCNEWVEC (int, max_uid); + int *uid_lock_length = XCNEWVEC (int, max_uid); insn_lengths_max_uid = max_uid; /* Syntax errors can lead to labels being outside of the main insn stream. Initialize insn_addresses, so that we get reproducible results. */ @@ -1065,6 +1114,7 @@ shorten_branches (rtx first) /* Compute initial lengths, addresses, and varying flags for each insn. */ int (*length_fun) (rtx) = increasing ? insn_min_length : insn_default_length; + int niter = increasing ? 0 : -1; for (insn_current_address = 0, insn = first; insn != 0; @@ -1072,8 +1122,6 @@ shorten_branches (rtx first) { uid = INSN_UID (insn); - insn_lengths[uid] = 0; - if (LABEL_P (insn)) { int log = LABEL_TO_ALIGNMENT (insn); @@ -1093,6 +1141,8 @@ shorten_branches (rtx first) if (INSN_DELETED_P (insn)) continue; + int length = 0; + body = PATTERN (insn); if (GET_CODE (body) == ADDR_VEC || GET_CODE (body) == ADDR_DIFF_VEC) { @@ -1100,13 +1150,12 @@ shorten_branches (rtx first) section. */ if (JUMP_TABLES_IN_TEXT_SECTION || readonly_data_section == text_section) - insn_lengths[uid] = (XVECLEN (body, - GET_CODE (body) == ADDR_DIFF_VEC) - * GET_MODE_SIZE (GET_MODE (body))); + length = (XVECLEN (body, GET_CODE (body) == ADDR_DIFF_VEC) + * GET_MODE_SIZE (GET_MODE (body))); /* Alignment is handled by ADDR_VEC_ALIGN. */ } else if (GET_CODE (body) == ASM_INPUT || asm_noperands (body) >= 0) - insn_lengths[uid] = asm_insn_count (body) * insn_default_length (insn); + length = asm_insn_count (body) * insn_default_length (insn); else if (GET_CODE (body) == SEQUENCE) { int i; @@ -1134,7 +1183,10 @@ shorten_branches (rtx first) else inner_length = inner_length_fun (inner_insn); - insn_lengths[inner_uid] = inner_length; + inner_length + = adjust_length (insn, inner_length, inner_uid, insn_lengths, + uid_lock_length, true, &niter, + &something_changed); if (const_delay_slots) { if ((varying_length[inner_uid] @@ -1145,21 +1197,18 @@ shorten_branches (rtx first) } else varying_length[inner_uid] = 0; - insn_lengths[uid] += inner_length; + length += inner_length; } } else if (GET_CODE (body) != USE && GET_CODE (body) != CLOBBER) { - insn_lengths[uid] = length_fun (insn); + length = length_fun (insn); varying_length[uid] = insn_variable_length_p (insn); } - + /* If needed, do any adjustment. */ -#ifdef ADJUST_INSN_LENGTH - ADJUST_INSN_LENGTH (insn, insn_lengths[uid]); - if (insn_lengths[uid] < 0) - fatal_insn ("negative insn length", insn); -#endif + adjust_length (insn, length, uid, insn_lengths, uid_lock_length, + false, &niter, &something_changed); } /* Now loop over all the insns finding varying length insns. For each, @@ -1168,16 +1217,16 @@ shorten_branches (rtx first) while (something_changed) { - something_changed = 0; + if (increasing) + niter++; + + something_changed = false; insn_current_align = MAX_CODE_ALIGN - 1; for (insn_current_address = 0, insn = first; insn != 0; insn = NEXT_INSN (insn)) { int new_length; -#ifdef ADJUST_INSN_LENGTH - int tmp_length; -#endif int length_align; uid = INSN_UID (insn); @@ -1363,17 +1412,13 @@ shorten_branches (rtx first) if (! varying_length[inner_uid]) inner_length = insn_lengths[inner_uid]; else - inner_length = insn_current_length (inner_insn); - - if (inner_length != insn_lengths[inner_uid]) { - if (!increasing || inner_length > insn_lengths[inner_uid]) - { - insn_lengths[inner_uid] = inner_length; - something_changed = 1; - } - else - inner_length = insn_lengths[inner_uid]; + inner_length = insn_current_length (inner_insn); + + inner_length + = adjust_length (insn, inner_length, inner_uid, + insn_lengths, uid_lock_length, true, + &niter, &something_changed); } insn_current_address += inner_length; new_length += inner_length; @@ -1385,21 +1430,13 @@ shorten_branches (rtx first) insn_current_address += new_length; } -#ifdef ADJUST_INSN_LENGTH /* If needed, do any adjustment. */ - tmp_length = new_length; - ADJUST_INSN_LENGTH (insn, new_length); + int tmp_length = new_length; + new_length + = adjust_length (insn, new_length, uid, insn_lengths, + uid_lock_length, false, &niter, + &something_changed); insn_current_address += (new_length - tmp_length); -#endif - - if (new_length != insn_lengths[uid] - && (!increasing || new_length > insn_lengths[uid])) - { - insn_lengths[uid] = new_length; - something_changed = 1; - } - else - insn_current_address += insn_lengths[uid] - new_length; } /* For a non-optimizing compile, do only a single pass. */ if (!increasing) diff -drup gcc-192879-haveattr/target.def gcc/target.def --- gcc-192879-haveattr/target.def 2012-10-28 01:07:38.517470606 +0000 +++ gcc/target.def 2012-10-28 01:31:15.525227979 +0000 @@ -2818,6 +2818,17 @@ DEFHOOK enum unwind_info_type, (void), default_debug_unwind_info) +DEFHOOK +(adjust_insn_length, + "Return an adjusted length for @var{insn}. @var{length} is the value that\ + has been calculated using the @code{length} instruction attribute. \ + @var{in_delay_sequence} if @var{insn} forms part of a delay sequence. \ + *@var{iteration_threshold} specifies the number of branch shortening\ + iterations before length decreases are inhibited. The default\ + implementation uses @code{ADJUST_INSN_LENGTH}, if defined.", + int, (rtx insn, int length, bool in_delay_sequence, int *iteration_threshold), + default_adjust_insn_length) + DEFHOOKPOD (atomic_test_and_set_trueval, "This value should be set if the result written by\ diff -drup gcc-192879-haveattr/targhooks.c gcc/targhooks.c --- gcc-192879-haveattr/targhooks.c 2012-10-25 03:33:26.000000000 +0100 +++ gcc/targhooks.c 2012-10-28 01:41:35.082931024 +0000 @@ -1540,4 +1540,20 @@ default_member_type_forces_blk (const_tr return false; } +/* Default version of adjust_insn_length. */ + +int +default_adjust_insn_length (rtx insn ATTRIBUTE_UNUSED, int length, + bool in_delay_sequence, int *iter_threshold) +{ + if (in_delay_sequence) + *iter_threshold = INT_MAX; +#ifdef ADJUST_INSN_LENGTH + else + ADJUST_INSN_LENGTH (insn, length); +#endif + return length; +} + + #include "gt-targhooks.h" diff -drup gcc-192879-haveattr/targhooks.h gcc/targhooks.h --- gcc-192879-haveattr/targhooks.h 2012-10-25 03:33:26.000000000 +0100 +++ gcc/targhooks.h 2012-10-28 01:31:15.479227682 +0000 @@ -193,3 +193,4 @@ extern const char *default_pch_valid_p ( extern void default_asm_output_ident_directive (const char*); extern bool default_member_type_forces_blk (const_tree, enum machine_mode); +extern int default_adjust_insn_length (rtx, int, bool, int *);