Re: [patch] More improvements for the code generation of the if-conversion

[Richard Guenther <rguenther at suse dot de>]

On Tue, 22 Jun 2010, Sebastian Pop wrote:

> Hi,
> 
> The attached patches clean the code generated by the if-conversion:
> 
> 0001 adds a debug counter, useful to reduce code generation problems
> due to the tree-if-conversion pass.

Ok.

> 0002 calls the cleanup_tree_cfg after if-conversion to ensure that the
> CFG representation passed to the vectorizer is in good shape.

Instead of

+  if (changed)
+    cleanup_tree_cfg ();

   return 0;
 }

do

   return changed ? TODO_cleanup_cfg : 0;

ok with that change.

> 0003 adds some more sanity checks after the if-conversion pass:
> verify_loop_structure and verify_loop_closed_ssa.

This is already done.  See passes.c.

Not ok.

> 0004 moves the code needed for the computation of the predicate of a
> BB at the beginning of the predicated BB.  The computation of the
> insertion place is simplified, and makes it possible to use this
> predicate in the code of the BB.

Why should it ever be needed to use it in the code of the BB?
As far as I see this only enlarges life-time of them.

> 0005 implements and calls an unfold SSA function that first expands
> the SSA_NAMEs and then calls fold on the expanded expression tree.
> unfold_ssa_names exposes to fold a more complete expression than
> otherwise available in the predicates of a BB.  This patch is now
> needed to improve the quality of the code generated by the
> if-conversion as if-convert gimplifies the predicates in order to
> avoid recomputations of some predicates.  With this patch it is
> possible to fold (a || b) into the true predicate when a and b are the
> predicates of the two branches of a condition, as the SSA_NAMEs a and
> b are first unfolded into their respective expressions and then fold
> finishes by computing the true predicate.  Note that this patch needs
> some adjustment, i.e., replacing the magic constant 5 that is the
> depth level for the unfold traversal to be some parameter, or so.

+      op2 = unfold_ssa_names (gimple_assign_rhs2 (stmt), level);

this possibly accesses uninitialized and unallocated memory.

I do not think this patch is a good idea at all and you allow
exponential growth as you do not decrement level for each operand
you visit.  Thus, for all-ternary operands and your use of a limit
of depth 5 you'd get 243 substitutions and foldings.

At most a substitution depth of 1 would be which in turn will
simplify that patch a lot.

Please re-work it.

> 0006 uses a single function reset_bb_predicate to reset the predicate
> of a BB to true.  This function has to release all the SSA_NAMEs used
> in the gimplification of a predicate.

Ok.

> 0007 avoids the generation of code computing the true predicate, that
> occurs for all the BBs merging disjunct predicates leading to the true
> predicate.

Ok.

> 0008 forces the predicate of a BB to be either is_gimple_condexpr or
> otherwise the predicate is gimplified, avoiding the insertion of the
> same computation on other predicates.

I suppose this is only necessary because of 0005, so defered.

Please post your patches separately and inline, not as attachment.
It's a major nuisance to deal with this kind of mails with its loads
of attachements.

Thanks,
Richard.