Re: [PATCH 10/18] merge adjust_cost and adjust_cost_2 target hooks

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Wed, Apr 20, 2016 at 07:17:03AM -0500, Segher Boessenkool wrote:
> On Wed, Apr 20, 2016 at 02:22:14AM -0400, tbsaunde+gcc@tbsaunde.org wrote:
> > 	* config/microblaze/microblaze.c (microblaze_adjust_cost):
> > 	* Likewise.
> 
> Stray * (here and elsewhere).
> 
> > --- a/gcc/config/alpha/alpha.c
> > +++ b/gcc/config/alpha/alpha.c
> > @@ -4758,14 +4758,15 @@ alpha_split_atomic_exchange_12 (rtx operands[])
> >     a dependency LINK or INSN on DEP_INSN.  COST is the current cost.  */
> >  
> >  static int
> > -alpha_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost)
> > +alpha_adjust_cost (rtx_insn *insn, int dep_type, rtx_insn *dep_insn, int cost,
> 
> Why an int and not enum reg_note?

I think I just copied what adjust_cost_2 did, but using the enum makes
sense.

> 
> > +		   unsigned int)
> >  {
> >    enum attr_type dep_insn_type;
> >  
> >    /* If the dependence is an anti-dependence, there is no cost.  For an
> >       output dependence, there is sometimes a cost, but it doesn't seem
> >       worth handling those few cases.  */
> > -  if (REG_NOTE_KIND (link) != 0)
> > +  if (dep_type != 0)
> >      return cost;
> 
> From reg-notes.def:
> 
> /* REG_DEP_TRUE is used in scheduler dependencies lists to represent a
>    read-after-write dependency (i.e. a true data dependency).  This is
>    here, not grouped with REG_DEP_ANTI and REG_DEP_OUTPUT, because some
>    passes use a literal 0 for it.  */
> REG_NOTE (DEP_TRUE)
> 
> Get rid of the literal 0 while you're at it?  Some places already have
> REG_DEP_TRUE.

not entirely related to what the patch is doing, but not a bad idea.

> > @@ -4486,7 +4487,7 @@ c6x_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost)
> >    if (insn_code_number >= 0)
> >      insn_type = get_attr_type (insn);
> >  
> > -  kind = REG_NOTE_KIND (link);
> > +  kind = (reg_note) dep_type;
> 
> Maybe it's just me, but it would look a lot less confusing with "enum".

well, if you change the arg to be an enum then it goes away I think.

> >  static int
> > -mips_adjust_cost (rtx_insn *insn ATTRIBUTE_UNUSED, rtx link,
> > -		  rtx_insn *dep ATTRIBUTE_UNUSED, int cost)
> > +mips_adjust_cost (rtx_insn *, int dep_type, rtx_insn *, int cost, unsigned int)
> >  {
> > -  if (REG_NOTE_KIND (link) == REG_DEP_OUTPUT
> > -      && TUNE_20KC)
> > -    return cost;
> > -  if (REG_NOTE_KIND (link) != 0)
> > +  if (dep_type != 0 && (dep_type != REG_DEP_OUTPUT || !TUNE_20KC))
> >      return 0;
> >    return cost;
> >  }
> 
> The original logic was a lot more readable (test positives, not negatives).

 I'm not sure what I think there.

> > +as a data-dependence.  If the scheduler using the automaton based pipeline
> >  description, the cost of anti-dependence is zero and the cost of
> >  output-dependence is maximum of one and the difference of latency
> >  times of the first and the second insns.  If these values are not
> 
> "is using" (pre-existing, but hey).

makes sense I guess.

> So I wonder how much is gained by adding an extra unused argument to so
> many places.

Well that's just a side effect of merging the two hooks, so I'm not sure
what else you'd do other than not pass the arg to ia64, I'm not sure how
important it is there.  On the other hand the unused arg probably isn't
important relative to the indirect call to the hook.

Trev

> 
> 
> Segher