law - Re: 2nd try for patch for automaton based pipeline hazardrecognizer

[Vladimir N. Makarov]

Bernd Schmidt wrote:

> On Fri, 20 Jul 2001, Vladimir Makarov wrote:
>
> > - /* During sched, for the LOG_LINKS of an insn, these cache the adjusted
> > -    cost of the dependence link.  The cost of executing an instruction
> > -    may vary based on how the results are used.  LINK_COST_ZERO is 1 when
> > -    the cost through the link varies and is unchanged (i.e., the link has
> > -    zero additional cost).  LINK_COST_FREE is 1 when the cost through the
> > -    link is zero (i.e., the link makes the cost free).  In other cases,
> > -    the adjustment to the cost is recomputed each time it is needed.  */
> > - #define LINK_COST_ZERO(X) ((X)->jump)
> > - #define LINK_COST_FREE(X) ((X)->call)
>
> Why eliminate these?
>

I wanted to remove this code long ago.  I'll try to remember why.

The comment does not correspond reality.  If you look at insn_cost, you will
see that the cost can be changed only once when cost is the same after the cost
adjustment.  E.g.  insn_cost may set up LINK_COST_ZERO in priority and the
adjustment will be not done in schedele_insn.   Also the code gives practically
nothing as a cache and only makes code ambiguous.

>
> > --- 2140,2159 ----
> >           if (! INSN_P (insn))
> >             continue;
> >
> > +         if (USE_AUTOMATON_PIPELINE_INTERFACE)
> > +           /* DFA pipeline hazard recognizer needs to have
> > +              non-negative insn code. */
> > +           recog_memoized (insn);
> > +
> >           if (!CANT_MOVE (insn)
> >               && (!IS_SPECULATIVE_INSN (insn)
> > !                 || ((0
> > !                      || (USE_AUTOMATON_PIPELINE_INTERFACE
> > !                          && INSN_CODE (insn) >= 0
> > !                          && min_insn_conflict_delay (curr_state, insn,
> > !                                                      insn) <= 3)
> > !                      || (!USE_AUTOMATON_PIPELINE_INTERFACE
> > !                          && insn_issue_delay (insn) <= 3))
> >                       && check_live (insn, bb_src)
> >                       && is_exception_free (insn, bb_src, target_bb))))
>
> This doesn't belong in sched-rgn.c.  This file only deals with issues
> specific to the multi-region scheduling code.  Anything that deals with
> the pipeline description should go into the caller.
>
> >     /* For speculative insns, before inserting to ready/queue,
> >        check live, exception-free, and issue-delay.  */
> >     if (INSN_BB (next) != target_bb
> >         && (!IS_VALID (INSN_BB (next))
> >         || CANT_MOVE (next)
> >         || (IS_SPECULATIVE_INSN (next)
> > !           && (0
> > !               || (USE_AUTOMATON_PIPELINE_INTERFACE
> > !                   && (INSN_CODE (next) < 0
> > !                       || min_insn_conflict_delay (curr_state, next,
> > !                                                   next) > 3))
> > !               || (!USE_AUTOMATON_PIPELINE_INTERFACE
> > !                   && insn_issue_delay (next) > 3)
> >                 || !check_live (next, INSN_BB (next))
> >                 || !is_exception_free (next, INSN_BB (next), target_bb)))))
> >       return 0;
>
> Likewise.
>
> I still think that we should rather extend varrays if they lack
> functionality, rather than wrap them inside yet another set of macros
> (the VLA_... stuff)
>

I'd love to expand the functionality.  But we could commit the DFA patch the
first.  And after that we could create the patch to expand VARRAY functionality
and use this functionality in genautomata.c.  It is better to solve problems
step by step.  The bigger patch, the harder its approval.

>
> I've looked at genautomaton.c somewhat, and I must say I'm still not very
> happy about the amount of documentation.  Most of the comments are
> one-liners at the top of functions which adhere to the letter rather than
> the spirit of the "one comment before each function" rule.  There ought
> to be some kind of high-level description of what's going on in this file,
> what algorithms are used, etc.  For many functions, it would help not only
> to know what it does (in more than one sentence), but also why it's needed
> (that probably falls under "document the algorithms").
>

You wrote about it and  I've added a brief description of the translator phases
in comments on the top of file genautomata.c.  I could add more details.

>
> A lesser problem (I don't consider fixing it a requirement, it's just
> annoying) which I mentioned earlier are the overly long variable names that
> make the code harder to read than necessary.  Examples:
>
>   set_el_t *unit_reserv_set_el_ptr_1;
>   set_el_t *unit_reserv_set_el_ptr_2;
>
> Apparently an attempt to have meaningful variable names - the only problem
> is that the names aren't really very helpful.  A comment describes what the
> variables are used for would help a lot more; and then you could just call
> them p1 and p2, reducing a lot of the subsequent visual clutter.
>

Actually this is a result of requirement (I don't remember from who) to decrease
length of the names.  Originally they were longer and more meaningful.  I wrote
this is my style.  I prefer to use long meaningful names instead of writing
comments for them.  This does not contradict the GNU coding rules.  I have no
problems to type this in emacs.  But I realize that it annoys people.  The same
problem was with hash tables.  Many names were changed or shortened after
committing the patch.  I didn't object this.

>
> Likewise for all the loop counters starting with "curr_".  For example, in
> function add_excls:
>
>   for (curr_dest_el = dest_list;
>        curr_dest_el != NULL;
>        curr_dest_el = curr_dest_el->next_unit_set_el)
>     for (curr_source_el = source_list;
>          curr_source_el != NULL;
>          curr_source_el = curr_source_el->next_unit_set_el)
>       {
>         if (curr_dest_el->unit_decl == curr_source_el->unit_decl)
>           {
>             error ("unit `%s' excludes itself",
>                    curr_source_el->unit_decl->name);
>             continue;
>           }
>         for (curr_excl_list_unit = curr_dest_el->unit_decl->excl_list,
>              prev_excl_list_unit = NULL;
>              curr_excl_list_unit != NULL;
>              prev_excl_list_unit = curr_excl_list_unit,
>              curr_excl_list_unit = curr_excl_list_unit->next_unit_set_el)
>           if (curr_excl_list_unit->unit_decl == curr_source_el->unit_decl)
>             break;
>
> This would be easier to read if less vertical space was wasted.
>
> We might want to think about checking this into a branch.
>
> Bernd