Re: [PATCH][doc] Improve pipeline description docs a bit

[Sandra Loosemore <sandra@codesourcery.com>]

On 04/20/2015 04:31 AM, Kyrill Tkachov wrote:
> Hi all,
>
> This patch attempts to improve the pipeline description documentation.
> It fixes some grammar errors,typos and clarifies some concepts.
>
> The sections on the syntactic constructs are formatted to have a
> small description, and example, description of syntax elements and some
> elaboration.
>
> Is this ok for trunk?
>
> Thanks,
> Kyrill
>
> 2014-04-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
> 	* doc/md.texi (Specifying processor pipeline description):
> 	Improve wording.
> 	Clarify some constructs.

Hmmmm.  I guess overall this is an improvement, but I still see quite a 
few things that need tweaking (and I wasn't even looking very hard).

> +latency time}.  Instructions may not complete execution until all inputs
> +to the instruction have been evaluated and are available for use.
> +Taking data dependence delays into account is simple.

I don't think the above sentence adds anything and could be deleted.

> +The data dependence (true, output, and anti-dependence) delay between two
> +instructions is modelled as being constant.  In most cases this approach is
> +adequate.  The second kind of interlock delays is a reservation delay.
> +The reservation delay means that two or more executing instructions will require

s/will require/require/

> +
> +The define_automaton construct declares the names of automata.
> +It takes the following form:
>
>  @smallexample
>  (define_automaton @var{automata-names})
>  @end smallexample
>
>  @var{automata-names} is a string giving names of the automata.  The
> -names are separated by commas.  All the automata should have unique names.
> -The automaton name is used in the constructions @code{define_cpu_unit} and
> -@code{define_query_cpu_unit}.
> +names are separated by commas.  All the automata must have unique names.
> +The automaton name is used to bind @code{define_cpu_unit} and
> +@code{define_query_cpu_unit} constructs to specific automata.
> +
> +This construct declares the names of automata.

You already said that a few sentences above; delete this one.

> +The define_query_cpu_unit construct can be used to define units

Add @code{} markup here.

> -@var{default_latency} is a number giving latency time of the
> +@var{default_latency} is a number giving the latency of the
>  instruction.  There is an important difference between the old
>  description and the automaton based pipeline description.  The latency
> -time is used for all dependencies when we use the old description.  In
> -the automaton based pipeline description, the given latency time is only
> -used for true dependencies.  The cost of anti-dependencies is always
> -zero and the cost of output dependencies is the difference between
> -latency times of the producing and consuming insns (if the difference
> -is negative, the cost is considered to be zero).  You can always
> -change the default costs for any description by using the target hook
> +is used for all types of dependencies when we used the old description.  In
> +the automaton based pipeline description, the  latency is only taken into
> +account when analysing true dependencies (i.e. not output or
> +anti-dependencies).  The cost of anti-dependencies is always zero and the
> +cost of output dependencies is the difference between the latencies
> +of the producing and consuming insns (if the difference is negative, the
> +cost is considered to be zero).  You can always change the default cost
> +between any pair of insns by using the target hook
>  @code{TARGET_SCHED_ADJUST_COST} (@pxref{Scheduling}).

Here I am confused.  What is the "old description"?  If this is a 
leftover of some obsolete way of doing things, the references to it 
should be deleted.

> +construct.  You must avoid having more than one
> +@code{define_insn_reservation} matching any one RTL insn, as the behaviour is

s/behaviour/behavior/

> +The following construct is used to describe a bypass i.e. an exception
> +in the execution latency between a pair of instructions:

@dfn{bypass} ??

>  @var{guard} is an optional string giving the name of a C function which
> -defines an additional guard for the bypass.  The function will get the
> +defines an additional guard for the bypass.  The function will take the
>  two insns as parameters.  If the function returns zero the bypass will
>  be ignored for this case.  The additional guard is necessary to

s/will take/takes/
s/will be ignored/is ignored/

> +If there is more one bypass with the same output and input insns, the
> +chosen bypass is the first bypass with a guard function in its definition that
> +returns nonzero.  If there is no such bypass, then a bypass without a guard
> +function is chosen.  These constructs can be used to describe, for example,
> +forwarding paths in a processor pipeline.

I don't understand what the last sentence has to do with the rest of 
this paragraph.  If this is part of the general discussion of what 
define_bypass does, it should be moved up to the paragraph where the 
concept of a bypass is introduced.

> -@var{unit-names} is a string giving names of functional units
> -separated by commas.
> +@var{unit-names} is a comma-separated string giving the names of functional
> +units.

Looking at examples, I think the original text is less confusing:  it is 
a string in which the names are separated by commas.

> -@var{patterns} is a string giving patterns of functional units
> -separated by comma.  Currently pattern is one unit or units
> -separated by white-spaces.
> +@var{patterns} is a comma-separated string specifying the patterns of
> +functional units.  Each pattern can be the name of a functional unit or a
> +whitespace-separated list of functional units.  This is described by the
> +following simple syntax:
> +
> +@smallexample
> +patterns = pattern "," pattern
> +
> +pattern = cpu_function_unit_name cpu_function_unit_name
> +          | cpu_function_unit_name
> +@end smallexample

I'm not sure this is an improvement either.  How about

@var{patterns} is a string specifying a list of patterns of functional 
units.  Patterns in the list are separated by commas.  Each pattern can 
be the name of a functional unit or a whitespace-separated list of 
functional unit names.

> -The first construction (@samp{exclusion_set}) means that each
> +The first construct (@samp{exclusion_set}) means that each

Here I'd just say

The @code{exclusion_set} construct means that each

and likewise for the second, etc of this enumeration.  Also note that 
all the markup on these names should be @code rather than @samp.

> -@var{options} is a string giving options which affect the generated
> -code.  Currently there are the following options:
> +@var{options} is a string giving specifying an option.
> +Currently the following options are available:

Does the implementation in fact recognize multiple options here?  If 
it's only a single option, we should use "@var{option}" here.  (It looks 
like all existing uses specify only a single option.)

And in the descriptions of the individual options, e.g.

>  @itemize @bullet
>  @item
>  @dfn{no-minimization} makes no minimization of the automaton.  This is
> -only worth to do when we are debugging the description and need to
> -look more accurately at reservations of states.
> +only worth doing when we are debugging the description and need to
> +look more accurately at the reservations of states.

These are constant strings and should be marked up as e.g. 
@code{"no-minimization"} instead of using @dfn{} markup.

-Sandra