Re: [patch] Add tree-ssa-loop.h and friends.

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Oct 7, 2013 at 3:39 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> On 10/07/2013 04:44 AM, Richard Biener wrote:
>>
>> On Thu, Oct 3, 2013 at 4:11 AM, Andrew MacLeod <amacleod@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.

Thanks,
Richard.


> Andrew