[PING] Re: [Patch] Fix the allocation of jump register for RTL sequence abstraction
Gabor Loki
loki@inf.u-szeged.hu
Mon Jun 12 06:58:00 GMT 2006
Bernd Schmidt wrote:
>> @@ -294,14 +310,18 @@ prev_insn_in_block (rtx insn)
>> static unsigned int
>> compute_hash (rtx insn)
>> {
>> - unsigned int hash = 0;
>> + unsigned int hash = 0, i = 0;
>> rtx prev;
>>
>> - hash = INSN_CODE (insn) * 100;
>> -
>> + hash = INSN_CODE (insn);
>> prev = prev_insn_in_block (insn);
>> - if (prev)
>> - hash += INSN_CODE (prev);
>> +
>> + while (prev && i < HASHED_INSNS)
>> + {
>> + hash = hash * 100 + INSN_CODE (prev);
>> + prev = prev_insn_in_block (insn);
>> + i++;
>> + }
>>
>> return hash;
>> }
>
>
> What's this for? Trying to make a better hash function? Does this have
> an effect on the rest of the code?
We need to redesign this function, because the modified 'compute_init_costs'
function could generate false hash buckets when HASHED_INSNS is too
small. This is not so critical as the following code, but it could lead to false
results.
>
>> @@ -498,6 +519,10 @@ collect_pattern_seqs (void)
>> FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e0,
>> p_hash_elem, hti1)
>> FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e1,
>> p_hash_elem,
>> hti2)
>> + {
>> + if (hti1.slot >= hti2.slot)
>> + hti2.slot = hti1.slot;
>> + else
>> if (e0 != e1
>
>
> What does this do? Could use a comment.
This modify the 'hti2' iterator to speed up the inner cycle.
like:
for (,,i++)
for (j=i,,j++)
>> +/* Creates a jump insn for abstraction. */
>> +
>> +static rtx
>> +seq_make_jump_insn_raw (rtx insn)
>> +{
>> + rtx new_insn;
>> +
>> + switch (GET_CODE (insn))
>> + {
>> + case INSN:
>> + case JUMP_INSN:
>> + case CALL_INSN:
>> + case CODE_LABEL:
>> + case BARRIER:
>> + case NOTE:
>> + new_insn = insn;
>> + break;
>
>
>> + default:
>> + new_insn = make_jump_insn_raw (insn);
>> + }
>
>
> This looks odd. Can you really pass both patterns and whole insns into this? Do you really need to handle NOTEs and INSNs?
Yes, we need. We don't know the results of 'gen_jump' and
'gen_indirect_jump' (which are the inputs of this function).
It could be a pattern (like x86) or a sequence (like sh) too.
>> +
>> + NEXT_INSN (new_insn) = new_insn;
>> + PREV_INSN (new_insn) = new_insn;
>
> Why?
H mm. I should had to place a comment. This is not a trivial
solution. The algorithm creates some temporary insns which
are used to measure the initial costs ('compute_init_costs'
function), but the cost computation only works for valid insns.
So we need to decorate the temporary insns to look like valid.
>
>> + if (!*insn_constraint)
>> + insn_class = ALL_REGS;
>> + else
>> + {
>> + insn_letter = *insn_constraint;
>> +
>> + insn_class =
>> + ((insn_letter == 'r' || insn_letter == 'p'
>> + || insn_letter ==
>> + 'g') ? BASE_REG_CLASS : REG_CLASS_FROM_CONSTRAINT ((unsigned char)
>> + insn_letter,
>> + insn_constraint));
>
>
> This assumes that there's only one reg class letter and it's in the first position.
Oops, you are right. I've fixed it.
>
>> + /* Init indirect jump insn data. */
>> + indirect_jump_insn_no = get_indirect_jump_insn_no ();
>> + if (indirect_jump_insn_no < 0)
>> + {
>> + if (dump_file)
>> + fprintf (dump_file, "Could not get INSN_NO for indirect jump.\n"); + return;
>> + }
>> +
>
>
> This can't happen can it?
>
> The whole thing seems to assume that gen_indirect_jump isn't an expander
> that can generate more than one insn.
It is conservative approach. The 'get_indirect_jump_insn_no'
function returns the result of 'recog_memoized' which could
return with negative value. IMHO it is better to be conservative
rather than any assumption leads to segfault.
>> @@ -1411,7 +1662,7 @@ rtl_seqabstr (void)
>> static bool
>> gate_rtl_seqabstr (void)
>> {
>> - return flag_rtl_seqabstr;
>> + return flag_rtl_seqabstr && optimize_size;
>> }
>
>
> This doesn't look right, why prevent people from using -O2
> -frtl-seqabstr? Not part of the bugfix either.
Maybe this is not the part of jump register bugfix, but it
is also important. We have a small discussion about cost
computing of the algorithm earlier. The final conclusion was
that the abstraction doesn't run when -Os isn't specified.
Please see the following threads:
- http://gcc.gnu.org/ml/gcc-patches/2005-12/threads.html#00988
- http://gcc.gnu.org/ml/gcc-patches/2006-01/threads.html#00331
Thanks for your suggestions. I hope there is nothing else left.
(Bootstrapped, and regtested on i386, arm, ppc, sh)
br,
Gabor Loki
2006-06-12 Gabor Loki <loki@gcc.gnu.org>
* gcc/rtl-factoring.c (gate_rtl_seqabstr): Only execute the optimization
when optimize_size is turned on.
(compute_hash): Use a parameter to control the number of hashed
insn. (collect_pattern_seq): Do not compare elements twice.
(recompute_gain_for_pattern_seq): Initialize STORE_REG and use
indirect jump register class instead of base reg. class to find free
jump register.
(split_pattern_seq): Use STORE_REG instead of LINK_REG if it exists
to save the return address.
(erase_matching_seq): Ditto.
(abstract_best_seq): Record the usage of the store register if it's
needed.
(dump_pattern_seq): Print STORE_REG info also.
(seq_make_jump_insn_raw): New function to create temporary jump
insn. (seq_make_insn_raw): New function to create temporary insn.
(get_indirect_jump_insn_no): New function to check indirect jump for
abstraction.
(get_insn_op_reg_class): Ditto.
(get_jump_reg): New function to find free register for indirect jump.
(compute_init_costs): Redesign the allocation of jump register.
(rtl_seqabstr): Check there is any possible register for indirect jump.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtl-fact.patch
Type: text/x-patch
Size: 15632 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20060612/e9390a21/attachment.bin>
More information about the Gcc-patches
mailing list