[patch] Cleanup tree-ssa-ter.c exports

[Andrew MacLeod]

On 09/13/2013 11:11 AM, Andrew MacLeod wrote:
> On 09/13/2013 03:54 AM, Richard Biener wrote:
>> On Thu, Sep 12, 2013 at 11:09 PM, Andrew MacLeod 
>> <amacleod@redhat.com> wrote:
>>> There are 2 parts of tre-ssa-ter.c to address.
>>>
>>> is_replaceable_p() is also used in expr.c, It has a flag to indicate 
>>> where
>>> its being called from, and we do different checks for each one.  
>>> There is a
>>> wrapper function stmt_is_replaceable_p() in tree-ssa-ter.c which 
>>> hides the
>>> setting of the flag to expr.c. Most of the function is common, so I
>>> extracted out the common part, and put it in tree-ssa.c.
>>> I then moved stmt_is_replaceable() to expr.c, has it call the common 
>>> routine
>>> and then added the extra bit it needs there.  Similarly 
>>> tree-ssa-ter.c gets
>>> ter_is_replaceable_p() which calls the common part, and then does 
>>> its own
>>> special checking.   This removes that general export and messiness from
>>> tree-ssa-ter.c
>>>
>>> I think I got the logic of the function right, but you might want to 
>>> double
>>> check...  It was giving me a headache when I split it :-)
>>>
>>> Unfortunately,  tree-ssa-ter.c also has 2 functions
>>> (find_replaceable_exprs() and  dump_replaceable_exprs()) which are 
>>> exported
>>> and utilized by tree-outof-ssa.c  (the file is a part of the out-of-ssa
>>> module).  So I moved the prototypes from tree-ssa-live.h into a newly
>>> created tree-ssa-ter.h file,  and included it directly from
>>> tree-outof-ssa.c, the only consumer.
>>>
>>> I also could have just left the is_replaceable_p() function as is, 
>>> put the
>>> prototype and the 'stmt_is_replaceable_p()' wrapper function in
>>> tree-ssa-ter.h, and then included that from expr.c...  but it just 
>>> seems
>>> like an odd thing to include directly there....  but that is an 
>>> option...
>>>
>>> eventually we mighty want to look at splitting expr.c.. it seems a bit
>>> multi-personality with some pure RTL, some tree/rtl and some 
>>> ssa...   even
>>> though they all serve the same ultimate function, it is 11,000+ 
>>> lines long
>>> now :-) .  A task for another day.
>> is_replaceable_p and friends is purely specific to the area of RTL 
>> expansion,
>> so putting it in tree-ssa.[ch] is definitely wrong.  It doesn't make 
>> sense to
>> use it anywhere else.
>>
>> The main driver of the RTL expansion process is in cfgexpand.c (the 
>> expand
>> pass itself) in gimple_expand_cfg.  We have one header file related to
>> the RTL expansion process and is_replaceable_p would simply fit in
>> ssaexpand.h (which doesn't have a .c file, so leaving the stuff in 
>> the .c files
>> where they are now is ok).
>>
>> So - I fear you have to re-do this patch (in a much simpler way).
>>
> I actually figured as much :-)  I had actually done that split before 
> I remembered there were other exports, and considered undoing that 
> part, but figured since I had done it I'd get opinion :-)
>
> Or are you suggesting we also bail on tree-ssa-ter.h and put those 
> prototypes in ssaexpand.h?
>
> If we want to make this fully consistent (.h's match .c exports) , I 
> could put is_replaceable_p() into a new ssaexpand.c, move the 3 
> tree-outof-ssa prototypes in ssaexpand.h into tree-outof-ssa.h, and 
> have ssaexpand.h include tree-ssa-ter.h and tree-outof-ssa.h.   That 
> would then be clean. and consistent.
>
> actually, we could just make is_replaceable_p() static inline in 
> ssaexpand.h...  thats not unreasonable either, but we'll probably find 
> functions which belong in ssaexpand.c sooner or later.
>
> No worries, I figured these first few patches would be slower and more 
> painful until a reasonable formula was determined :-)
>
>
> Andrew
>
OK, a slightly different take..
I realized that I should be adding tree-outof-ssa.h to handle the 3 
exports from tree-outof-ssa.c that are in ssaexpand.h...  In fact, by 
far the most sensible thing to do is to simply rename tree-outof-ssa.c 
to ssaexpand.c.  This actually resolves a number of warts... And 
is_replaceable_p() very naturally fits in ssaexpand.c now...

what do you think of this option? :-)  and svn rename preserves all the 
tree-outof-ssa.c history...

Andrew




-------------- next part --------------
A non-text attachment was scrubbed...
Name: f1.diff
Type: text/x-patch
Size: 46326 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130913/f336d1a0/attachment.bin>