Re: [patch] Predictive commoning, updated

[Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>]

Hello,

some progress report, since it took me rather long time to get to
including your remarks.  I have run into some problems with
data reference analysis (and I do not see a simple clear solution;
either I can hack over them, or rewrite half of tree-data-ref.c;
slowly I am getting persuaded that the latter is a better choice).

> Would you be willing to be the maintainer of this pass?

yes

> > + /* Hash function for struct name_expansion.  */
> > + 
> > + static hashval_t
> > + name_expansion_hash (const void *e)
> > + {
> > +   return SSA_NAME_VERSION (((struct name_expansion *) e)->name);
> > + }
> > + 
> > + /* Equality function for struct name_expansion.  The second argument is an
> > +    SSA name.  */
> > + 
> > + static int
> > + name_expansion_eq (const void *e, const void *n)
> > + {
> > +   return ((struct name_expansion *) e)->name == n;
> > + }
> > + 
> 
> Why not use a dense array indexed by SSA name?  Or a pointer map?

regarding the array indexed by SSA versions -- this does not seem like a good choice,
since only a few ssa names are usually inserted to the table, so
allocating and deallocating it would consume too much time.  Pointer
maps were not available when the patch was written; I have rewritten the
patch to use them (although it does not simplify it significantly).

> > + /* Similar to operand_equal_p, but handles the case that X and Y are NULL.  */
> > + 
> > + static bool
> > + operand_eq_p (tree x, tree y)
> > + {
> > +   if (!x)
> > +     return y == NULL_TREE;
> > +   if (!y)
> > +     return false;
> > + 
> > +   return operand_equal_p (x, y, 0);
> > + }
> 
> I would rather handle NULL_TREE in operand_equal_p.  Any reason not to?

operand_equal_p is heavily used function, introducing any new checks to
it could have negative impact on compilation speed (there are several
other places in gcc that test for NULL_TREE before operand_equal_p,
and they were not inlined to operand_equal_p for this reason).

> > +   while (1)
> > +     {
> > +       block_stmt_iterator bsi;
> > +     
> > +       bsi = bsi_for_stmt (stmt);
> > + 
> > +       name = GIMPLE_STMT_OPERAND (stmt, 0);
> > +       gcc_assert (TREE_CODE (name) == SSA_NAME);
> > + 
> > +       next = single_nonlooparound_use (name);
> > + 
> > +       mark_virtual_ops_for_renaming (stmt);
> > +       bsi_remove (&bsi, true);
> > + 
> > +       if (!next
> > + 	  || TREE_CODE (next) != GIMPLE_MODIFY_STMT
> > + 	  || GIMPLE_STMT_OPERAND (next, 1) != name)
> > + 	return;
> > + 
> > +       stmt = next;
> > +     }
> 
> Ouch.  Why not just store BSIs in the chains?

Since bsi_for_stmt will be a constant-time operation once gimple tupples
are implemented, I do not think it is worth complicating the patch with
this.

Zdenek