Re: [PATCH 1/4] Add flag -ftree-loop-if-convert-memory-writes.

[Richard Guenther <rguenther@suse.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2904 bytes --]

On Fri, 13 Aug 2010, Sebastian Pop wrote:

> On Fri, Aug 13, 2010 at 03:54, Richard Guenther <rguenther@suse.de> wrote:
> >> +static void
> >> +predicate_mem_writes (loop_p loop)
> >> +{
> >> +  unsigned int i, orig_loop_num_nodes = loop->num_nodes;
> >> +
> >> +  for (i = 1; i < orig_loop_num_nodes; i++)
> >> +    {
> >> +      gimple_stmt_iterator gsi;
> >> +      basic_block bb = ifc_bbs[i];
> >> +      tree cond = bb_predicate (bb);
> >> +
> >> +      if (is_true_predicate (cond))
> >> +     continue;
> >> +
> >> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >
> > So this is another loop over all loops and statements.  Why not
> > do this loop once and dispatch to mem/scalar if-conversion there?
> >
> >> @@ -1178,7 +1270,10 @@ combine_blocks (struct loop *loop)
> >>
> >>    remove_conditions_and_labels (loop);
> >>    insert_gimplified_predicates (loop);
> >> -  ifconvert_phi_nodes (loop);
> >> +  predicate_all_scalar_phis (loop);
> >> +
> >> +  if (flag_tree_loop_if_convert_memory_writes)
> >> +    predicate_mem_writes (loop);
> >
> > As suggested above, predicate_all_scalar_phis and predicate_mem_writes
> > should loop over all loops/bbs once.
> >
> 
> The only thing that predicate_all_scalar_phis and predicate_mem_writes
> have in common is that they iterate over all the basic blocks of an
> innermost loop.
> 
> predicate_mem_writes iterates over all the statements of the BB.
> 
> predicate_all_scalar_phis iterates over all the phi nodes of the BB.
> 
> Should I go ahead and merge these two functions as asked?
> I still find the implementation more clear in the separated form.

Yeah, thinking again it's ok as-is.

> Please find attached a preliminary patch (passed compile and
> make -k check RUNTESTFLAGS=tree-ssa.exp and vect.exp) on top of
> the previous one, that shows the corrections for all your comments
> (except this last point).  I will submit the combined patch +
> corrections once it passes bootstrap and test.

-                 || gimple_code (gsi_stmt (gsi)) == GIMPLE_COND)
+                 || gimple_code (gsi_stmt (gsi)) == GIMPLE_COND
+                 || stmt_ends_bb_p (gsi_stmt (gsi)))

GIMPLE_CONDs already are handled in stmt_ends_bb_p, so the check
for GIMPLE_COND can be removed.

@@ -1628,7 +1716,7 @@ main_tree_if_conversion (void)
     changed |= tree_if_conversion (loop);

   if (changed && flag_tree_loop_if_convert_memory_writes)
-    update_ssa (TODO_update_ssa);
+    return TODO_update_ssa_only_virtuals;

   return changed ? TODO_cleanup_cfg : 0;

So you skip cfg_cleanup in that case.  I'd have done

  unsigned todo = 0;

...

   if (changed)
     todo |= TODO_cleanup_cfg;
   if (changed && flag_tree_loop_if_convert_memory_writes)
     todo |= TODO_update_ssa_only_virtuals;

   return todo;

I'll wait for a new combined patch and have a quick look at it.

Richard.