[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCH/RFC/PR28071]: New scheduler dependencies lists.
From:       Maxim Kuvyrkov <mkuvyrkov () ispras ! ru>
Date:       2007-01-31 14:46:55
Message-ID: 45C0ABDF.8050500 () ispras ! ru
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]