On Fri, Sep 13, 2013 at 9:15 PM, Andrew MacLeod <amacl...@redhat.com> wrote:
> 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 <amacl...@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...
I don't like the new name for tree-outof-ssa.c, it matches less to its contents.
I'd say either keep ssaexpand.h and tree-outof-ssa.c as-is or rename
ssaexpand.h to tree-outof-ssa.h. I prefer the latter.
The rest of the changes look ok to me, but watch out for odd whitespace
changes:
+static inline bool
+ter_is_replaceable_p (gimple stmt)
+{
+
+ if (ssa_is_replaceable_p (stmt))
spurious vertical space.
Thanks,
Richard.
> Andrew
>
>
>
>
