On 10/08/2013 05:57 AM, Richard Biener wrote:
On Mon, Oct 7, 2013 at 3:39 PM, Andrew MacLeod <amacl...@redhat.com> wrote:
On 10/07/2013 04:44 AM, Richard Biener wrote:
On Thu, Oct 3, 2013 at 4:11 AM, Andrew MacLeod <amacl...@redhat.com>
wrote:
this patch consolidates tree-ssa-loop*.c files with new .h files as
required
(8 in total)

A number of the prototypes were in tree-flow.h, but there were also a few
in
cfgloop.h.  tree-ssa-loop.h was created to contain a couple of common
structs and act as the gathering file for any generally applicable
tree-ssa-loop includes. tree-flow.h includes this file for now.

There is a bit of a criss-cross mess between the cfg-* and tree-ssa-loop*
routines, but I'm not touching that for now.  Some of that might have to
get
resolved when I try to remove tree-flow.h as a standard include file from
the .c files.. we'll see.

In particular, tree-ssa-loop-niter.h exports a lot of more generally used
routines. loop-iv.c, loop-unroll.c and loop-unswitch.c needed to include
it.

A few routines werent referenced outside their file so I made those
static,
and there was one routine stmt_invariant_in_loop_p wich was actually
unused.

bootstraps on x86_64-unknown-linux-gnu and passes with no new
regressions.
OK?
+   enum tree_code cmp;
+ };
+
+ #include "tree-ssa-loop-im.h"
+ #include "tree-ssa-loop-ivcanon.h"

what's the particular reason to not do includes first?  That looks really
odd (and maybe rather than doing that these includes should be done
elsewhere, like in .c files or in a header that includes tree-ssa-loop.h).

I had sent a followup patch because I didn't like it either :-). required
just a little shuffling since a couple of them used the typedefs declared
before them.

You seem to export things like movement_possibility that are not
used outside of tree-ssa-loop-im.c (in my tree at least).

Hmm, seems I  missed that one somehow... I've been checking every function
and enum, but seemed to have managed to miss that one somehow. easy to fix.

Yes, both those should go into the tree-ssa-loop-im.c


Other than that the single reason we have to have all these exports
is that loop passes have their pass structures and gate / entries
defined in tree-ssa-loop.c instead of in the files the passes reside.
Consider changing that as a preparatory patch - it should cut down
the number of needed new .h files.

Yeah, good observation.   And by shuffling a few other exported routines
which are more generally used into tree-ssa-loop.c:
   for_each_index, lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name,
tree_num_loop_insns and enhancing get_lsm_tmp_name() to accept a suffix
parameter, those loop-im and loop-ivcanon no longer need a .h file either.

I moved the passes out of tree-ssa-loop.c  that would cause new .h files
unnecessarily, but left the others since that is orthogonal and could be
followed up later.

I just bundled it all together since it changes the original 2 patches a
bit.

Bootstraps on x86_64-unknown-linux-gnu, testsuite regressions running.
Assuming they pass fine, OK?
Hm.

Index: loop-iv.c
===================================================================
*** loop-iv.c   (revision 203243)
--- loop-iv.c   (working copy)
*************** along with GCC; see the file COPYING3.
*** 62,67 ****
--- 62,68 ----
   #include "df.h"
   #include "hash-table.h"
   #include "dumpfile.h"
+ #include "tree-ssa-loop-niter.h"

loop-iv.c is RTL land (likewise loop-unroll.c and loop-unswitch.c),
why do they need tree-ssa-loop-niter.h?

Apart from that the patch is ok.

we've got a bit of a mess in a number of places... I've cleaned up a few of the easy ones I found.

This file is required for record_niter_bound(), max_loop_iterations_int() and estimated_loop_iterations_int().. so all bounds estimations. There was enough of an infrastructural requirement for these routines within the file that I decided it was beyond the scope of what I'm doing at the moment to split them out.

Eventually I'd like to break this up into modules and make sure that thing aren't creeping in from the wrong places, and restructure a bit... Loop suffer from that (like here), cfg has a couple of places where either rtl or generic cfg routines care calling into a routine that can understand SSA. I noted one in one of the earlier patches that I'll have to get back to. This sort of thing has in fact prompted me to start looking now at our include web and what should be within the logical modules so we can more easily identify these sorts of things.

Too many things to clean up!! I think I could be doing this sort of thing for the next 8 months easily :-)

I'd prefer to come back and revisit this with a followup to try to split tree-ssa-loop-niter.c into another component, maybe loop-niter.[ch]...

Andrew


Reply via email to