Re: [patch] Separate immediate uses and phi routines from tree-flow*.h

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

On Tue, Sep 24, 2013 at 4:39 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> This larger patch moves all the immediate use and operand routines from
> tree-flow.h into tree-ssa-operands.h.
> It also moves the basic phi routines and prototypes into a newly created
> tree-phinodes.h, or tree-ssa-operands.h if they belong there.
> And finally shuffles a couple of other routines which allows
> tree-ssa-operands.h to be removed from the gimple.h header file.
>
> of note or interest:
>
> 1 - dump_decl_set() was defined in tree-into-ssa.c, but isn't really ssa
> specific. Its tree-specific, so normally I'd throw it into tree.c.  Looking
> forward a little, its only used in a gimple context, so when we map to
> gimple_types it will need to be converted to/created for those. If it is in
> tree.c, I'll have to create a new version for gimple types, and then the
> routine in tree.c will become unused.  Based on that, I figured gimple.c is
> the place place for it.
>
> 2 - has_zero_uses_1() and single_imm_use_1() were both in tree-cfg.c for
> some reason.. they've been moved to tree-ssa-operands.c
>
> 3 - a few routines seem like basic gimple routines, but really turn out to
> require the operand infrastructure to implement... so they are moved to
> tree-ssa-operands.[ch] as well.  This sort of thing showed up when removing
> tree-ssa-operands.h from the gimple.h include file.  These were things like
> gimple_vuse_op, gimple_vdef_op, update_stmt, and update_stmt_if_modified

Note that things like gimple_vuse_op are on the interface border between
gimple (where the SSA operands are stored) and SSA operands.  So
it's not so clear for them given they access internal gimple fields directly
but use the regular SSA operand API.

I'd prefer gimple_vuse_op and gimple_vdef_op to stay in gimple.[ch].

If you want to remove the tree-ssa-operands.h include from gimple.h
(in some way makes sense), then we need a new gimple-ssa.[ch]
for routines that operate on the "gimple in SSA" IL (not purely on
gimple, which could be not in SSA form and not purely on the SSA
web which could in theory be constructed over a non-GIMPLE representation).

Actually I like the idea of gimple-ssa.[ch].  tree-ssa-operands.[ch]
would then be solely the place for the operand infrastructure internals
implementation, not a place for generic SSA utility routines related
to SSA operands.

Moving update_stmt/update_stmt_if_modified is a similar case but
maybe less obvious (unless we have gimple-ssa.[ch]).

What do you think?

> 4 - gimple_phi_arg_def()  was oddly defined:
>
> static inline tree
> gimple_phi_arg_def (gimple gs, size_t index)
> {
>   struct phi_arg_d *pd = gimple_phi_arg (gs, index);
>   return get_use_from_ptr (&pd->imm_use);
> }
>
> /* Return a pointer to the tree operand for argument I of PHI node GS.  */
>
> static inline tree *
> gimple_phi_arg_def_ptr (gimple gs, size_t index)
> {
>   return &gimple_phi_arg (gs, index)->def;
> }
>
>
> Im not sure why it goes through the immediate use routines... it should be
> the same as '* gimple_phi_arg_def_ptr(x,y)' ...   And by using immediate use
> routine, it couldn't be put in tree-phinodes like it belong.   I changed it
> to be simply
>    return gimple_phi_arg (gs, index)->def;
> and bootstrapped that change, along with an assert that it was the same
> value as the immediate_use method.   everything worked as expected. old wart
> I guess.

Nice cleanup.

> I didn't bother with a Makefile.in change since its only dependencies
> change, and Tromey's pending auto-dependency patch would just delete those
> anyway.
>
> This bootstraps on  x86_64-unknown-linux-gnu and has no new regressions.
> OK?

(swap_tree_operands): Move prototype to tree-ssa-operands.h.

rename to swap_ssa_operands and remove the !ssa_operands_active
path?  (should be no longer needed I think)

        (gimple_phi_arg_def, gimple_phi_arg_def_ptr, gimple_phi_arg_edge,
        gimple_phi_arg_location, gimple_phi_arg_location_from_edge,
        gimple_phi_arg_set_location, gimple_phi_arg_has_location, phi_nodes,
        phi_nodes_ptr, set_phi_nodes, phi_ssa_name_p): Move to tree-phinodes.h.

I always found the separation of PHI node API from gimple API odd ...
now, PHI nodes are a SSA speciality.  Unless the gimple_phi class
moves to tree-phinodes.h the same comments as to gimple_vuse_op
apply.  But maybe everything will be more obvious once we separate
the core gimple data structure from the gimple API stuff ...

Overall the patch looks ok, either massage it according to my comments
above (thinking about gimple-ssa.[ch]) or follow up with a separate patch
(not that moving things twice will hurt).

Thanks,
Richard.

> Andrew