This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

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


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.

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

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.

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

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

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

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?

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

> + 			&& 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?

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

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

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]