On Sun, Oct 28, 2012 at 6:13 PM, Joern Rennecke
<joern.renne...@embecosm.com> wrote:
> Quoting Richard Biener <richard.guent...@gmail.com>:
>
>>> 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.
Apart from the iteration_threshold the hookization would be straight-forward.
Now I cannot decipher from the patch what functional change it introduces ;)
There are also ony 10 users of ADJUST_INSN_LENGTH, not enough to
warrant introducing a hook without removing the macro IMHO.
Thanks,
Richard.
> 2012-10-28 Joern Rennecke <joern.renne...@embecosm.com>
>
> * 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 *);
>
