Re: PING^(n+1) fwprop merge (ping 6 or 7 by now)

[Ian Lance Taylor <ian at airs dot com>]

I want to stay out of any battles over this patch.  Clearly it should
have been reviewed long ago, and clearly the reviewers as a whole,
including myself, are at fault.

I agree with Mark that we should be prepared to accept this patch even
in stage 3 since it was submitted before entering stage 3.  That has
always been my understanding of the general rule.

Ian

> +/* Return a pointer to one of the occurrences of FIND in *PX.  */
> +
> +rtx *
> +find_occurrence (rtx *px, rtx find)

I think it would be preferable to write this in terms for
for_each_rtx, unless there is some reason not to.  Every time we have
a list like this:

> +    case REG:
> +    case CONST_INT:
> +    case CONST_DOUBLE:
> +    case CONST_VECTOR:
> +    case SYMBOL_REF:
> +    case CODE_LABEL:
> +    case PC:
> +    case CC0:

we have something that needs to be updated from here until eternity.
It's true that count_occurrence is not written using for_each_rtx, but
that is a bug, not a feature.

> @@ -2847,10 +2909,16 @@ auto_inc_p (rtx x)
>  int
>  loc_mentioned_in_p (rtx *loc, rtx in)
>  {
> -  enum rtx_code code = GET_CODE (in);
> -  const char *fmt = GET_RTX_FORMAT (code);
> +  enum rtx_code code;
> +  const char *fmt;
>    int i, j;
>  
> +  /* Allow this function to work in EXPR_LISTs.  */
> +  if (!in)
> +    return 0;
> +
> +  code = GET_CODE (in);
> +  fmt = GET_RTX_FORMAT (code);

No need for the comment--I would take it out.  The code is
self-evidently reasonable, and the inline comment is distracting.


> Index: df-core.c
> ===================================================================
> --- df-core.c	(revision 113581)
> +++ df-core.c	(working copy)
> @@ -919,7 +919,11 @@ df_bb_regno_last_use_find (struct df *df
>    FOR_BB_INSNS_REVERSE (bb, insn)
>      {
>        unsigned int uid = INSN_UID (insn);
> -      for (use = DF_INSN_UID_GET (df, uid)->uses; use; use = use->next_ref)
> +      struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
> +      if (!insn_info)
> +	continue;
> +
> +      for (use = insn_info->uses; use; use = use->next_ref)
>  	if (DF_REF_REGNO (use) == regno)
>  	  return use;
>      }
> @@ -938,7 +942,11 @@ df_bb_regno_first_def_find (struct df *d
>    FOR_BB_INSNS (bb, insn)
>      {
>        unsigned int uid = INSN_UID (insn);
> -      for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
> +      struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
> +      if (!insn_info)
> +	continue;
> +
> +      for (def = insn_info->defs; def; def = def->next_ref)
>  	if (DF_REF_REGNO (def) == regno)
>  	  return def;
>      }
> @@ -957,8 +965,11 @@ df_bb_regno_last_def_find (struct df *df
>    FOR_BB_INSNS_REVERSE (bb, insn)
>      {
>        unsigned int uid = INSN_UID (insn);
> +      struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
> +      if (!insn_info)
> +	continue;
>  
> -      for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
> +      for (def = insn_info->defs; def; def = def->next_ref)
>  	if (DF_REF_REGNO (def) == regno)
>  	  return def;
>      }

I count six uses of DF_INSN_UID_GET in df-core.c.  You are fixing
three of them.  Is there any reason not to fix the other three?

There should be a blank line between the declaration of insn_info and
the test of insn_info.

In general this patch appears to be independent of the rest of the
code.  Can you separate this part out and run it past Ken Zadeck and
Dan Berlin?  I don't know enough to know whether this is fixing a bug
in the DF code or a bug in how the DF code is being used.  I note that
the code on dataflow-branch still assumes that DF_INSN_UID_GET is
always non-NULL.


> +static bool
> +can_simplify_addr (rtx addr)
> +{
> +  rtx reg;
> +
> +  if (GET_CODE (addr) == PLUS)
> +    reg = XEXP (addr, 0);
> +  else
> +    reg = addr;
> +
> +  return !REG_P (reg)
> +	 || (REGNO (reg) > FIRST_PSEUDO_REGISTER
> +	     && REGNO (reg) != FRAME_POINTER_REGNUM
> +	     && REGNO (reg) != HARD_FRAME_POINTER_REGNUM
> +	     && REGNO (reg) != ARG_POINTER_REGNUM);
> +}

This can't be right.  FRAME_POINTER_REGNUM, HARD_FRAME_POINTER_REGNUM,
and ARG_POINTER_REGNUM are always < FIRST_PSEUDO_REGISTER.

The test against FIRST_PSEUDO_REGISTER should be >=, not >.  Better
would be to use HARD_REGISTER_P.

There should be a parenthesis around the whole expression.

Perhaps this is intended to be
  return (!REG_P (reg)
	  || (!HARD_REGISTER_P (reg)
	      || (REGNO (reg) != FRAME_POINTER_REGNUM
		  && REGNO (reg) != HARD_FRAME_POINTER_REGNUM
		  && REGNO (reg) != ARG_POINTER_REGNUM)));
which can be simplified to
  return (!REG_P (reg)
	  || (REGNO (reg) != FRAME_POINTER_REGNUM
	      && REGNO (reg) != HARD_FRAME_POINTER_REGNUM
	      && REGNO (reg) != ARG_POINTER_REGNUM));

Of course such a change would have to be retested.

In general this looks like an attempt to rewrite the test near the
start of find_best_addr in cse.c.  If that is the case, why is it OK
to not test CONSTANT_ADDRESS_P?

> +/* Returns a canonical version of X for the address, from the point of view,
> +   that all multiplications are represented as MULT instead of the multiply
> +   by a power of 2 being represented as ASHIFT.
> +
> +   Every ASHIFT we find has been made by simplify_gen_binary and was not
> +   there before, so it is not shared.  So we can do this in place.  */

How can you be sure of that?  Doesn't that assume that no target will
use ASHIFT when representing an address?

> +/* Inside INSN, the expression rooted at *LOC has been changed, moving some
> +   uses from ORIG_USES.  Find those that are present, and create new items
> +   in the data flow object of the pass.  Mark any new uses as having the
> +   given TYPE.  */
> +static void
> +update_df (rtx insn, rtx *loc, struct df_ref *orig_uses, enum df_ref_type type,
> +	   int new_flags)
> +{
> +  struct df_ref *use;
> +
> +  /* Else, add a use for the registers that were propagated.  */

Why "Else"?

> +/* Try substituting NEW into LOC, which originated from forward propagation
> +   of USE's value from NEW_DEF_INSN.  SET_REG_EQUAL says whether we are
> +   substituting the whole SET_SRC, so we can set a REG_EQUAL note if the
> +   new insn is not recognized.  Return whether the substitution was
> +   performed.  */
> +
> +static bool
> +subst (struct df_ref *use, rtx *loc, rtx new, rtx def_insn, bool set_reg_equal)

I think the name "subst" is too generic, and in particular combine.c
has a function named "subst".  Please use a different name.


So, please split out the df-core.c changes, and please make the
changes above (or explain why they should not be made) and repost.

Thanks, and sorry again for how long it has taken to look at these
patches.

Ian