Re: Merging Graphite to trunk

[Richard Guenther <rguenther at suse dot de>]

On Thu, 30 Jul 2009, Sebastian Pop wrote:

> Hi,
> 
> This set of patches merge the new Graphite infrastructure to trunk.
> I would need review for the following parts:
> 
> 0003: Document -fgraphite-identity
> 
> 	 * doc/invoke.texi (-fgraphite-identity): Documented.

Please mention that this option is not useful for production
purposes but is a debugging tool.

> 0004: Document -fgraphite-write and -fgraphite-read
> 
> 	 * doc/invoke.texi (-fgraphite-write): Documented.
> 	 (-fgraphite-read): Documented.

Likewise.  What is @var{file}?  The usual dump-base?  Please check
-fdump-tree-* documentation if that uses the same wording or adjust
yours accordingly.

Please mention that this option is not useful for production
purposes.

0003 and 0004 are ok with these changes.


> 0005: Document -fgraphite-force-parallel
> 
> 	 * doc/invoke.texi (-fgraphite-force-parallel): Documented.

Is this special to graphite or does it apply to -ftree-parallelize-loops?

> 0006: Leave the loop_latch basic block empty.
> 
> 	 * cfgloop.h (create_empty_loop_on_edge): Pass an extra argument.
> 	 * cfgloopmanip.c (create_empty_loop_on_edge): Leave the loop_latch
> 	 basic block empty.

Ok.  (do you think it ever made sense to have upper_bound be not
loop invariant?)

> 0007: Move canonicalize_loop_ivs to tree-ssa-loop-manip.c
> 
> This patch has already been submitted by Razya separately.
> This is just a duplicata of the same patch:
> 
> 	* tree-ssa-loop-manip.c: Include langhooks.h.
> 	(rewrite_phi_with_iv): New.
> 	(rewrite_all_phi_nodes_with_iv): New.
> 	(canonicalize_loop_ivs): Move here from tree-parloops.c.
> 	Remove reduction_list argument. Use rewrite_all_phi_nodes_with_iv.
> 	* tree-parloops.c (loop_parallel_p): Move out all conditions
> 	except dependency check.
> 	(canonicalize_loop_ivs): Move to tree-ssa-loop-manip.c.
> 	(gen_parallel_loop): Call canonicalize_loop_ivs without
> 	reduction_list argument.
> 	(build_new_reduction): New.
> 	(gather_scalar_reductions): New.
> 	(try_get_loop_niter): New.
> 	(try_create_reduction_list): New.
> 	(parallleize_loops): Change the parallel conditions check.
> 	* tree-flow.h (canonicalize_loop_ivs): Remove one argument.
> 	* Makefile.in (tree-ssa-loop-manip.o): Add langhooks.h dependency.

I think I have already reviewed this.  Zdenek didn't have any complaints,
so the patch is ok.
 
> 0013: Automatic tester scripts.
> 
> 	 * contrib/ChangeLog.graphite: New.
> 	 * contrib/automatic_tester/git_run.sh: New.
> 	 * contrib/automatic_tester/graphite_env.sh.example: New.
> 	 * contrib/automatic_tester/run_build.sh: New.
> 
> 0015: New implementation of Graphite.
> 
> 	* Makefile.in (OBJS-common): Added dependence on graphite-blocking.o,
> 	graphite-clast-to-gimple.o, graphite-dependences.o,
> 	graphite-interchange.o, graphite-poly.o, graphite-ppl.o,
> 	graphite-scop-detection.o, graphite-sese-to-poly.o, and sese.o.
> 	(graphite-blocking.o,
> 	graphite-clast-to-gimple.o, graphite-dependences.o,
> 	graphite-interchange.o, graphite-poly.o, graphite-ppl.o,
> 	graphite-scop-detection.o, graphite-sese-to-poly.o, and sese.o): New.
> 	* cfgloop.c (alloc_loop): Set loop->can_be_parallel to false.
> 	* cfgloop.h (struct loop): Add can_be_parallel field.
> 	* common.opt (fgraphite-identity): Moved up.
> 	(fgraphite-write): New flag.
> 	(fgraphite-read): New.
> 	(fgraphite-force-parallel): New.
> 	* graphite.c: Rewrite.
> 	* graphite.h: Rewrite.
> 	* output.h (graphite_out_file): Declared.
> 	(graphite_in_file): Declared.
> 	* passes.c (init_optimization_passes): Schedule a pass of DCE and LIM
> 	after Graphite.
> 	* toplev.c (graphite_out_file): New file descriptor.
> 	(graphite_in_file): New.
> 	(init_asm_output): With flag_graphite_read and flag_graphite_write,
> 	initialize graphite_out_file and graphite_in_file.
> 	(process_options): flag_graphite_force_parallel cannot be used without
> 	Graphite.
> 	* tree-ssa-loop.c: Include toplev.h.
> 	(gate_graphite_transforms): Enable flag_graphite for
> 	flag_graphite_force_parallel.

I don't see sese.c in the changelog but it is mentioned in the makefile
changes.

+; This option is not documented as it does not perform any useful 
optimization.
+fgraphite-identity
+Common Report Var(flag_graphite_identity) Optimization
+Enable Graphite Identity transformation
+
+; This option is not documented as it does not perform any useful 
optimization.
+fgraphite-write
+Common Report Var(flag_graphite_write) Optimization
+Write Graphite transformation file
+
+; This option is not documented as it does not perform any useful 
optimization.
+fgraphite-read
+Common Report Var(flag_graphite_read) Optimization
+Read Graphite transformation file

these shouldn't be of Optimization kind.

+  if (flag_graphite_read)
+    {
+      int len = strlen (dump_base_name);
+      char *dumpname = XNEWVEC (char, len + 10);
+
+      gcc_assert (!flag_graphite_write);
...

do we really have to merge this?  Why does it have to be in toplev.c
and using global variables?

The other changes in this patch are ok with the above suggested change.

Thanks,
Richard.

> These patches have been tested all together on the graphite branch:
> they pass both bootstrap and regression tests regularly: see for
> example the last report from the automatic tester:
> http://gcc.gnu.org/ml/gcc-testresults/2009-07/msg03113.html
> 
> The SPEC Cpu2006 benchmarks pass the compile time and runtime tests:
> http://gcc.gnu.org/ml/gcc-testresults/2009-07/msg03117.html
> except for the following miscompiles: 416.gamess, 447.dealII, and
> 483.xalancbmk.  483.xalancbmk is broken in the graphite branch even
> without the graphite pass, maybe also present on trunk, as
> 483.xalancbmk started failing last Friday, after the last merge from
> trunk.  I analyzed the problem in the two other benchmarks and it
> seems to be a same error due to the types of induction variables.
> I am planing to work on fixing this after the merge.
> 
> In the graphite branch the new graphite flags are enabled by default
> in -O2, and this makes the pattern matching testcases of the passes
> occurring after graphite to fail.  I will report the status of the
> bootstrap and the regression tests on trunk with the attached patches.
> 
> Sebastian Pop
> --
> AMD - GNU Tools
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex