Re: Patch for automaton based pipeline hazard recognizer (part #1).

[Vladimir Makarov <vmakarov at redhat dot com>]

Bernd Schmidt wrote:
> 
> On Wed, 31 Jan 2001, Vladimir Makarov wrote:
> 
> >   Hello, we'd like to contribute new code for more accurate
> > description of pipeline behavior of processors and for fast
> > recognition of pipeline hazards.
> 
> To get this started, I've begun reviewing parts of this code.  I haven't
> really looked at genautomata.c, that part is still very opaque to me, but
> I have a few comments about the rest of the code.
> 
> > 2001-01-31  Vladimir Makarov  <vmakarov@touchme.toronto.redhat.com>
> 
> I'm not happy about the amount of ifdefs this adds to the scheduler code.
> In many cases there are obvious ways to avoid them; I'll point out some
> of them below.
>

Ok I'll try to minimize them.
 
> > + /*--Start of constructions for CPU pipeline description described by NDFAs.--*/
> 
> Comments shouldn't be specially formatted.  Spaces instead of "--".
> 
> > + /* (exclusion_set string string) means that each CPU function unit in
> > +    the first string can not be reserved simultaneously with each unit
> > +    whose name is in the second string and vise versa.  CPU units in
> 
> "vice versa".
> 
> > + #include "genautomata.c"
>

  You are absolutely right. I'll fix it.  Originally, code in
genautomata.c was part of the genattrtab.c.  That was quick fix of Nick
Clifton to simplify gcc->redhat repository merge.

> It would be nicer if this were a first-class C file on its own, rather
> than being #included.
> 
> > + /* If the following macro value is nonzero, we will make multi-pass
> > +    scheduling for the first cycle. */
> > + #ifndef FIRST_CYCLE_MULTIPASS_SCHEDULING
> > + #define FIRST_CYCLE_MULTIPASS_SCHEDULING 0
> > + #endif
> > +
> > + #ifndef FIRST_CYCLE_MULTIPASS_SCHEDULING_LOOKAHEAD
> > + #define FIRST_CYCLE_MULTIPASS_SCHEDULING_LOOKAHEAD 0
> > + #endif
> 
> I'd like more commentary for these two.  What do they do?
> They also ought to be documented in tm.texi.
> 

Ok.

> > + /* The following is for safety to support the
> > +    two pipeline description interfaces.  */
> > + #if !OLD_PIPELINE_INTERFACE
> > +
> > + #undef FUNCTION_UNITS_SIZE
> > + #undef BLOCKAGE_BITS
> > + #undef MAX_MULTIPLICITY
> > + #undef MIN_BLOCKAGE
> > + #undef MAX_BLOCKAGE
> 
> Why #undef them?  IMO this is unnecessary clutter.
>
  It is for additional precaution if there is only automaton based
pipeline description.  If the scheduler code using old pipeline
description vocationally is switched on, the compiler will generate a
message.
 
> > *************** struct haifa_insn_data
> > *** 171,179 ****
> > --- 227,237 ----
> >        the ready queue when its counter reaches zero.  */
> >     int dep_count;
> >
> > + #if OLD_PIPELINE_INTERFACE
> >     /* An encoding of the blockage range function.  Both unit and range
> >        are coded.  */
> >     unsigned int blockage;
> > + #endif
> >
> >     /* Number of instructions referring to this insn.  */
> >     int ref_count;
> 
> I tend to prefer not adding ifdefs to hide data members unless spectacular
> space savings are possible.  They add clutter for little gain, but this is
> a matter of taste and I wouldn't be surprised if lots of people here
> disagreed with me.
>

I am agree but this is not for saving memory space (which is
insignificant) -- see above my comments about undef.

> >   {
> >     register int cost = INSN_COST (insn);
> >
> > !   if ((!USE_AUTOMATON_PIPELINE_INTERFACE && cost == 0)
> > !       || (USE_AUTOMATON_PIPELINE_INTERFACE && cost < 0))
> >       {
> >         recog_memoized (insn);
> >
> > ! #if OLD_PIPELINE_INTERFACE
> > !
> > !       if (!USE_AUTOMATON_PIPELINE_INTERFACE)
> >       {
> > !       /* A USE insn, or something else we don't need to
> > !          understand.  We can't pass these directly to
> > !          result_ready_cost because it will trigger a fatal error
> > !          for unrecognizable insns.  */
> > !       if (INSN_CODE (insn) < 0)
> > !         {
> > !           INSN_COST (insn) = 1;
> > !           return 1;
> > !         }
> > !       else
> > !         {
> > !           cost = result_ready_cost (insn);
> > !
> > !           if (cost < 1)
> > !             cost = 1;
> > !
> > !           INSN_COST (insn) = cost;
> > !         }
> >       }
> > !
> > ! #endif /* #if OLD_PIPELINE_INTERFACE */
> >
> > ! #if AUTOMATON_PIPELINE_INTERFACE
> >
> > !       if (USE_AUTOMATON_PIPELINE_INTERFACE)
> > !     {
> > !       /* A USE insn, or something else we don't need to
> > !          understand.  We can't pass these directly to
> > !          result_ready_cost or insn_default_latency because it will
> > !          trigger a fatal error for unrecognizable insns.  */
> > !       if (INSN_CODE (insn) < 0)
> > !         {
> > !           INSN_COST (insn) = 0;
> > !           return 0;
> > !         }
> > !       else
> > !         {
> > !           cost = insn_default_latency (insn);
> > !
> > !           if (cost < 0)
> > !             cost = 0;
> > !
> > !           INSN_COST (insn) = cost;
> > !         }
> >       }
> > +
> > + #endif /* #if AUTOMATON_PIPELINE_INTERFACE */
> > +
> >       }
> 
> This kind of code duplication is common in the patch, and I'm not really
> happy about it.  This sort of thing is IMO much better solved by identifying
> the part that needs to be different and giving it a name - in this case
> something like MIN_INSN_COST for the 1/0 difference, and a common function
> name for insn_default_latency/result_ready_cost.  This could be solved e.g.
> with a table of function pointers as it's done right now for the ebb/rgn
> scheduling code.
>

Agree,  I should made more modification to the patch.  The original
reason of this is in gcc->redhat repository merging problems.

  As for minimal cost, the current scheduler doesn't take into account
that there are processors where insn latency is zero (of course
logically).  We could make that the scheduler using old pipeline
interface also could be adequate to such processors.

> Do we actually need to have a difference in the minimum cost of an
> instruction, or could either of the two be changed so that we end up
> with a consistent value?
> 
> > !   this_priority = INSN_PRIORITY (insn);
> > !   if ((this_priority == 0 && !USE_AUTOMATON_PIPELINE_INTERFACE)
> > !       || (this_priority < 0 && USE_AUTOMATON_PIPELINE_INTERFACE))
> 
> Likewise.  Is there a way to make the two implementations consistent?
> 
> > + #if AUTOMATON_PIPELINE_INTERFACE
> > +       if (USE_AUTOMATON_PIPELINE_INTERFACE)
> 
> I believe the #if is unnecessary - if AUTOMATON_PIPELINE_INTERFACE is 0,
> then USE_AUTOMATON_PIPELINE_INTERFACE will be 0 as well, right?

  You can have two description in md file (old and new).  If there is a
new description,  AUTOMATON_PIPELINE_INTERFACE will equal 1. 
USE_AUTOMATON_PIPELINE_INTERFACE says about should we use the new
pipeline description or not.

  So, You can describe one processor subtarget with the old model, and
another one with automaton based description and define
USE_AUTOMATON_PIPELINE_INTERFACE equal to zero for the first subtarget
and 1 for the second one.

> 
> > ! #if OLD_PIPELINE_INTERFACE
> > !       if (!USE_AUTOMATON_PIPELINE_INTERFACE)
> > !     /* Sort the ready list based on priority.  */
> > !     ready_sort (&ready);
> > ! #endif
> 
> Likewise (and in many other cases).
> 
> > +                     && min_insn_conflict_delay (curr_state, insn, insn) <= 3
> 
> I have been unable to find documentation for this function.  I am assuming
> none of the autogenerated functions are documented, and they should be.
> 

The old pipeline interface is also undocumented.  I am not sure that it
is really needed because they are used only for scheduler.  But I could
output comments too for this generated functions.

> > +                     && check_live (insn, bb_src)
> > +                     && is_exception_free (insn, bb_src, target_bb))))
> > +           move_p = 0;
> > + #endif /* #if AUTOMATON_PIPELINE_INTERFACE */
> > +
> > + #if OLD_PIPELINE_INTERFACE
> > +         if (!USE_AUTOMATON_PIPELINE_INTERFACE && !CANT_MOVE (insn)
> > +             && (!IS_SPECULATIVE_INSN (insn)
> >                   || (insn_issue_delay (insn) <= 3
> >                       && check_live (insn, bb_src)
> >                       && is_exception_free (insn, bb_src, target_bb))))
> > +           move_p = 0;
> > + #endif
> 
> Another example of unnecessary code duplication.
> 
> > + /* The function removes marks about start of new cycle made in the first
> > +    instruction scheduling.  Although regmove may remove them too.  */
> > +
> > + static void
> > + remove_new_cpu_cycle_marks (bb)
> 
> Why is this necessary?  Does any code care about the modes of insns?
>

Yes, Fujitsu VLIW compiler uses it for VLIW insn packing. I think, it is
also nice to see information about starting new cpu cycle in rtl dump.

 
> > + getruntime.o: $(srcdir)/../libiberty/getruntime.c $(CONFIG_H)
> > +     rm -f getruntime.c
> > +     $(LN_S) $(srcdir)/../libiberty/getruntime.c getruntime.c
> > +     $(CC) -c $(ALL_CFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) getruntime.c
> > +
> 
> Do we really need this?
> 

  It is nice to see the automaton time generation (it is possibly to
write description for which automaton will be generated 10 or more
minutes).

> The documentation needs to be written more carefully; I'll point out just
> a few of the spelling mistakes.
>

Thanks, I'll fix it.

> > + @findex define_bypass
> > + The following construction is used to describe exceptions in latency
> > + time for given instruction pair.  This is so called bypasses.
> > +
> > + @smallexample
> > + (define_reservation @var{number} @var{out_insn_names} @var{in_insn_names}
> > +                     [@var{guard}])
> > + @end smallexample
> 
> I think this should be a define_bypass, not a define_reservation.
> 

> > + Usually the following tree constructions are used to describe VLIW
> 
> "three"
> 
> > + processors (more correctly to describe placement of small insns into
> > + VLIW insn slots).  Although they can be used for RISC processor too.
> > +
> > + @smallexample
> > + (exclussion_set @var{unit-names} @var{unit-names})
> 
> "exclusion"
> 
> > + (presense_set @var{unit-names} @var{unit-names})
> 
> "presence"
> 
> > + (absense_set @var{unit-names} @var{unit-names})
> 
> "absence"
> 
> > + @end smallexample
> > +
> > + @var{unit-names} is string giving names of functional units separated by comma.
> > +
> > + The first construction means that each functional unit in the first
> > + string can not be reserved simultaneously with unit whose name is in
> > + the second string and vise versa.  For example, the construction is
> 
> "vice versa"
> 
> > + useful for description processors (e.g. some SPARC processors) with
> 
> "describing"
> 
> > + The second construction means that each functional unit in the first
> 
> Rather than referring to them by first, second, third, it would be more
> readable to refer to them by name (exclusion_set, presence_set, absence_set).
> 
> > + @dfn{w} means generation of file describing the result automaton.  The
> > + file can be used to the description verification.
> 
> "to verify the description".
> 
> > + The old instruction level parallelism description and pipeline hazards
> > + recognizer based on it have the following drawbacks in comparisons
> > + with new one:
> 
> People regularly point out that "old" and "new" are somewhat problematic
> labels.  Maybe "DFA-based" instead of "new".
> 
> > + (@pxref{Processor pipeline description,,Specifying processor pipeline
> 
> Only one comma.
> 

Bernd, Thanks for the very useful patch review.

Vlad