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

[Bernd Schmidt <bernds_cb1 at t-online dot de>]

Gabor Loki wrote:
> 2006-06-09  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, recompute_gain_for_pattern_seq, 
> split_pattern_seq,
>         erase_matching_seq, abstract_best_seq, 
> dump_pattern_seq,fill_hash_bucket,
>         seq_make_jump_insn_raw, seq_make_insn_raw, 
> get_indirect_jump_insn_no,
>         get_insn_op_reg_class, get_jump_reg, compute_init_costs, 
> rtl_seqabstr):
>     Fix the allocation of jump register.

Not the most informative ChangeLog I've seen.  Should list the changes 
in more detail, record which functions are new, etc.

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

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

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

> +
> +  NEXT_INSN (new_insn) = new_insn;
> +  PREV_INSN (new_insn) = new_insn;

Why?

> +/* The get_insn_op_reg_class returns the register class for the first
> +   operand of the given insn.  */
> +
> +static enum reg_class
> +get_insn_op_reg_class (int insn_no)
> +{
> +  const char *insn_constraint;
> +  char insn_letter;
> +  enum reg_class insn_class;
> +
> +  gcc_assert (insn_no >= 0);
> +
> +  /* Get the constraint for operands.  */
> +  insn_constraint = insn_data[insn_no].operand[0].constraint;
> +
> +  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.  Please follow what other places in the compiler do to 
compute the class.

Formatting looks odd.  A switch might work better.

> +  /* Calculate save insn cost if its needed.  */

"it's"

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

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


Bernd