PATCH: use type-safe vectors for basic block edges

[Zdenek Dvorak]

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.

which you can do with FOR_EACH_OPERAND-type implementation as well --
simply move the checking code to the "next" iterator.  In fact
this approach has the advantage that in case there is "continue"
inside the loop body, the checking code will not be skipped.

> 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.

All of which are hidden inside the iterator.  So only thing you need to
do is to declare and pass the single variable for the iterator.  Which
seems as easy to me as to write END_FOR_EACH_EDGE.  It also won't confuse
automatic indenation in editors.

Btw. it might be cleaner to have FOR_EACH_EDGE_PRED and
FOR_EACH_EDGE_SUCC that would take the basic block instead of the
edge vector as an argument.  If we sometimes in the future decide
to modify the representation of edge lists somehow, this would
simplify the change.

I.e. instead of

FOR_EACH_EDGE (e, bb->succs)
  code
END_FOR_EACH_EDGE

I think using macro looking like

edge_iterator ei;

FOR_EACH_EDGE_SUCC (bb, ei, e)
  {
    code;
  }

Or perhaps

FOR_EACH_EDGE (bb, ei, SUCCESSORS, e)
  {
    code;
  }

if you prefer to have just one macro for both successors and
predecessors.

> 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??)

Because I misunderstood what you ment; sorry.

> > 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.

OK, thanks.

Zdenek