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]
Other format: [Raw text]

Re: PATCH: use type-safe vectors for basic block edges


On Monday 13 September 2004 15:18, Zdenek Dvorak wrote:
> Hello,
>
> > +#define FOR_EACH_EDGE(EDGE,EDGE_VEC)					\
> > +do {									\
> > +  VEC(edge) *__ev = (EDGE_VEC);						\
> > +  edge __check_edge;							\
> > +  unsigned int __ix;							\
> > +  unsigned int  __num_edges = EDGE_COUNT (__ev);			\
> > +  (EDGE) = NULL;							\
> > +  for (__ix = 0; VEC_iterate (edge, __ev, __ix, (EDGE)); __ix++)	\
> > +    {									\
> > +      if (ENABLE_VEC_CHECKING)						\
> > +	__check_edge = (EDGE);
> > +
> > +#define END_FOR_EACH_EDGE						\
> > +      if (ENABLE_VEC_CHECKING						\
> > +	&& (__ix >= EDGE_COUNT (__ev)					\
> > +	    || EDGE_I (__ev, __ix) != __check_edge))			\
> > +	internal_error ("edge modified in FOR_EACH_EDGE: %s:%s",	\
> > +			__FILE__, __FUNCTION__);			\
> > +    }									\
> > +  if (ENABLE_VEC_CHECKING						\
> > +	  && __num_edges > EDGE_COUNT (__ev))				\
> > +	internal_error ("insufficient edges FOR_EACH_EDGE: %s:%s", 	\
> > +			__FILE__, __FUNCTION__);			\
> > +  if (ENABLE_VEC_CHECKING						\
> > +          && __num_edges < EDGE_COUNT (__ev))				\
> > +  	internal_error ("excess edges FOR_EACH_EDGE: %s:%s",		\
> > +			__FILE__, __FUNCTION__);			\
> > +}									\
> > +while (0)
>
> I do not think it is a good idea to introduce yet another way for
> FOR_EACH_... macros (and especially not this one; it looks too much
> like fortran to me :-).  If you really cannot do with simple
>
> FOR_EACH_EDGE (bb, e)
>   {
>     ... code ...
>   }
>
> type macro, please use the same way FOR_EACH_OPERAND macros do (i.e.
> encapsulate the variables you need to an iterator passed explicitly
> to the macro).

The first implementation had code like FOR_EACH_OPERAND, and it was
a complete disaster.  With the END_FOR_EACH_EDGE we could check that
the edge vector was not modified inappropriately, this was extremely
helpful for finding bugs.

For a FOR_EACH_OPERAND like implementation, you would have to pass
a basic block, an edge vector, an index to it, and an edge.  You also
have to add a variable declaration for each index variable.  That is
not very pretty either.  I think the way it's implemented now is the
best compromise between safety and ease of use.

(I showed you last week that the edge-vector branch implemented these
iterators like this, I thought you said it looked quite OK??)


> >  /* Control flow edge information.  */
> > -struct edge_def GTY((chain_next ("%h.pred_next")))
> > +struct edge_def GTY(())
> >  {
> > -  /* Links through the predecessor and successor lists.  */
> > -  struct edge_def *pred_next;
> > -  struct edge_def *succ_next;
> > -
> >    /* The two blocks at the ends of the edge.  */
> >    struct basic_block_def *src;
> >    struct basic_block_def *dest;
> > @@ -152,6 +150,13 @@ struct edge_def GTY((chain_next ("%h.pre
> >  };
>
> It might also be a good idea to keep the index of the edge in
> the pred/succ vector inside struct edge_def.  That way functions like
(...)
> in order to be able to access phi node arguments for a given edge in a
> constant time anyway, which should be the primary purpose of the patch.
> Or did you implement a different way how to achieve this?

No, I talked about that with Ben, we'll add it when the start adding
the simpler PHI argument stuff.  When we do that we can fix the above.
Let's please get the patch in as-is first.

Gr.
Steven



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