Re: [PATCH/RFC/PR28071]: New scheduler dependencies lists.

[Maxim Kuvyrkov <mkuvyrkov at ispras dot ru>]

Ayal Zaks wrote:

> In some cases (indicated below, there could be others) you may want to
> verify that a given list is backward, or is forward. One simple (necessary
> and insufficient) property to check is that all CON (PRO) fields are the
> same, say by deps_list_consistent_back_p(list) and
> deps_list_consistent_forw_p(list). Then deps_list_consistent_p could also
> check if deps_list_consistent_back_p || deps_list_consistent_forw_p.
>
> > +/* Add a dependency described by DEP to the list L.
> > +   L should be either INSN_BACK_DEPS or INSN_RESOLVED_BACK_DEPS.  */
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +void
> > +add_back_dep_to_deps_list (deps_list_t l, dep_t dep_from)
> > +{
> > +  dep_node_t n = (dep_node_t) obstack_alloc (dn_obstack,
> > +                    sizeof (*n));
> > +  dep_t dep_to = DEP_NODE_DEP (n);
> > +  dep_link_t back = DEP_NODE_BACK (n);
> > +  dep_link_t forw = DEP_NODE_FORW (n);
> > +
>      gcc_assert (deps_list_consistent_back_p (l)); ??

I had this kind of check here, but it showed terrible compile time 
performance, so I removed it.  *_consistent_* function were introduced 
to find few bugs during the development and now remain for the purpose 
of someone someday using them from debugger.

...

> > +/* Make a copy of FROM in TO with substituting consumer with CON.
> > +   TO and FROM should be RESOLVED_BACK_DEPS lists.  */
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +void
> > +copy_deps_list_change_con (deps_list_t to, deps_list_t from, rtx con)
> > +{
> > +  dep_link_t l;
> > +
> > +  gcc_assert (deps_list_empty_p (to));
>      gcc_assert (deps_list_consistent_back_p (from)); ??

Same.

...

> You may find it useful to also provide FOR_EACH_PRO(PRO, LINK, CON) and
> FOR_EACH_CON(PRO, LINK, CON) macros, e.g. something like
>
> +#define FOR_EACH_PRO(PRO, LINK, CON) \
> +  for (LINK = DEPS_LIST_FIRST (INSN_BACK_DEPS (CON)), \
> +       PRO = (LINK? DEP_LINK_PRO (LINK) : NULL); \
> +       LINK != NULL; \
> +       LINK = DEP_LINK_NEXT (LINK), \
> +       PRO = (LINK? DEP_LINK_PRO (LINK) : NULL))
>
> or so, to make sure people take PRO when traversing (RESOLVED?)_BACK_DEPS,
> and CON when traversing FORW_DEPS.

Personally, I don't like complicated macros especially those which have 
a comma in them.  And using an iterator I see as an overkill in this case.

> And a couple of typos:
>
> >  /* Remove both backward and forward dependencies between INSN and ELEM.
> */
> Update comment to refer to L instead.

Thanks.

> >  void
> > -delete_back_forw_dep (rtx insn, rtx elem)
> > +delete_back_forw_dep (dep_link_t l)
> >  {
> > +  dep_node_t n = DEP_LINK_NODE (l);
> > +

...

> > +  /* Pointer to the next field of the previous link in the list.
> > +     For the first link this points to the deps_list->first and for the
> last
> > +     one it is NULL.
> ^^^^^^^^^^^^^^^^^^^^^^^?
> Only need to comment regarding first link; for the last link (if it is not
> also first) also points to next field of the previous link in the list. You
> may want to comment that the next field of the last link is NULL, as
> probably intended.

Thanks.

> ...
>
> > +typedef struct _dep_link *dep_link_t;
> > +
> > +#define DEP_LINK_NODE(N) ((N)->node)
> > +#define DEP_LINK_NEXT(N) ((N)->next)
> > +#define DEP_LINK_PREV_NEXTP(N) ((N)->prev_nextp)
> > +
> > +/* Macros to work dep_link.  For most usecases only part of the
> dependency
> > +   information is need.  These macros conviniently provide that piece of
>                                          ^^^^^^^^^^^^

Thanks.

> In the diagram I sent you, forgot to indicate that forw_deps and back_deps
> with their "first" field are of type deps_list.

I think it is obvious and it is better not to add such small details to 
that stunning diagram you've made.


Thanks,

Maxim