[PING] Re: [Patch] Fix the allocation of jump register for RTL sequence abstraction

[Bernd Schmidt]

Gabor Loki wrote:

> 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.

How can this happen?  This problem should be documented somewhere in the 
code.

>>
>>> @@ -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++)

Depends on iterator and hash table internals which kind of breaks the 
abstraction.  Better to create a new FOR_EACH_HTAB_ELEMENT_FROM with an 
extra argument?  That's preapproved as a separate patch.

>>> +/* 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.

But all the following code assumes that you only get a single insn, 
starting with the setting of NEXT_INSN/PREV_INSN.  If you get a single 
one, it must be a JUMP_INSN.  Better return NULL and abort the 
optimization if you get anything other than a jump pattern/insn.

>>> +  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 you're only computing costs you shouldn't need anything other than a 
pattern.
Hmm, let me see if I understand all this correctly.  It goes through 
compute_rtx_cost, which in turn tries to look it up in a hash table 
using compute_hash, which looks at PREV_INSN.  In fact, with the patch 
it'll now loop through the same insn for HASHED_INSN times?
At the end it'll call get_attr_length which is likely to be unreliable 
for a newly generated indirect jump using a pseudo.  Won't that cause a 
problem once get_attr_length accesses INSN_ADDRESSES for the new insn?
I still don't see why it needs NEXT_INSN/PREV_INSN.

> It is conservative approach. The 'get_indirect_jump_insn_no'
> function returns the result of 'recog_memoized' which could
> return with negative value.

An expander like gen_indirect_jump isn't supposed to produce an invalid 
insn if the inputs match the predicates.  If really you want the sanity 
check, put a gcc_unreachable in there - that way we'll know immediately 
when something isn't as it should be.

>>> @@ -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

Ah.  It helps to repeat these things when submitting a patch to prevent 
the reviewer from having to guess the purpose of a change.  This part is 
ok separately, although there is room for improvement - we really should 
be able to estimate insn lengths at all times, even with -O2.


Bernd