This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/4] Add flag -ftree-loop-if-convert-memory-writes.
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.