Re: [patch] Some operands building cleanups

[Andrew MacLeod <amacleod at redhat dot com>]

On Thu, 2006-03-09 at 01:35 +0100, Zdenek Dvorak wrote:
> Hello,
> 
> -- there seems to be no reason for operand_memory to be ggc allocated

At the time it was faster for whatever reason, perhaps just collection
noise, but it was consistently slightly faster.

> -- type of build_defs and build_uses is changed to VEC(tree_ptr), so
>    that we do not have to cast the values stored there from and to tree *

sure, now that the functions are broken out.

> -- finalize_ssa_def_ops and finalize_ssa_use_ops compare pointers that
>    do not belong to the same object by <, which is undefined.  However,
>    since the real operand lists are not sorted anyway, this is
>    unnecessary.

They are sorted so we can avoid a doubly nested loop.  Your changes
aren't right now. at least before there was an ordering to them, even if
it isn't officially defined.  With your change, if we have a case where
the old stmt had 2 uses(or defs), and the stmt was changed to have a
third one which occurs between the two existing ones, your loop will
eliminate the second of the original elements.

ie  if
   = s_3, f_4
became
   = s_3, g_9, f_4

because there is no ordering whatsoever, you would have to scan the
entire list to see if f_4 is in the list. Since you don't do that with
this change, when f_4 is compared to g_9, f_4 will be added to the free
list, then new operands will be created for g_9 and another for f_4
because it is no longer in the list. 

if this happens during an operand iterator loop over f_4, the immediate
uses for f_4 will get buggered up because the old one was removed and a
new one created. Placement within the list will not be preserved.

if you dont impose an ordering of some sort on the pointers, you will
have to turn this into doubly nested loop.  what is the official
language rules for comparing pointers with < or >?  can they be cast to
ints and compared?  It is rumored that casting to (intptr_t)
historically works within gcc.  I don't care if < or > compares truly <
and >, as long as there is a predictable ordering.  otherwise we either
need the double loop or you could base the loop comparisons on the
variable decl_uid/ssa-name like virtual operands do. You would also need
to compare pointers to see if there is more than one (with slight
lookahead to see if there are other pointers with the same variable in
it). As it stands, this is incorrect.

Andrew
 
>   static inline void
>   finalize_ssa_def_ops (tree stmt)
> *************** finalize_ssa_def_ops (tree stmt)
> *** 510,518 ****
>     old_ops = DEF_OPS (stmt);
>   
>     new_i = 0;
> !   while (old_ops && new_i < VEC_length (tree, build_defs))
>       {
> !       tree *new_base = (tree *) VEC_index (tree, build_defs, new_i);
>         old_base = DEF_OP_PTR (old_ops);
>   
>         if (old_base == new_base)
> --- 516,524 ----
>     old_ops = DEF_OPS (stmt);
>   
>     new_i = 0;
> !   while (old_ops && new_i < VEC_length (tree_ptr, build_defs))
>       {
> !       tree *new_base = VEC_index (tree_ptr, build_defs, new_i);
>         old_base = DEF_OP_PTR (old_ops);
>   
>         if (old_base == new_base)
> *************** finalize_ssa_def_ops (tree stmt)
> *** 521,542 ****
>   	  MOVE_HEAD_AFTER (old_ops, last);
>   	  new_i++;
>   	}
> -       else if (old_base < new_base)
> - 	{
> - 	  /* if old is less than new, old goes to the free list.  */
> - 	  MOVE_HEAD_TO_FREELIST (old_ops, def);
> - 	}
>         else
>   	{
> ! 	  /* This is a new operand.  */
>   	  add_def_op (new_base, &last);
>   	  new_i++;
>   	}
>       }
>   
>     /* If there is anything remaining in the build_defs list, simply emit it.  */
> !   for ( ; new_i < VEC_length (tree, build_defs); new_i++)
> !     add_def_op ((tree *) VEC_index (tree, build_defs, new_i), &last);
>   
>     last->next = NULL;
>   
> --- 527,544 ----
>   	  MOVE_HEAD_AFTER (old_ops, last);
>   	  new_i++;
>   	}
>         else
>   	{
> ! 	  /* Remove the old operand and add the new one.  */
> ! 	  MOVE_HEAD_TO_FREELIST (old_ops, def);
>   	  add_def_op (new_base, &last);
>   	  new_i++;
>   	}
>       }
>   
>     /* If there is anything remaining in the build_defs list, simply emit it.  */
> !   for ( ; new_i < VEC_length (tree_ptr, build_defs); new_i++)
> !     add_def_op (VEC_index (tree_ptr, build_defs, new_i), &last);
>   
>     last->next = NULL;
>