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: [tree-ssa] Supefluous phi removal


In message <20040105124305.GA8321@atrey.karlin.mff.cuni.cz>, Zdenek Dvorak writ
es:
 >Hello,
 >
 >this patch adds cleanup of the redundant phis.  It mostly aims for
 >removing of degenerate phi nodes in basic blocks with just one entry
 >edge, but it of course catches also cases when phi nodes with more
 >arguments become redundant due to removal of definitions.
 >
 >Bootstrapped and regtested on i686.  Compile time impact is not
 >measurable (within bounds of measurement error).
 >
 >Zdenek
 >
 >	* tree-dfa.c (free_df_for_stmt, free_df): New functions.
 >	(compute_immediate_uses_for_stmt): Record uses in VDEFs.
 >	* tree-flow.h (free_df, kill_redundant_phi_nodes): Declare.
 >	* tree-optimize.c (optimize_function_tree): Call
 >	kill_redundant_phi_nodes.
 >	* tree-ssa-ccp.c (finalize): Call free_df.
 >	* tree-ssa.c (replace_immediate_uses, raise_value,
 >	kill_redundant_phi_nodes): New functions.
 >	* testsuite/gcc.dg/tree-ssa/20030709-2.c: Update outcome.
I like it -- it's a lot cleaner than what I had lying around to do something
similar.

On a high level, don't you have to verify that you don't have dependencies
between PHI nodes before copy propagating values like this?   ie, if the
output of a PHI you are removing is used as an input by another PHI in
the same block, then simple copy propagation may not be appropriate
(remember conceptually the RHS of all PHIs are evaluated at the same time,
then assignments take place.  When you turn them into copies (even those
which you propagate away) you effectively serialize the PHI nodes which is
not always correct.

This is the single biggest reason why I haven't ever taken the time to 
polish up what I had -- I hadn't found the time to hook into the dependency
graphs to detect and correctly handle this situation.

Also note that you can (in theory) ignore a PHI alternative which matches
the PHI destination.  I haven't looked closely to see if we're getting
those anymore.

It seems to me the compute_immediate_uses_for_stmt change is an obvious
bugfix -- you can go ahead and install that.

Similarly the code to free the dataflow information along with the
call to free_df from tree-ssa-ccp.c are OK to install immediately as well.

I think you've got a minor formatting goof in replace_immediate_uses.
Specifically the open curly-brace after the ELSE (there is only one
ELSE clause in that function) probably has 8 spaces where instead you
should use a tab.  You might consider making it an external function
with a little sanity checking as well -- I'd expect other hunks of code
could make use of this routine.

For kill_redundant_phi_nodes, it seems to me you ought to document the code
a little.  Specifically I think documenting how EQ_TO is used would be wise,
particularly the case when it indicates that a PHI should not be eliminated
and a copy should not be propagated.  

It also seems to me that this test is wrong:

+ 	      if (TREE_CODE (stmt) != PHI_NODE
+ 		  || SSA_NAME_OCCURS_IN_ABNORMAL_PHI (t))
+ 		{
+ 		  eq_to[aver] = t;
+ 		  raise_value (phi, t, eq_to);
+ 		}

Shouldn't that be ! SSA_NAME_OCCURS_IN_ABNORMAL_PHI (t)) since it would
seem to me that we do _not_ want to record an equivalence for PHI alternative
associated with an abnormal edge.  It also seems to me you want to avoid
this if the destination of the PHI has SSA_NAME_OCCURS_IN_ABNORMAL_PHI set.

Don't install the redundant PHI killing code yet -- I think we need to resolve
question of whether or not we can have dependencies in the PHIs first (and
of course if we can have dependencies, we need to handle them correctly).


Jeff


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