| Message ID | 1398099480-49147-4-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/21/14 10:56, David Malcolm wrote: > This updates all of the gimple_bind_* accessors in gimple.h from taking a > plain gimple to taking a gimple_bind (or const_gimple_bind), with the > checking happening at the point of cast. > > Various other types are strengthened from gimple to gimple_bind, and from > plain vec<gimple> to vec<gimple_bind>. > > gcc/ > * coretypes.h (gimple_bind): New typedef. > (const_gimple_bind): New typedef. > > * gdbhooks.py (build_pretty_printer): Add gimple_bind > and its variants, using the gimple printer. > > * gimple-pretty-print.c (dump_gimple_bind): Update type-signature to > require a gimple_bind rather than just a gimple. > > * gimple.c (gimple_build_bind): Return a gimple_bind rather than > just a gimple. > * gimple.h (gimple_build_bind): Likewise. > > * gimple.h (gimple_statement_base::as_a_gimple_bind): New. > (gimple_statement_base::dyn_cast_gimple_bind): New. > (gimple_seq_first_stmt_as_a_bind): New. > > * gimple.h (gimple_bind_vars): Update type-signature to > require a gimple_bind rather than just a gimple, removing > as_a and hence run-time check. > (gimple_bind_set_vars): Likewise. > (gimple_bind_append_vars): Likewise. > (gimple_bind_body_ptr): Likewise. > (gimple_bind_body): Likewise. > (gimple_bind_set_body): Likewise. > (gimple_bind_add_stmt): Likewise. > (gimple_bind_add_seq): Likewise. > (gimple_bind_block): Likewise. > (gimple_bind_set_block): Likewise. > * gimplify.c (gimple_push_bind_expr): Likewise. > (gimple_current_bind_expr): Likewise. > * tree-inline.c (copy_gimple_bind): Likewise. > > * gimplify.h (gimple_current_bind_expr): Return a gimple_bind > rather than a plain gimple. > (gimplify_body): Likewise. > (gimple_bind_expr_stack): Return a vec<gimple_bind> rather than > a vec<gimple>. > > * gimplify.c (struct gimplify_ctx): Strengthen field > "bind_expr_stack" from vec<gimple> to vec<gimple_bind>. > (gimple_bind_expr_stack): Likewise for type of returned value. > > * gimplify.c (gimplify_body): Strengthen various types from gimple > to gimple_bind, including the return type. > > * gimplify.c (declare_vars): Introduce "gs" as a generic gimple, > so that local "scope" can be of type gimple_bind once we've reached > the region where it must be of code GIMPLE_BIND. > > * gimple-low.c (lower_gimple_bind): Add checked cast to > gimple_bind, since both callers (lower_function_body and > lower_stmt) have checked the code for us. > > * gimple.c (gimple_copy): Add checked cast to gimple_bind in > region guarded by check for code GIMPLE_BIND. > * gimple-low.c (gimple_stmt_may_fallthru): Likewise. > * gimple-pretty-print.c (pp_gimple_stmt_1): Likewise. > * gimple-walk.c (walk_gimple_stmt): Likewise. > * omp-low.c (scan_omp_1_stmt): Likewise. > (lower_omp_1): Likewise. > (lower_omp_for): Likewise. > * tree-cfg.c (verify_gimple_in_seq_2): Likewise. > (do_warn_unused_result): Likewise. > * tree-inline.c (remap_gimple_stmt): Likewise. > (estimate_num_insns): Likewise. > * tree-nested.c (convert_nonlocal_reference_stmt): Likewise. > > * gimplify.c (gimplify_bind_expr): Update local(s) to be a > gimple_bind rather than just a gimple. > (gimplify_function_tree): Likewise. > * omp-low.c (lower_omp_sections): Likewise. > (lower_omp_single): Likewise. > (lower_omp_master): Likewise. > (lower_omp_taskgroup): Likewise. > (lower_omp_ordered): Likewise. > (lower_omp_critical): Likewise. > (lower_omp_taskreg): Likewise. > (lower_omp_teams): Likewise. > * omp-low.c (lower_omp_for): Likewise; use > gimple_seq_first_stmt_as_a_bind to encapsulate the checked cast. > (lower_omp_target): Likewise. > * tree-nested.c (finalize_nesting_tree_1): Likewise. > > * gimple.c (empty_stmt_p): Add dyn_cast to a gimple_bind. > * tree-inline.c (replace_locals_stmt): Add dyn_cast to gimple_bind. > > gcc/c-family/ > * c-gimplify.c (add_block_to_enclosing): Strengthen local "stack" > from being just a vec<gimple> to a vec<gimple_bind>. > > gcc/java/ > * java-gimplify.c (java_gimplify_block): Update local to be a > gimple_bind rather than just a gimple. This is fine, with the same requested changes as #2; specifically using an explicit cast rather than hiding the conversion in a method. Once those changes are in place, it's good for 4.9.1. Thanks, Jeff
On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: > On 04/21/14 10:56, David Malcolm wrote: > > This updates all of the gimple_bind_* accessors in gimple.h from taking a > > plain gimple to taking a gimple_bind (or const_gimple_bind), with the > > checking happening at the point of cast. > > > > Various other types are strengthened from gimple to gimple_bind, and from > > plain vec<gimple> to vec<gimple_bind>. [...] > This is fine, with the same requested changes as #2; specifically using > an explicit cast rather than hiding the conversion in a method. Once > those changes are in place, it's good for 4.9.1. Thanks - presumably you mean "good for *trunk* after 4.9.1 is released" right?
On 04/23/14 15:13, David Malcolm wrote: > On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >> On 04/21/14 10:56, David Malcolm wrote: >>> This updates all of the gimple_bind_* accessors in gimple.h from taking a >>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the >>> checking happening at the point of cast. >>> >>> Various other types are strengthened from gimple to gimple_bind, and from >>> plain vec<gimple> to vec<gimple_bind>. > > [...] > >> This is fine, with the same requested changes as #2; specifically using >> an explicit cast rather than hiding the conversion in a method. Once >> those changes are in place, it's good for 4.9.1. > Thanks - presumably you mean > "good for *trunk* after 4.9.1 is released" Right. Sorry for the confusion. jeff
On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: > On 04/23/14 15:13, David Malcolm wrote: >> >> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >>> >>> On 04/21/14 10:56, David Malcolm wrote: >>>> >>>> This updates all of the gimple_bind_* accessors in gimple.h from taking >>>> a >>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the >>>> checking happening at the point of cast. >>>> >>>> Various other types are strengthened from gimple to gimple_bind, and >>>> from >>>> plain vec<gimple> to vec<gimple_bind>. >> >> >> [...] >> >>> This is fine, with the same requested changes as #2; specifically using >>> an explicit cast rather than hiding the conversion in a method. Once >>> those changes are in place, it's good for 4.9.1. >> >> Thanks - presumably you mean >> "good for *trunk* after 4.9.1 is released" > > Right. Sorry for the confusion. Note I still want that less-typedefs (esp. the const_*) variants to be explored. Changing this will touch all the code again, so I'd like to avoid that. That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' and not 'gimple_statement_base *'? The main reason that we have so many typedefs is that in C you had to use 'struct foo' each time you refer to foo as a type - I suppose it was then convenient to do the typedef to the pointer type. With 'gimple' being not a pointer type we get const correctness in the way people would expect it to work. [no, I don't suggest you change 'tree' or 'const_tree' at this point, just gimple (and maybe gimple_seq) as you are working on the 'gimple' type anyway]. So I'd rather not say "ok for trunk after 4.9.1" for the current form with the cast changes. Not yet. Thanks, Richard. > jeff
On 04/24/2014 04:33 AM, Richard Biener wrote: > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: >> On 04/23/14 15:13, David Malcolm wrote: >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >>>> On 04/21/14 10:56, David Malcolm wrote: >>>>> This updates all of the gimple_bind_* accessors in gimple.h from taking >>>>> a >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the >>>>> checking happening at the point of cast. >>>>> >>>>> Various other types are strengthened from gimple to gimple_bind, and >>>>> from >>>>> plain vec<gimple> to vec<gimple_bind>. >>> >>> [...] >>> >>>> This is fine, with the same requested changes as #2; specifically using >>>> an explicit cast rather than hiding the conversion in a method. Once >>>> those changes are in place, it's good for 4.9.1. >>> Thanks - presumably you mean >>> "good for *trunk* after 4.9.1 is released" >> Right. Sorry for the confusion. > Note I still want that less-typedefs (esp. the const_*) variants to be explored. > Changing this will touch all the code again, so I'd like to avoid that. > > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' > and not 'gimple_statement_base *'? The main reason that we have so > many typedefs is that in C you had to use 'struct foo' each time you > refer to foo as a type - I suppose it was then convenient to do the > typedef to the pointer type. With 'gimple' being not a pointer type > we get const correctness in the way people would expect it to work. > [no, I don't suggest you change 'tree' or 'const_tree' at this point, just > gimple (and maybe gimple_seq) as you are working on the 'gimple' > type anyway]. > > So if we change 'gimple' everywhere to be 'gimple *', can we just abandon the 'gimple' typedef completely and go directly to using something like gimple_stmt, or some other agreeable name instead? I think its more descriptive and then frees up the generic 'gimple' name should we decide to do something more with namespaces in the future... Andrew
On Thu, 2014-04-24 at 09:09 -0400, Andrew MacLeod wrote: > On 04/24/2014 04:33 AM, Richard Biener wrote: > > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: > >> On 04/23/14 15:13, David Malcolm wrote: > >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: > >>>> On 04/21/14 10:56, David Malcolm wrote: > >>>>> This updates all of the gimple_bind_* accessors in gimple.h from taking > >>>>> a > >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the > >>>>> checking happening at the point of cast. > >>>>> > >>>>> Various other types are strengthened from gimple to gimple_bind, and > >>>>> from > >>>>> plain vec<gimple> to vec<gimple_bind>. > >>> > >>> [...] > >>> > >>>> This is fine, with the same requested changes as #2; specifically using > >>>> an explicit cast rather than hiding the conversion in a method. Once > >>>> those changes are in place, it's good for 4.9.1. > >>> Thanks - presumably you mean > >>> "good for *trunk* after 4.9.1 is released" > >> Right. Sorry for the confusion. > > Note I still want that less-typedefs (esp. the const_*) variants to be explored. > > Changing this will touch all the code again, so I'd like to avoid that. > > > > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' > > and not 'gimple_statement_base *'? The main reason that we have so > > many typedefs is that in C you had to use 'struct foo' each time you > > refer to foo as a type - I suppose it was then convenient to do the > > typedef to the pointer type. With 'gimple' being not a pointer type > > we get const correctness in the way people would expect it to work. > > [no, I don't suggest you change 'tree' or 'const_tree' at this point, just > > gimple (and maybe gimple_seq) as you are working on the 'gimple' > > type anyway]. > > > > > > So if we change 'gimple' everywhere to be 'gimple *', can we just > abandon the 'gimple' typedef completely and go directly to using > something like gimple_stmt, or some other agreeable name instead? > > I think its more descriptive and then frees up the generic 'gimple' name > should we decide to do something more with namespaces in the future... There have been a few different proposals as to what the resulting gimple API might look like, in various subthreads of this discusssion, so I thought it might help the discussion to gather up the proposals, and to apply them to some specific code examples, to see what the results might look like. So here are a couple of code fragments, from gcc/graphite-sese-to-poly.c and gcc/tree-ssa-uninit.c respectively: Status quo ========== static gimple detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, vec<gimple> *out) { if (scalar_close_phi_node_p (stmt)) { gimple def, loop_phi, phi, close_phi = stmt; tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; gimple_stmt_iterator gsi; vec<gimple> worklist = vNULL; pointer_set_t *added_to_worklist; The currently-posted patch series ================================= Here's the cumulative effect of the patch series I posted, using the casting methods of the base class (the "stmt->as_a_gimple_phi" call): -static gimple +static gimple_phi detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, vec<gimple> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; + gimple def; + gimple_phi loop_phi, phi, close_phi = stmt->as_a_gimple_phi (); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + gimple_phi_iterator gsi; + vec<gimple_phi> worklist = vNULL; pointer_set_t *added_to_worklist; Direct use of is-a.h, retaining typedefs of pointers ==================================================== The following patch shows what the above might look like using the patch series as posted, but eliminating the casting methods in favor of direct use of the is-a.h API (but still using lots of typedefs of pointers): -static gimple +static gimple_phi detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, vec<gimple> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + gimple def; + gimple_phi loop_phi, phi, close_phi = as_a <gimple_phi> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + gimple_phi_iterator gsi; + vec<gimple_phi> worklist = vNULL; pointer_set_t *added_to_worklist; I posted an example of porting a patch in the series to this approach as: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html Explicit pointers, rather than typedefs ======================================= Richi suggested making pointers be explicit rather than hidden in the typedefs in: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html which might give something like this: -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static gimple_phi * +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, + vec<gimple *> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + gimple *def; + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + gimple_phi_iterator gsi; + vec<gimple_phi *> worklist = vNULL; pointer_set_t *added_to_worklist; Changing the meaning of "gimple" makes this a much bigger patch IMHO than what I've currently posted. One way to achieve this could be a mega-patch (ugh) which ports the whole middle-end at once to change the "pointerness" of "gimple", probably auto-generated and, once that's in place, then look at introducing subclass usage. Note: it's more fiddly than a simply sed of "gimple" to "gimple *" (or whatever); consider the gimple_phi decls above, which would change thusly: - gimple def, loop_phi, phi, close_phi = stmt; + gimple *def, *loop_phi, *phi, *close_phi = stmt; Implicit naming =============== Several people have suggested that the "gimple_" prefix is redundant. Andrew MacLeod suggested in: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html that we could simple drop the "gimple_" prefix. Combining this with the pointer approach, for example, gives: -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static phi * +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, + vec<gimple *> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + gimple *def; + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + phi_iterator gsi; + vec<phi *> worklist = vNULL; pointer_set_t *added_to_worklist; though it could also be done with typedefs of pointers, rather than the "explicit pointers" above. Namespaces (explicit) ===================== Continuing with the idea that the "gimple_" prefix is redundant, Andrew MacLeod also suggested in: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html that we could repurpose "gimple" to be a namespace. Here's what things might look like written out in full (perhaps using gimple::stmt to be the base class): -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static gimple::phi * +detect_commutative_reduction (scop_p scop, gimple::stmt *stmt, + vec<gimple::stmt *> *in, + vec<gimple::stmt *> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + gimple::stmt *def; + gimple::phi loop_phi, phi, close_phi = as_a <gimple::phi *> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + gimple::phi_iterator gsi; + vec<gimple::phi *> worklist = vNULL; pointer_set_t *added_to_worklist; This may require some gengtype support, for the case of fields within structs. Andrew suggested renaming "gimple" to "gimple_stmt" in: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html as a possible migration path towards this. Namespaces (implicit) ===================== The above is, of course, verbose - I'm mostly posting it to clarify the following, which uses a "using" decl to eliminate all of the "gimple::" from the above: using gimple; -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static phi * +detect_commutative_reduction (scop_p scop, stmt *stmt, + vec<stmt *> *in, + vec<stmt *> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + stmt *def; + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + phi_iterator gsi; + vec<phi *> worklist = vNULL; pointer_set_t *added_to_worklist; This would require some gengtype support (again, for the case of fields within structs). C++ references (without namespaces) =================================== Richi suggested the use of references rather than pointers when the address is required to be non-NULL: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html though there's been some pushback on C++ references in the past e.g. from Jeff: http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01430.html Here's what the "Explicit pointers, rather than typedefs" might look like, but with references rather than ptrs for some vars where NULL isn't valid: -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static gimple_phi * +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple &> *in, + vec<gimple &> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + gimple *def; + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + gimple_phi_iterator gsi; + vec<gimple_phi &> worklist = vNULL; pointer_set_t *added_to_worklist; ...though arguably there's plenty more conversion of the above that could be done: "def" is initialized once, with a non-NULL ptr, so arguably could be reference, but that would require moving the decl down to the point of initialization so I didn't touch it above. I think use of references would tend to break up such declarations of locals. C++ references (with implicit namespaces) ========================================= ...and here's what the above "namespaces with a using decl" approach might look like with references: using gimple; -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static phi * +detect_commutative_reduction (scop_p scop, stmt &stmt, + vec<stmt &> &in, + vec<stmt &> &out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); - tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); + stmt *def; + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); + tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + phi_iterator gsi; + vec<phi *> worklist = vNULL; pointer_set_t *added_to_worklist; So the above hopefully gives an idea of what a more compile-time type-safe gimple API might look like; sorry if I've mischaracterized any of the ideas. I believe they're roughly sorted by increasing invasiveness, and by increasing "C++ ness" (both of which give me pause). Thoughts? FWIW, my own preference is for "Direct use of is-a.h, retaining typedefs of pointers" but that may be because it would be less work for me ;) Andrew: I know you've been working on improving the typesafety of expressions vs types in the middle-end. I'm curious as to what the above code fragments look like with just your changes? Hope this is useful. Dave
Hi, On Thu, 24 Apr 2014, David Malcolm wrote: > Implicit naming > =============== > Several people have suggested that the "gimple_" prefix is redundant. Not generally though (for instance I find it redundant in the cast-method names, but _not_ in the global types). > Andrew MacLeod suggested in: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > that we could simple drop the "gimple_" prefix. Combining this with the > pointer approach, for example, gives: > > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple *def; > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); That is, I'm not fond of a global type named just "phi" or "bind" or "assign". There the gimple_ prefix is sort of fine, we could perhaps trade it for a _t suffix though (phi_t, assign_t? hmm don't know, still feels too broad). But for method names (that necessarily don't conflict with method names from other classes or even just with local variable names) such prefixes are useless. For similar reasons I find the "as_a_" prefix too verbose in method names, "as_" is enough to convey the meaning. OTOH the awkward _a suffix is necessary for the globally named templates to not clash with single word names of local variable like "as" or "is". That is, methods simply can be named much more sensible and still be shorter than global entities as they always carry class context with them. I'm sorta fond of methods :) Ciao, Michael.
On Thu, Apr 24, 2014 at 4:59 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Thu, 2014-04-24 at 09:09 -0400, Andrew MacLeod wrote: >> On 04/24/2014 04:33 AM, Richard Biener wrote: >> > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: >> >> On 04/23/14 15:13, David Malcolm wrote: >> >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >> >>>> On 04/21/14 10:56, David Malcolm wrote: >> >>>>> This updates all of the gimple_bind_* accessors in gimple.h from taking >> >>>>> a >> >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the >> >>>>> checking happening at the point of cast. >> >>>>> >> >>>>> Various other types are strengthened from gimple to gimple_bind, and >> >>>>> from >> >>>>> plain vec<gimple> to vec<gimple_bind>. >> >>> >> >>> [...] >> >>> >> >>>> This is fine, with the same requested changes as #2; specifically using >> >>>> an explicit cast rather than hiding the conversion in a method. Once >> >>>> those changes are in place, it's good for 4.9.1. >> >>> Thanks - presumably you mean >> >>> "good for *trunk* after 4.9.1 is released" >> >> Right. Sorry for the confusion. >> > Note I still want that less-typedefs (esp. the const_*) variants to be explored. >> > Changing this will touch all the code again, so I'd like to avoid that. >> > >> > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' >> > and not 'gimple_statement_base *'? The main reason that we have so >> > many typedefs is that in C you had to use 'struct foo' each time you >> > refer to foo as a type - I suppose it was then convenient to do the >> > typedef to the pointer type. With 'gimple' being not a pointer type >> > we get const correctness in the way people would expect it to work. >> > [no, I don't suggest you change 'tree' or 'const_tree' at this point, just >> > gimple (and maybe gimple_seq) as you are working on the 'gimple' >> > type anyway]. >> > >> > >> >> So if we change 'gimple' everywhere to be 'gimple *', can we just >> abandon the 'gimple' typedef completely and go directly to using >> something like gimple_stmt, or some other agreeable name instead? >> >> I think its more descriptive and then frees up the generic 'gimple' name >> should we decide to do something more with namespaces in the future... > > There have been a few different proposals as to what the resulting > gimple API might look like, in various subthreads of this discusssion, > so I thought it might help the discussion to gather up the proposals, > and to apply them to some specific code examples, to see what the > results might look like. > > So here are a couple of code fragments, from gcc/graphite-sese-to-poly.c > and gcc/tree-ssa-uninit.c respectively: > > Status quo > ========== > > static gimple > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > vec<gimple> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > gimple def, loop_phi, phi, close_phi = stmt; > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > gimple_stmt_iterator gsi; > vec<gimple> worklist = vNULL; > pointer_set_t *added_to_worklist; > > The currently-posted patch series > ================================= > Here's the cumulative effect of the patch series I posted, using the > casting methods of the base class (the "stmt->as_a_gimple_phi" call): > > -static gimple > +static gimple_phi > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > vec<gimple> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > + gimple def; > + gimple_phi loop_phi, phi, close_phi = stmt->as_a_gimple_phi (); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + gimple_phi_iterator gsi; > + vec<gimple_phi> worklist = vNULL; > pointer_set_t *added_to_worklist; > > Direct use of is-a.h, retaining typedefs of pointers > ==================================================== > The following patch shows what the above might look like using the patch > series as posted, but eliminating the casting methods in favor of > direct use of the is-a.h API (but still using lots of typedefs of > pointers): > > -static gimple > +static gimple_phi > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > vec<gimple> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple def; > + gimple_phi loop_phi, phi, close_phi = as_a <gimple_phi> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + gimple_phi_iterator gsi; > + vec<gimple_phi> worklist = vNULL; > pointer_set_t *added_to_worklist; > > I posted an example of porting a patch in the series to this approach as: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html > > Explicit pointers, rather than typedefs > ======================================= > Richi suggested making pointers be explicit rather than hidden in the > typedefs in: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html > which might give something like this: > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static gimple_phi * > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, > + vec<gimple *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple *def; > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + gimple_phi_iterator gsi; > + vec<gimple_phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > Changing the meaning of "gimple" makes this a much bigger patch IMHO > than what I've currently posted. One way to achieve this could be a > mega-patch (ugh) which ports the whole middle-end at once to change the > "pointerness" of "gimple", probably auto-generated and, once that's in > place, then look at introducing subclass usage. > > Note: it's more fiddly than a simply sed of "gimple" to "gimple *" (or > whatever); consider the gimple_phi decls above, which would change > thusly: > - gimple def, loop_phi, phi, close_phi = stmt; > + gimple *def, *loop_phi, *phi, *close_phi = stmt; > > Implicit naming > =============== > Several people have suggested that the "gimple_" prefix is redundant. > > Andrew MacLeod suggested in: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > that we could simple drop the "gimple_" prefix. Combining this with the > pointer approach, for example, gives: > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static phi * > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, > + vec<gimple *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple *def; > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + phi_iterator gsi; > + vec<phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > though it could also be done with typedefs of pointers, rather than the > "explicit pointers" above. > > > Namespaces (explicit) > ===================== > Continuing with the idea that the "gimple_" prefix is redundant, Andrew > MacLeod also suggested in: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > that we could repurpose "gimple" to be a namespace. Here's what things > might look like written out in full (perhaps using gimple::stmt to be > the base class): > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static gimple::phi * > +detect_commutative_reduction (scop_p scop, gimple::stmt *stmt, > + vec<gimple::stmt *> *in, > + vec<gimple::stmt *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple::stmt *def; > + gimple::phi loop_phi, phi, close_phi = as_a <gimple::phi *> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + gimple::phi_iterator gsi; > + vec<gimple::phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > This may require some gengtype support, for the case of fields within > structs. > > Andrew suggested renaming "gimple" to "gimple_stmt" in: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > as a possible migration path towards this. > > Namespaces (implicit) > ===================== > The above is, of course, verbose - I'm mostly posting it to clarify the > following, which uses a "using" decl to eliminate all of the "gimple::" > from the above: > > using gimple; > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static phi * > +detect_commutative_reduction (scop_p scop, stmt *stmt, > + vec<stmt *> *in, > + vec<stmt *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + stmt *def; > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + phi_iterator gsi; > + vec<phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > This would require some gengtype support (again, for the case of fields > within structs). > > C++ references (without namespaces) > =================================== > Richi suggested the use of references rather than pointers when the > address is required to be non-NULL: > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html > though there's been some pushback on C++ references in the past e.g. > from Jeff: > http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01430.html > > Here's what the "Explicit pointers, rather than typedefs" might look > like, but with references rather than ptrs for some vars where NULL > isn't valid: > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static gimple_phi * > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple &> *in, > + vec<gimple &> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + gimple *def; > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + gimple_phi_iterator gsi; > + vec<gimple_phi &> worklist = vNULL; > pointer_set_t *added_to_worklist; > > ...though arguably there's plenty more conversion of the above that > could be done: "def" is initialized once, with a non-NULL ptr, so > arguably could be reference, but that would require moving the decl down > to the point of initialization so I didn't touch it above. I think use > of references would tend to break up such declarations of locals. > > C++ references (with implicit namespaces) > ========================================= > ...and here's what the above "namespaces with a using decl" approach > might look like with references: > > using gimple; > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static phi * > +detect_commutative_reduction (scop_p scop, stmt &stmt, > + vec<stmt &> &in, > + vec<stmt &> &out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > - tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); > + stmt *def; > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); > + tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + phi_iterator gsi; > + vec<phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > > So the above hopefully gives an idea of what a more compile-time > type-safe gimple API might look like; sorry if I've mischaracterized any > of the ideas. I believe they're roughly sorted by increasing > invasiveness, and by increasing "C++ ness" (both of which give me > pause). > > Thoughts? The 'should gimple be a pointer typedef' issue is rather orthogonal and it merely affects how we do const-correctness (but it affects your ongoing work, thus I brought it up - also because you address the const correctness issue). It's convenient to do such change first IMHO. And I never liked the const_ typedef variants that were introduced. The main reason for them was probably to avoid all the churn of replacing the tree pointer typedef with a tree (non-pointer) typedef. The mistake was to repeat that for 'gimple' ... I have no strong opinion on const correctness in general, but I do have a strong opinion against introducing more of the const_* typedefs. Those simply feel completely bogus (and alienate the new GCC developers we want to attract with C++ and all these changes (uh? heh!?)). So if you turn gimple up-side-down (which you do with more static type checking) then please fix const correctness first. As of namespaces - yes, ideally we'd move the various prefixes to namespaces (that a gimple pass can simply use for example). But that's again an orthogonal issue. You could always add the namespace with your work and add typedefs like typedef gimple::phi gimple_phi; (though that invites inconsistent coding-style, using gimple::phi vs. gimple_phi throughout the code) Richard. > FWIW, my own preference is for "Direct use of is-a.h, retaining typedefs > of pointers" but that may be because it would be less work for me ;) > > Andrew: I know you've been working on improving the typesafety of > expressions vs types in the middle-end. I'm curious as to what the > above code fragments look like with just your changes? > > Hope this is useful. > Dave > >
On Fri, 2014-04-25 at 10:37 +0200, Richard Biener wrote: > On Thu, Apr 24, 2014 at 4:59 PM, David Malcolm <dmalcolm@redhat.com> wrote: > > On Thu, 2014-04-24 at 09:09 -0400, Andrew MacLeod wrote: > >> On 04/24/2014 04:33 AM, Richard Biener wrote: > >> > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: > >> >> On 04/23/14 15:13, David Malcolm wrote: > >> >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: > >> >>>> On 04/21/14 10:56, David Malcolm wrote: > >> >>>>> This updates all of the gimple_bind_* accessors in gimple.h from taking > >> >>>>> a > >> >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the > >> >>>>> checking happening at the point of cast. > >> >>>>> > >> >>>>> Various other types are strengthened from gimple to gimple_bind, and > >> >>>>> from > >> >>>>> plain vec<gimple> to vec<gimple_bind>. > >> >>> > >> >>> [...] > >> >>> > >> >>>> This is fine, with the same requested changes as #2; specifically using > >> >>>> an explicit cast rather than hiding the conversion in a method. Once > >> >>>> those changes are in place, it's good for 4.9.1. > >> >>> Thanks - presumably you mean > >> >>> "good for *trunk* after 4.9.1 is released" > >> >> Right. Sorry for the confusion. > >> > Note I still want that less-typedefs (esp. the const_*) variants to be explored. > >> > Changing this will touch all the code again, so I'd like to avoid that. > >> > > >> > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' > >> > and not 'gimple_statement_base *'? The main reason that we have so > >> > many typedefs is that in C you had to use 'struct foo' each time you > >> > refer to foo as a type - I suppose it was then convenient to do the > >> > typedef to the pointer type. With 'gimple' being not a pointer type > >> > we get const correctness in the way people would expect it to work. > >> > [no, I don't suggest you change 'tree' or 'const_tree' at this point, just > >> > gimple (and maybe gimple_seq) as you are working on the 'gimple' > >> > type anyway]. > >> > > >> > > >> > >> So if we change 'gimple' everywhere to be 'gimple *', can we just > >> abandon the 'gimple' typedef completely and go directly to using > >> something like gimple_stmt, or some other agreeable name instead? > >> > >> I think its more descriptive and then frees up the generic 'gimple' name > >> should we decide to do something more with namespaces in the future... > > > > There have been a few different proposals as to what the resulting > > gimple API might look like, in various subthreads of this discusssion, > > so I thought it might help the discussion to gather up the proposals, > > and to apply them to some specific code examples, to see what the > > results might look like. > > > > So here are a couple of code fragments, from gcc/graphite-sese-to-poly.c > > and gcc/tree-ssa-uninit.c respectively: > > > > Status quo > > ========== > > > > static gimple > > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > vec<gimple> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > gimple def, loop_phi, phi, close_phi = stmt; > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > gimple_stmt_iterator gsi; > > vec<gimple> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > The currently-posted patch series > > ================================= > > Here's the cumulative effect of the patch series I posted, using the > > casting methods of the base class (the "stmt->as_a_gimple_phi" call): > > > > -static gimple > > +static gimple_phi > > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > vec<gimple> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > + gimple def; > > + gimple_phi loop_phi, phi, close_phi = stmt->as_a_gimple_phi (); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + gimple_phi_iterator gsi; > > + vec<gimple_phi> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > Direct use of is-a.h, retaining typedefs of pointers > > ==================================================== > > The following patch shows what the above might look like using the patch > > series as posted, but eliminating the casting methods in favor of > > direct use of the is-a.h API (but still using lots of typedefs of > > pointers): > > > > -static gimple > > +static gimple_phi > > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > vec<gimple> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + gimple def; > > + gimple_phi loop_phi, phi, close_phi = as_a <gimple_phi> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + gimple_phi_iterator gsi; > > + vec<gimple_phi> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > I posted an example of porting a patch in the series to this approach as: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html > > > > Explicit pointers, rather than typedefs > > ======================================= > > Richi suggested making pointers be explicit rather than hidden in the > > typedefs in: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html > > which might give something like this: > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static gimple_phi * > > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, > > + vec<gimple *> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + gimple *def; > > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + gimple_phi_iterator gsi; > > + vec<gimple_phi *> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > Changing the meaning of "gimple" makes this a much bigger patch IMHO > > than what I've currently posted. One way to achieve this could be a > > mega-patch (ugh) which ports the whole middle-end at once to change the > > "pointerness" of "gimple", probably auto-generated and, once that's in > > place, then look at introducing subclass usage. > > > > Note: it's more fiddly than a simply sed of "gimple" to "gimple *" (or > > whatever); consider the gimple_phi decls above, which would change > > thusly: > > - gimple def, loop_phi, phi, close_phi = stmt; > > + gimple *def, *loop_phi, *phi, *close_phi = stmt; > > > > Implicit naming > > =============== > > Several people have suggested that the "gimple_" prefix is redundant. > > > > Andrew MacLeod suggested in: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > > that we could simple drop the "gimple_" prefix. Combining this with the > > pointer approach, for example, gives: > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static phi * > > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, > > + vec<gimple *> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + gimple *def; > > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + phi_iterator gsi; > > + vec<phi *> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > though it could also be done with typedefs of pointers, rather than the > > "explicit pointers" above. > > > > > > Namespaces (explicit) > > ===================== > > Continuing with the idea that the "gimple_" prefix is redundant, Andrew > > MacLeod also suggested in: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > > that we could repurpose "gimple" to be a namespace. Here's what things > > might look like written out in full (perhaps using gimple::stmt to be > > the base class): > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static gimple::phi * > > +detect_commutative_reduction (scop_p scop, gimple::stmt *stmt, > > + vec<gimple::stmt *> *in, > > + vec<gimple::stmt *> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + gimple::stmt *def; > > + gimple::phi loop_phi, phi, close_phi = as_a <gimple::phi *> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + gimple::phi_iterator gsi; > > + vec<gimple::phi *> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > This may require some gengtype support, for the case of fields within > > structs. > > > > Andrew suggested renaming "gimple" to "gimple_stmt" in: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html > > as a possible migration path towards this. > > > > Namespaces (implicit) > > ===================== > > The above is, of course, verbose - I'm mostly posting it to clarify the > > following, which uses a "using" decl to eliminate all of the "gimple::" > > from the above: > > > > using gimple; > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static phi * > > +detect_commutative_reduction (scop_p scop, stmt *stmt, > > + vec<stmt *> *in, > > + vec<stmt *> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + stmt *def; > > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + phi_iterator gsi; > > + vec<phi *> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > This would require some gengtype support (again, for the case of fields > > within structs). > > > > C++ references (without namespaces) > > =================================== > > Richi suggested the use of references rather than pointers when the > > address is required to be non-NULL: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html > > though there's been some pushback on C++ references in the past e.g. > > from Jeff: > > http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01430.html > > > > Here's what the "Explicit pointers, rather than typedefs" might look > > like, but with references rather than ptrs for some vars where NULL > > isn't valid: > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static gimple_phi * > > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple &> *in, > > + vec<gimple &> *out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > + gimple *def; > > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); > > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + gimple_phi_iterator gsi; > > + vec<gimple_phi &> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > ...though arguably there's plenty more conversion of the above that > > could be done: "def" is initialized once, with a non-NULL ptr, so > > arguably could be reference, but that would require moving the decl down > > to the point of initialization so I didn't touch it above. I think use > > of references would tend to break up such declarations of locals. > > > > C++ references (with implicit namespaces) > > ========================================= > > ...and here's what the above "namespaces with a using decl" approach > > might look like with references: > > > > using gimple; > > > > -static gimple > > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > > - vec<gimple> *out) > > +static phi * > > +detect_commutative_reduction (scop_p scop, stmt &stmt, > > + vec<stmt &> &in, > > + vec<stmt &> &out) > > { > > if (scalar_close_phi_node_p (stmt)) > > { > > - gimple def, loop_phi, phi, close_phi = stmt; > > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > > - tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); > > + stmt *def; > > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); > > + tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); > > > > if (TREE_CODE (arg) != SSA_NAME) > > > > /* ...etc... */ > > > > static unsigned int > > execute_late_warn_uninitialized (void) > > { > > basic_block bb; > > - gimple_stmt_iterator gsi; > > - vec<gimple> worklist = vNULL; > > + phi_iterator gsi; > > + vec<phi *> worklist = vNULL; > > pointer_set_t *added_to_worklist; > > > > > > So the above hopefully gives an idea of what a more compile-time > > type-safe gimple API might look like; sorry if I've mischaracterized any > > of the ideas. I believe they're roughly sorted by increasing > > invasiveness, and by increasing "C++ ness" (both of which give me > > pause). > > > > Thoughts? > > The 'should gimple be a pointer typedef' issue is rather orthogonal > and it merely affects how we do const-correctness > (but it affects your ongoing work, thus I brought it up - also because > you address the const correctness issue). > > It's convenient to do such change first IMHO. And I never liked the > const_ typedef variants that were introduced. The main reason for > them was probably to avoid all the churn of replacing the tree pointer > typedef with a tree (non-pointer) typedef. The mistake was to > repeat that for 'gimple' ... > > I have no strong opinion on const correctness in general, but I do > have a strong opinion against introducing more of the const_* > typedefs. Those simply feel completely bogus (and alienate the new > GCC developers we want to attract with C++ and all these changes (uh? heh!?)). > > So if you turn gimple up-side-down (which you do with more static > type checking) then please fix const correctness first. OK. I've started looking at this, and immediately ran into an annoying gengtype limitation; it can't yet cope with anything beyond: vec<ID1, ID2, ..., IDN> and so complains with things like: vec<gimple *> vec<const gimple *> so I'll have a look at fixing that, since that would otherwise block the more concrete things like: vec<const gimple_phi *> that motivate this upheaval. > As of namespaces - yes, ideally we'd move the various prefixes > to namespaces (that a gimple pass can simply use for example). > But that's again an orthogonal issue. You could always add the > namespace with your work and add typedefs like > > typedef gimple::phi gimple_phi; > > (though that invites inconsistent coding-style, using gimple::phi > vs. gimple_phi throughout the code) OK. Given your preference for doing this without typedefs, I'm working on an automated way to convert "gimple" / "const_gimple" to... something, making it (I hope) relatively easy to choose whether that something should be: gimple * const gimple * or gimple_stmt * const gimple_stmt * or gimple::stmt * const gimple::stmt * or stmt * const stmt * with the last one maybe using C++ namespaces with a "using gimple" decl (and thus actually a "gimple::stmt", or maybe just being a plain class. One much more invasive possible change I didn't mention in the "Examples" email would be to convert the gimple accessors to be actual methods, giving something like this (in this case built on top of the renaming, with implicit namespaces idea): using gimple; -static gimple -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, - vec<gimple> *out) +static phi * +detect_commutative_reduction (scop_p scop, stmt *stmt, + vec<stmt *> *in, + vec<stmt *> *out) { if (scalar_close_phi_node_p (stmt)) { - gimple def, loop_phi, phi, close_phi = stmt; - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); + stmt *def; + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); + tree init, lhs, arg = close_phi->arg_def (0); if (TREE_CODE (arg) != SSA_NAME) /* ...etc... */ static unsigned int execute_late_warn_uninitialized (void) { basic_block bb; - gimple_stmt_iterator gsi; - vec<gimple> worklist = vNULL; + phi_iterator gsi; + vec<phi *> worklist = vNULL; pointer_set_t *added_to_worklist; (i.e. note how the call to gimple_phi_arg_def has become a method call). I assumed that such a change would be simply too much upheaval (and would impact greppability), but since you seem to be advocating for a "if we're going to do it, do it properly" position, I was wondering your thoughts on that kind of change? (the patches would be *much* bigger, of course, since it means changing every callsite rather than just every decl). Again, this may be simply orthogonal to the original goal of expressing the gimple codes in a way that can be checked at compile-time. I'm not especially keen on such a further transition - more work and churn [I can envisage a transition path in which the accessor functions become calls to methods so that no callsites need changing, and then the accessor functions gradually get removed, porting their callsites to use methods]. Dave > Richard. > > > FWIW, my own preference is for "Direct use of is-a.h, retaining typedefs > > of pointers" but that may be because it would be less work for me ;) > > > > Andrew: I know you've been working on improving the typesafety of > > expressions vs types in the middle-end. I'm curious as to what the > > above code fragments look like with just your changes? > > > > Hope this is useful. > > Dave > > > >
On Fri, Apr 25, 2014 at 5:28 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Fri, 2014-04-25 at 10:37 +0200, Richard Biener wrote: >> On Thu, Apr 24, 2014 at 4:59 PM, David Malcolm <dmalcolm@redhat.com> wrote: >> > On Thu, 2014-04-24 at 09:09 -0400, Andrew MacLeod wrote: >> >> On 04/24/2014 04:33 AM, Richard Biener wrote: >> >> > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <law@redhat.com> wrote: >> >> >> On 04/23/14 15:13, David Malcolm wrote: >> >> >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >> >> >>>> On 04/21/14 10:56, David Malcolm wrote: >> >> >>>>> This updates all of the gimple_bind_* accessors in gimple.h from taking >> >> >>>>> a >> >> >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with the >> >> >>>>> checking happening at the point of cast. >> >> >>>>> >> >> >>>>> Various other types are strengthened from gimple to gimple_bind, and >> >> >>>>> from >> >> >>>>> plain vec<gimple> to vec<gimple_bind>. >> >> >>> >> >> >>> [...] >> >> >>> >> >> >>>> This is fine, with the same requested changes as #2; specifically using >> >> >>>> an explicit cast rather than hiding the conversion in a method. Once >> >> >>>> those changes are in place, it's good for 4.9.1. >> >> >>> Thanks - presumably you mean >> >> >>> "good for *trunk* after 4.9.1 is released" >> >> >> Right. Sorry for the confusion. >> >> > Note I still want that less-typedefs (esp. the const_*) variants to be explored. >> >> > Changing this will touch all the code again, so I'd like to avoid that. >> >> > >> >> > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' >> >> > and not 'gimple_statement_base *'? The main reason that we have so >> >> > many typedefs is that in C you had to use 'struct foo' each time you >> >> > refer to foo as a type - I suppose it was then convenient to do the >> >> > typedef to the pointer type. With 'gimple' being not a pointer type >> >> > we get const correctness in the way people would expect it to work. >> >> > [no, I don't suggest you change 'tree' or 'const_tree' at this point, just >> >> > gimple (and maybe gimple_seq) as you are working on the 'gimple' >> >> > type anyway]. >> >> > >> >> > >> >> >> >> So if we change 'gimple' everywhere to be 'gimple *', can we just >> >> abandon the 'gimple' typedef completely and go directly to using >> >> something like gimple_stmt, or some other agreeable name instead? >> >> >> >> I think its more descriptive and then frees up the generic 'gimple' name >> >> should we decide to do something more with namespaces in the future... >> > >> > There have been a few different proposals as to what the resulting >> > gimple API might look like, in various subthreads of this discusssion, >> > so I thought it might help the discussion to gather up the proposals, >> > and to apply them to some specific code examples, to see what the >> > results might look like. >> > >> > So here are a couple of code fragments, from gcc/graphite-sese-to-poly.c >> > and gcc/tree-ssa-uninit.c respectively: >> > >> > Status quo >> > ========== >> > >> > static gimple >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > gimple def, loop_phi, phi, close_phi = stmt; >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > gimple_stmt_iterator gsi; >> > vec<gimple> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > The currently-posted patch series >> > ================================= >> > Here's the cumulative effect of the patch series I posted, using the >> > casting methods of the base class (the "stmt->as_a_gimple_phi" call): >> > >> > -static gimple >> > +static gimple_phi >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > + gimple def; >> > + gimple_phi loop_phi, phi, close_phi = stmt->as_a_gimple_phi (); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > Direct use of is-a.h, retaining typedefs of pointers >> > ==================================================== >> > The following patch shows what the above might look like using the patch >> > series as posted, but eliminating the casting methods in favor of >> > direct use of the is-a.h API (but still using lots of typedefs of >> > pointers): >> > >> > -static gimple >> > +static gimple_phi >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple def; >> > + gimple_phi loop_phi, phi, close_phi = as_a <gimple_phi> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > I posted an example of porting a patch in the series to this approach as: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html >> > >> > Explicit pointers, rather than typedefs >> > ======================================= >> > Richi suggested making pointers be explicit rather than hidden in the >> > typedefs in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html >> > which might give something like this: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple_phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, >> > + vec<gimple *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > Changing the meaning of "gimple" makes this a much bigger patch IMHO >> > than what I've currently posted. One way to achieve this could be a >> > mega-patch (ugh) which ports the whole middle-end at once to change the >> > "pointerness" of "gimple", probably auto-generated and, once that's in >> > place, then look at introducing subclass usage. >> > >> > Note: it's more fiddly than a simply sed of "gimple" to "gimple *" (or >> > whatever); consider the gimple_phi decls above, which would change >> > thusly: >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > + gimple *def, *loop_phi, *phi, *close_phi = stmt; >> > >> > Implicit naming >> > =============== >> > Several people have suggested that the "gimple_" prefix is redundant. >> > >> > Andrew MacLeod suggested in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > that we could simple drop the "gimple_" prefix. Combining this with the >> > pointer approach, for example, gives: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> *in, >> > + vec<gimple *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > though it could also be done with typedefs of pointers, rather than the >> > "explicit pointers" above. >> > >> > >> > Namespaces (explicit) >> > ===================== >> > Continuing with the idea that the "gimple_" prefix is redundant, Andrew >> > MacLeod also suggested in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > that we could repurpose "gimple" to be a namespace. Here's what things >> > might look like written out in full (perhaps using gimple::stmt to be >> > the base class): >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple::phi * >> > +detect_commutative_reduction (scop_p scop, gimple::stmt *stmt, >> > + vec<gimple::stmt *> *in, >> > + vec<gimple::stmt *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple::stmt *def; >> > + gimple::phi loop_phi, phi, close_phi = as_a <gimple::phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple::phi_iterator gsi; >> > + vec<gimple::phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > This may require some gengtype support, for the case of fields within >> > structs. >> > >> > Andrew suggested renaming "gimple" to "gimple_stmt" in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > as a possible migration path towards this. >> > >> > Namespaces (implicit) >> > ===================== >> > The above is, of course, verbose - I'm mostly posting it to clarify the >> > following, which uses a "using" decl to eliminate all of the "gimple::" >> > from the above: >> > >> > using gimple; >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, stmt *stmt, >> > + vec<stmt *> *in, >> > + vec<stmt *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + stmt *def; >> > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > This would require some gengtype support (again, for the case of fields >> > within structs). >> > >> > C++ references (without namespaces) >> > =================================== >> > Richi suggested the use of references rather than pointers when the >> > address is required to be non-NULL: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html >> > though there's been some pushback on C++ references in the past e.g. >> > from Jeff: >> > http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01430.html >> > >> > Here's what the "Explicit pointers, rather than typedefs" might look >> > like, but with references rather than ptrs for some vars where NULL >> > isn't valid: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple_phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple &> *in, >> > + vec<gimple &> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi &> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > ...though arguably there's plenty more conversion of the above that >> > could be done: "def" is initialized once, with a non-NULL ptr, so >> > arguably could be reference, but that would require moving the decl down >> > to the point of initialization so I didn't touch it above. I think use >> > of references would tend to break up such declarations of locals. >> > >> > C++ references (with implicit namespaces) >> > ========================================= >> > ...and here's what the above "namespaces with a using decl" approach >> > might look like with references: >> > >> > using gimple; >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, stmt &stmt, >> > + vec<stmt &> &in, >> > + vec<stmt &> &out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > - tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); >> > + stmt *def; >> > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); >> > + tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > >> > So the above hopefully gives an idea of what a more compile-time >> > type-safe gimple API might look like; sorry if I've mischaracterized any >> > of the ideas. I believe they're roughly sorted by increasing >> > invasiveness, and by increasing "C++ ness" (both of which give me >> > pause). >> > >> > Thoughts? >> >> The 'should gimple be a pointer typedef' issue is rather orthogonal >> and it merely affects how we do const-correctness >> (but it affects your ongoing work, thus I brought it up - also because >> you address the const correctness issue). >> >> It's convenient to do such change first IMHO. And I never liked the >> const_ typedef variants that were introduced. The main reason for >> them was probably to avoid all the churn of replacing the tree pointer >> typedef with a tree (non-pointer) typedef. The mistake was to >> repeat that for 'gimple' ... >> >> I have no strong opinion on const correctness in general, but I do >> have a strong opinion against introducing more of the const_* >> typedefs. Those simply feel completely bogus (and alienate the new >> GCC developers we want to attract with C++ and all these changes (uh? heh!?)). >> >> So if you turn gimple up-side-down (which you do with more static >> type checking) then please fix const correctness first. > > OK. I've started looking at this, and immediately ran into an annoying > gengtype limitation; it can't yet cope with anything beyond: > > vec<ID1, ID2, ..., IDN> > > and so complains with things like: > > vec<gimple *> > vec<const gimple *> > > so I'll have a look at fixing that, since that would otherwise block the > more concrete things like: > > vec<const gimple_phi *> > > that motivate this upheaval. > >> As of namespaces - yes, ideally we'd move the various prefixes >> to namespaces (that a gimple pass can simply use for example). >> But that's again an orthogonal issue. You could always add the >> namespace with your work and add typedefs like >> >> typedef gimple::phi gimple_phi; >> >> (though that invites inconsistent coding-style, using gimple::phi >> vs. gimple_phi throughout the code) > > OK. Given your preference for doing this without typedefs, I'm working > on an automated way to convert "gimple" / "const_gimple" to... > something, making it (I hope) relatively easy to choose whether that > something should be: > gimple * const gimple * > or > gimple_stmt * const gimple_stmt * > or > gimple::stmt * const gimple::stmt * > or > stmt * const stmt * > with the last one maybe using C++ namespaces with a "using gimple" decl > (and thus actually a "gimple::stmt", or maybe just being a plain class. > > One much more invasive possible change I didn't mention in the > "Examples" email would be to convert the gimple accessors to be actual > methods, giving something like this (in this case built on top of the > renaming, with implicit namespaces idea): Yeah ... the usual argument against this is that it makes grepping harder (unless you make it phi->phi_arg_def () ...). Not the very strongest argument I'd say, but I'd like to defer this to a separate discussion ... ;) We'd still have the mix of tree and GIMPLE objects everywhere so mixing both styles will make our code-base very ugly (unless you volunteer to apply the same transforms to 'tree' ...). So, please not at this point in time ;) Thanks, Richard. > using gimple; > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static phi * > +detect_commutative_reduction (scop_p scop, stmt *stmt, > + vec<stmt *> *in, > + vec<stmt *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + stmt *def; > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); > + tree init, lhs, arg = close_phi->arg_def (0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + phi_iterator gsi; > + vec<phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > (i.e. note how the call to gimple_phi_arg_def has become a method call). > > I assumed that such a change would be simply too much upheaval (and > would impact greppability), but since you seem to be advocating for a > "if we're going to do it, do it properly" position, I was wondering your > thoughts on that kind of change? (the patches would be *much* bigger, > of course, since it means changing every callsite rather than just every > decl). Again, this may be simply orthogonal to the original goal of > expressing the gimple codes in a way that can be checked at > compile-time. I'm not especially keen on such a further transition - > more work and churn [I can envisage a transition path in which the > accessor functions become calls to methods so that no callsites need > changing, and then the accessor functions gradually get removed, porting > their callsites to use methods]. > > Dave > >> Richard. >> >> > FWIW, my own preference is for "Direct use of is-a.h, retaining typedefs >> > of pointers" but that may be because it would be less work for me ;) >> > >> > Andrew: I know you've been working on improving the typesafety of >> > expressions vs types in the middle-end. I'm curious as to what the >> > above code fragments look like with just your changes? >> > >> > Hope this is useful. >> > Dave >> > >> > > >
diff --git a/gcc/c-family/c-gimplify.c b/gcc/c-family/c-gimplify.c index 737be4d..460a413 100644 --- a/gcc/c-family/c-gimplify.c +++ b/gcc/c-family/c-gimplify.c @@ -112,8 +112,8 @@ add_block_to_enclosing (tree block) { unsigned i; tree enclosing; - gimple bind; - vec<gimple> stack = gimple_bind_expr_stack (); + gimple_bind bind; + vec<gimple_bind> stack = gimple_bind_expr_stack (); FOR_EACH_VEC_ELT (stack, i, bind) if (gimple_bind_block (bind)) diff --git a/gcc/coretypes.h b/gcc/coretypes.h index a39ae1a..a39900f 100644 --- a/gcc/coretypes.h +++ b/gcc/coretypes.h @@ -73,6 +73,10 @@ struct gimple_statement_switch; typedef struct gimple_statement_switch *gimple_switch; typedef const struct gimple_statement_switch *const_gimple_switch; +struct gimple_statement_bind; +typedef struct gimple_statement_bind *gimple_bind; +typedef const struct gimple_statement_bind *const_gimple_bind; + union section; typedef union section section; struct gcc_options; diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py index 891877a..5dcc76f 100644 --- a/gcc/gdbhooks.py +++ b/gcc/gdbhooks.py @@ -458,7 +458,9 @@ def build_pretty_printer(): # Keep this in the same order as gimple.def: 'gimple_switch', 'const_gimple_switch', - 'gimple_statement_switch *'], + 'gimple_statement_switch *', + 'gimple_bind', 'const_gimple_bind', + 'gimple_statement_bind *'], 'gimple', GimplePrinter) diff --git a/gcc/gimple-low.c b/gcc/gimple-low.c index 80fd786..d56ff0a 100644 --- a/gcc/gimple-low.c +++ b/gcc/gimple-low.c @@ -380,7 +380,7 @@ static void lower_gimple_bind (gimple_stmt_iterator *gsi, struct lower_data *data) { tree old_block = data->block; - gimple stmt = gsi_stmt (*gsi); + gimple_bind stmt = gsi_stmt (*gsi)->as_a_gimple_bind (); tree new_block = gimple_bind_block (stmt); if (new_block) @@ -574,7 +574,8 @@ gimple_stmt_may_fallthru (gimple stmt) return false; case GIMPLE_BIND: - return gimple_seq_may_fallthru (gimple_bind_body (stmt)); + return gimple_seq_may_fallthru ( + gimple_bind_body (stmt->as_a_gimple_bind ())); case GIMPLE_TRY: if (gimple_try_kind (stmt) == GIMPLE_TRY_CATCH) diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index 25abd77..22d927f 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -889,7 +889,7 @@ dump_gimple_goto (pretty_printer *buffer, gimple gs, int spc, int flags) TDF_* in dumpfile.h). */ static void -dump_gimple_bind (pretty_printer *buffer, gimple gs, int spc, int flags) +dump_gimple_bind (pretty_printer *buffer, gimple_bind gs, int spc, int flags) { if (flags & TDF_RAW) dump_gimple_fmt (buffer, spc, flags, "%G <", gs); @@ -2099,7 +2099,7 @@ pp_gimple_stmt_1 (pretty_printer *buffer, gimple gs, int spc, int flags) break; case GIMPLE_BIND: - dump_gimple_bind (buffer, gs, spc, flags); + dump_gimple_bind (buffer, gs->as_a_gimple_bind (), spc, flags); break; case GIMPLE_CALL: diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c index b6f0495..85b6dcd 100644 --- a/gcc/gimple-walk.c +++ b/gcc/gimple-walk.c @@ -541,8 +541,9 @@ walk_gimple_stmt (gimple_stmt_iterator *gsi, walk_stmt_fn callback_stmt, switch (gimple_code (stmt)) { case GIMPLE_BIND: - ret = walk_gimple_seq_mod (gimple_bind_body_ptr (stmt), callback_stmt, - callback_op, wi); + ret = + walk_gimple_seq_mod (gimple_bind_body_ptr (stmt->as_a_gimple_bind ()), + callback_stmt, callback_op, wi); if (ret) return wi->callback_result; break; diff --git a/gcc/gimple.c b/gcc/gimple.c index 6c524c2..b3fbb0f 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -513,10 +513,10 @@ gimple_build_nop (void) VARS are the variables in BODY. BLOCK is the containing block. */ -gimple +gimple_bind gimple_build_bind (tree vars, gimple_seq body, tree block) { - gimple p = gimple_alloc (GIMPLE_BIND, 0); + gimple_bind p = gimple_alloc (GIMPLE_BIND, 0)->as_a_gimple_bind (); gimple_bind_set_vars (p, vars); if (body) gimple_bind_set_body (p, body); @@ -1248,8 +1248,8 @@ empty_stmt_p (gimple stmt) { if (gimple_code (stmt) == GIMPLE_NOP) return true; - if (gimple_code (stmt) == GIMPLE_BIND) - return empty_body_p (gimple_bind_body (stmt)); + if (gimple_bind bind_stmt = stmt->dyn_cast_gimple_bind ()) + return empty_body_p (gimple_bind_body (bind_stmt)); return false; } @@ -1621,10 +1621,15 @@ gimple_copy (gimple stmt) switch (gimple_code (stmt)) { case GIMPLE_BIND: - new_seq = gimple_seq_copy (gimple_bind_body (stmt)); - gimple_bind_set_body (copy, new_seq); - gimple_bind_set_vars (copy, unshare_expr (gimple_bind_vars (stmt))); - gimple_bind_set_block (copy, gimple_bind_block (stmt)); + { + gimple_bind bind_stmt = stmt->as_a_gimple_bind (); + gimple_bind bind_copy = copy->as_a_gimple_bind (); + new_seq = gimple_seq_copy (gimple_bind_body (bind_stmt)); + gimple_bind_set_body (bind_copy, new_seq); + gimple_bind_set_vars (bind_copy, + unshare_expr (gimple_bind_vars (bind_stmt))); + gimple_bind_set_block (bind_copy, gimple_bind_block (bind_stmt)); + } break; case GIMPLE_CATCH: diff --git a/gcc/gimple.h b/gcc/gimple.h index b448e0e..620495d 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -220,6 +220,12 @@ public: return as_a <gimple_statement_switch> (this); } + inline gimple_bind + as_a_gimple_bind () + { + return as_a <gimple_statement_bind> (this); + } + /* Dynamic casting methods, where the cast returns NULL if the stmt is not of the required kind. @@ -234,6 +240,12 @@ public: return dyn_cast <gimple_statement_switch> (this); } + inline gimple_bind + dyn_cast_gimple_bind () + { + return dyn_cast <gimple_statement_bind> (this); + } + }; @@ -1234,7 +1246,7 @@ void gimple_cond_set_condition_from_tree (gimple, tree); gimple gimple_build_label (tree label); gimple gimple_build_goto (tree dest); gimple gimple_build_nop (void); -gimple gimple_build_bind (tree, gimple_seq, tree); +gimple_bind gimple_build_bind (tree, gimple_seq, tree); gimple gimple_build_asm_vec (const char *, vec<tree, va_gc> *, vec<tree, va_gc> *, vec<tree, va_gc> *, vec<tree, va_gc> *); @@ -1369,6 +1381,16 @@ gimple_seq_first_stmt (gimple_seq s) return n; } +/* Return the first statement in GIMPLE sequence S as a gimple_bind, + verifying that it has code GIMPLE_BIND in a checked build. */ + +static inline gimple_bind +gimple_seq_first_stmt_as_a_bind (gimple_seq s) +{ + gimple_seq_node n = gimple_seq_first (s); + return as_a <gimple_statement_bind> (n); +} + /* Return the last node in GIMPLE sequence S. */ @@ -3128,10 +3150,8 @@ gimple_goto_set_dest (gimple gs, tree dest) /* Return the variables declared in the GIMPLE_BIND statement GS. */ static inline tree -gimple_bind_vars (const_gimple gs) +gimple_bind_vars (const_gimple_bind bind_stmt) { - const gimple_statement_bind *bind_stmt = - as_a <const gimple_statement_bind> (gs); return bind_stmt->vars; } @@ -3140,9 +3160,8 @@ gimple_bind_vars (const_gimple gs) statement GS. */ static inline void -gimple_bind_set_vars (gimple gs, tree vars) +gimple_bind_set_vars (gimple_bind bind_stmt, tree vars) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); bind_stmt->vars = vars; } @@ -3151,24 +3170,22 @@ gimple_bind_set_vars (gimple gs, tree vars) statement GS. */ static inline void -gimple_bind_append_vars (gimple gs, tree vars) +gimple_bind_append_vars (gimple_bind bind_stmt, tree vars) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); bind_stmt->vars = chainon (bind_stmt->vars, vars); } static inline gimple_seq * -gimple_bind_body_ptr (gimple gs) +gimple_bind_body_ptr (gimple_bind bind_stmt) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); return &bind_stmt->body; } /* Return the GIMPLE sequence contained in the GIMPLE_BIND statement GS. */ static inline gimple_seq -gimple_bind_body (gimple gs) +gimple_bind_body (gimple_bind gs) { return *gimple_bind_body_ptr (gs); } @@ -3178,9 +3195,8 @@ gimple_bind_body (gimple gs) statement GS. */ static inline void -gimple_bind_set_body (gimple gs, gimple_seq seq) +gimple_bind_set_body (gimple_bind bind_stmt, gimple_seq seq) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); bind_stmt->body = seq; } @@ -3188,9 +3204,8 @@ gimple_bind_set_body (gimple gs, gimple_seq seq) /* Append a statement to the end of a GIMPLE_BIND's body. */ static inline void -gimple_bind_add_stmt (gimple gs, gimple stmt) +gimple_bind_add_stmt (gimple_bind bind_stmt, gimple stmt) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); gimple_seq_add_stmt (&bind_stmt->body, stmt); } @@ -3198,9 +3213,8 @@ gimple_bind_add_stmt (gimple gs, gimple stmt) /* Append a sequence of statements to the end of a GIMPLE_BIND's body. */ static inline void -gimple_bind_add_seq (gimple gs, gimple_seq seq) +gimple_bind_add_seq (gimple_bind bind_stmt, gimple_seq seq) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); gimple_seq_add_seq (&bind_stmt->body, seq); } @@ -3209,10 +3223,8 @@ gimple_bind_add_seq (gimple gs, gimple_seq seq) GS. This is analogous to the BIND_EXPR_BLOCK field in trees. */ static inline tree -gimple_bind_block (const_gimple gs) +gimple_bind_block (const_gimple_bind bind_stmt) { - const gimple_statement_bind *bind_stmt = - as_a <const gimple_statement_bind> (gs); return bind_stmt->block; } @@ -3221,9 +3233,8 @@ gimple_bind_block (const_gimple gs) statement GS. */ static inline void -gimple_bind_set_block (gimple gs, tree block) +gimple_bind_set_block (gimple_bind bind_stmt, tree block) { - gimple_statement_bind *bind_stmt = as_a <gimple_statement_bind> (gs); gcc_gimple_checking_assert (block == NULL_TREE || TREE_CODE (block) == BLOCK); bind_stmt->block = block; diff --git a/gcc/gimplify.c b/gcc/gimplify.c index ad2178d..533766e 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -112,7 +112,7 @@ struct gimplify_ctx { struct gimplify_ctx *prev_context; - vec<gimple> bind_expr_stack; + vec<gimple_bind> bind_expr_stack; tree temps; gimple_seq conditional_cleanups; tree exit_label; @@ -262,10 +262,10 @@ pop_gimplify_context (gimple body) /* Push a GIMPLE_BIND tuple onto the stack of bindings. */ static void -gimple_push_bind_expr (gimple gimple_bind) +gimple_push_bind_expr (gimple_bind bind_stmt) { gimplify_ctxp->bind_expr_stack.reserve (8); - gimplify_ctxp->bind_expr_stack.safe_push (gimple_bind); + gimplify_ctxp->bind_expr_stack.safe_push (bind_stmt); } /* Pop the first element off the stack of bindings. */ @@ -278,7 +278,7 @@ gimple_pop_bind_expr (void) /* Return the first element of the stack of bindings. */ -gimple +gimple_bind gimple_current_bind_expr (void) { return gimplify_ctxp->bind_expr_stack.last (); @@ -286,7 +286,7 @@ gimple_current_bind_expr (void) /* Return the stack of bindings created during gimplification. */ -vec<gimple> +vec<gimple_bind> gimple_bind_expr_stack (void) { return gimplify_ctxp->bind_expr_stack; @@ -564,14 +564,14 @@ get_initialized_tmp_var (tree val, gimple_seq *pre_p, gimple_seq *post_p) generate debug info for them; otherwise don't. */ void -declare_vars (tree vars, gimple scope, bool debug_info) +declare_vars (tree vars, gimple gs, bool debug_info) { tree last = vars; if (last) { tree temps, block; - gcc_assert (gimple_code (scope) == GIMPLE_BIND); + gimple_bind scope = gs->as_a_gimple_bind (); temps = nreverse (last); @@ -1024,7 +1024,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p) tree bind_expr = *expr_p; bool old_save_stack = gimplify_ctxp->save_stack; tree t; - gimple gimple_bind; + gimple_bind gimple_bind; gimple_seq body, cleanup; gimple stack_save; @@ -8520,12 +8520,13 @@ gimplify_one_sizepos (tree *expr_p, gimple_seq *stmt_p) containing the sequence of corresponding GIMPLE statements. If DO_PARMS is true, also gimplify the parameters. */ -gimple +gimple_bind gimplify_body (tree fndecl, bool do_parms) { location_t saved_location = input_location; gimple_seq parm_stmts, seq; - gimple outer_bind; + gimple outer_stmt; + gimple_bind outer_bind; struct cgraph_node *cgn; timevar_push (TV_TREE_GIMPLIFY); @@ -8565,18 +8566,18 @@ gimplify_body (tree fndecl, bool do_parms) /* Gimplify the function's body. */ seq = NULL; gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq); - outer_bind = gimple_seq_first_stmt (seq); - if (!outer_bind) + outer_stmt = gimple_seq_first_stmt (seq); + if (!outer_stmt) { - outer_bind = gimple_build_nop (); - gimplify_seq_add_stmt (&seq, outer_bind); + outer_stmt = gimple_build_nop (); + gimplify_seq_add_stmt (&seq, outer_stmt); } /* The body must contain exactly one statement, a GIMPLE_BIND. If this is not the case, wrap everything in a GIMPLE_BIND to make it so. */ - if (gimple_code (outer_bind) == GIMPLE_BIND + if (gimple_code (outer_stmt) == GIMPLE_BIND && gimple_seq_first (seq) == gimple_seq_last (seq)) - ; + outer_bind = as_a<gimple_statement_bind> (outer_stmt); else outer_bind = gimple_build_bind (NULL_TREE, seq, NULL); @@ -8690,7 +8691,7 @@ gimplify_function_tree (tree fndecl) { tree parm, ret; gimple_seq seq; - gimple bind; + gimple_bind bind; gcc_assert (!gimple_body (fndecl)); @@ -8734,7 +8735,7 @@ gimplify_function_tree (tree fndecl) && !flag_instrument_functions_exclude_p (fndecl)) { tree x; - gimple new_bind; + gimple_bind new_bind; gimple tf; gimple_seq cleanup = NULL, body = NULL; tree tmp_var; diff --git a/gcc/gimplify.h b/gcc/gimplify.h index 6bc0057..c37b5ab 100644 --- a/gcc/gimplify.h +++ b/gcc/gimplify.h @@ -53,8 +53,8 @@ extern void free_gimplify_stack (void); extern void push_gimplify_context (bool in_ssa = false, bool rhs_cond_ok = false); extern void pop_gimplify_context (gimple); -extern gimple gimple_current_bind_expr (void); -extern vec<gimple> gimple_bind_expr_stack (void); +extern gimple_bind gimple_current_bind_expr (void); +extern vec<gimple_bind> gimple_bind_expr_stack (void); extern void gimplify_and_add (tree, gimple_seq *); extern tree get_formal_tmp_var (tree, gimple_seq *); extern tree get_initialized_tmp_var (tree, gimple_seq *, gimple_seq *); @@ -75,7 +75,7 @@ extern enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq *, extern void gimplify_type_sizes (tree, gimple_seq *); extern void gimplify_one_sizepos (tree *, gimple_seq *); -extern gimple gimplify_body (tree, bool); +extern gimple_bind gimplify_body (tree, bool); extern void gimplify_function_tree (tree); extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, gimple_seq *); diff --git a/gcc/java/java-gimplify.c b/gcc/java/java-gimplify.c index 6c7a1b6..4565482 100644 --- a/gcc/java/java-gimplify.c +++ b/gcc/java/java-gimplify.c @@ -128,7 +128,7 @@ java_gimplify_block (tree java_block) { tree decls = BLOCK_VARS (java_block); tree body = BLOCK_EXPR_BODY (java_block); - gimple outer = gimple_current_bind_expr (); + gimple_bind outer = gimple_current_bind_expr (); tree block; /* Don't bother with empty blocks. */ diff --git a/gcc/omp-low.c b/gcc/omp-low.c index cd10717..59e632f 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -2674,7 +2674,9 @@ scan_omp_1_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p, *handled_ops_p = false; if (ctx) - for (var = gimple_bind_vars (stmt); var ; var = DECL_CHAIN (var)) + for (var = gimple_bind_vars (stmt->as_a_gimple_bind ()); + var ; + var = DECL_CHAIN (var)) insert_decl_map (&ctx->cb, var, var); } break; @@ -8407,7 +8409,8 @@ lower_omp_sections (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree block, control; gimple_stmt_iterator tgsi; - gimple stmt, new_stmt, bind, t; + gimple stmt, t; + gimple_bind new_stmt, bind; gimple_seq ilist, dlist, olist, new_body; stmt = gsi_stmt (*gsi_p); @@ -8616,7 +8619,8 @@ static void lower_omp_single (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree block; - gimple t, bind, single_stmt = gsi_stmt (*gsi_p); + gimple t, single_stmt = gsi_stmt (*gsi_p); + gimple_bind bind; gimple_seq bind_body, bind_body_tail = NULL, dlist; push_gimplify_context (); @@ -8674,7 +8678,8 @@ static void lower_omp_master (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree block, lab = NULL, x, bfn_decl; - gimple stmt = gsi_stmt (*gsi_p), bind; + gimple stmt = gsi_stmt (*gsi_p); + gimple_bind bind; location_t loc = gimple_location (stmt); gimple_seq tseq; @@ -8714,7 +8719,8 @@ lower_omp_master (gimple_stmt_iterator *gsi_p, omp_context *ctx) static void lower_omp_taskgroup (gimple_stmt_iterator *gsi_p, omp_context *ctx) { - gimple stmt = gsi_stmt (*gsi_p), bind, x; + gimple stmt = gsi_stmt (*gsi_p), x; + gimple_bind bind; tree block = make_node (BLOCK); bind = gimple_build_bind (NULL, NULL, block); @@ -8742,7 +8748,8 @@ static void lower_omp_ordered (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree block; - gimple stmt = gsi_stmt (*gsi_p), bind, x; + gimple stmt = gsi_stmt (*gsi_p), x; + gimple_bind bind; push_gimplify_context (); @@ -8785,7 +8792,8 @@ lower_omp_critical (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree block; tree name, lock, unlock; - gimple stmt = gsi_stmt (*gsi_p), bind; + gimple stmt = gsi_stmt (*gsi_p); + gimple_bind bind; location_t loc = gimple_location (stmt); gimple_seq tbody; @@ -8927,7 +8935,8 @@ lower_omp_for (gimple_stmt_iterator *gsi_p, omp_context *ctx) { tree *rhs_p, block; struct omp_for_data fd, *fdp = NULL; - gimple stmt = gsi_stmt (*gsi_p), new_stmt; + gimple stmt = gsi_stmt (*gsi_p); + gimple_bind new_stmt; gimple_seq omp_for_body, body, dlist; size_t i; @@ -8948,7 +8957,8 @@ lower_omp_for (gimple_stmt_iterator *gsi_p, omp_context *ctx) if (!gimple_seq_empty_p (omp_for_body) && gimple_code (gimple_seq_first_stmt (omp_for_body)) == GIMPLE_BIND) { - gimple inner_bind = gimple_seq_first_stmt (omp_for_body); + gimple_bind inner_bind = + gimple_seq_first_stmt (omp_for_body)->as_a_gimple_bind (); tree vars = gimple_bind_vars (inner_bind); gimple_bind_append_vars (new_stmt, vars); /* bind_vars/BLOCK_VARS are being moved to new_stmt/block, don't @@ -9442,12 +9452,12 @@ lower_omp_taskreg (gimple_stmt_iterator *gsi_p, omp_context *ctx) tree clauses; tree child_fn, t; gimple stmt = gsi_stmt (*gsi_p); - gimple par_bind, bind, dep_bind = NULL; + gimple_bind par_bind, bind, dep_bind = NULL; gimple_seq par_body, olist, ilist, par_olist, par_rlist, par_ilist, new_body; location_t loc = gimple_location (stmt); clauses = gimple_omp_taskreg_clauses (stmt); - par_bind = gimple_seq_first_stmt (gimple_omp_body (stmt)); + par_bind = gimple_seq_first_stmt_as_a_bind (gimple_omp_body (stmt)); par_body = gimple_bind_body (par_bind); child_fn = ctx->cb.dst_fn; if (gimple_code (stmt) == GIMPLE_OMP_PARALLEL @@ -9564,7 +9574,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx) tree clauses; tree child_fn, t, c; gimple stmt = gsi_stmt (*gsi_p); - gimple tgt_bind = NULL, bind; + gimple_bind tgt_bind = NULL, bind; gimple_seq tgt_body = NULL, olist, ilist, new_body; location_t loc = gimple_location (stmt); int kind = gimple_omp_target_kind (stmt); @@ -9573,7 +9583,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx) clauses = gimple_omp_target_clauses (stmt); if (kind == GF_OMP_TARGET_KIND_REGION) { - tgt_bind = gimple_seq_first_stmt (gimple_omp_body (stmt)); + tgt_bind = gimple_seq_first_stmt_as_a_bind (gimple_omp_body (stmt)); tgt_body = gimple_bind_body (tgt_bind); } else if (kind == GF_OMP_TARGET_KIND_DATA) @@ -9876,7 +9886,7 @@ lower_omp_teams (gimple_stmt_iterator *gsi_p, omp_context *ctx) push_gimplify_context (); tree block = make_node (BLOCK); - gimple bind = gimple_build_bind (NULL, NULL, block); + gimple_bind bind = gimple_build_bind (NULL, NULL, block); gsi_replace (gsi_p, bind, true); gimple_seq bind_body = NULL; gimple_seq dlist = NULL; @@ -10005,7 +10015,7 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context *ctx) lower_omp (gimple_transaction_body_ptr (stmt), ctx); break; case GIMPLE_BIND: - lower_omp (gimple_bind_body_ptr (stmt), ctx); + lower_omp (gimple_bind_body_ptr (stmt->as_a_gimple_bind ()), ctx); break; case GIMPLE_OMP_PARALLEL: case GIMPLE_OMP_TASK: diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 53aced3..b1355ab 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -4547,7 +4547,8 @@ verify_gimple_in_seq_2 (gimple_seq stmts) switch (gimple_code (stmt)) { case GIMPLE_BIND: - err |= verify_gimple_in_seq_2 (gimple_bind_body (stmt)); + err |= verify_gimple_in_seq_2 ( + gimple_bind_body (stmt->as_a_gimple_bind ())); break; case GIMPLE_TRY: @@ -8292,7 +8293,7 @@ do_warn_unused_result (gimple_seq seq) switch (gimple_code (g)) { case GIMPLE_BIND: - do_warn_unused_result (gimple_bind_body (g)); + do_warn_unused_result (gimple_bind_body (g->as_a_gimple_bind ())); break; case GIMPLE_TRY: do_warn_unused_result (gimple_try_eval (g)); diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index c0f93ac..bf1c3c7 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -744,7 +744,7 @@ remap_gimple_seq (gimple_seq body, copy_body_data *id) block using the mapping information in ID. */ static gimple -copy_gimple_bind (gimple stmt, copy_body_data *id) +copy_gimple_bind (gimple_bind stmt, copy_body_data *id) { gimple new_bind; tree new_block, new_vars; @@ -1292,7 +1292,7 @@ remap_gimple_stmt (gimple stmt, copy_body_data *id) switch (gimple_code (stmt)) { case GIMPLE_BIND: - copy = copy_gimple_bind (stmt, id); + copy = copy_gimple_bind (stmt->as_a_gimple_bind (), id); break; case GIMPLE_CATCH: @@ -3914,7 +3914,9 @@ estimate_num_insns (gimple stmt, eni_weights *weights) return 10; case GIMPLE_BIND: - return estimate_num_insns_seq (gimple_bind_body (stmt), weights); + return estimate_num_insns_seq ( + gimple_bind_body (stmt->as_a_gimple_bind ()), + weights); case GIMPLE_EH_FILTER: return estimate_num_insns_seq (gimple_eh_filter_failure (stmt), weights); @@ -4834,9 +4836,9 @@ replace_locals_stmt (gimple_stmt_iterator *gsip, struct walk_stmt_info *wi) { copy_body_data *id = (copy_body_data *) wi->info; - gimple stmt = gsi_stmt (*gsip); + gimple gs = gsi_stmt (*gsip); - if (gimple_code (stmt) == GIMPLE_BIND) + if (gimple_bind stmt = dyn_cast <gimple_statement_bind> (gs)) { tree block = gimple_bind_block (stmt); diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c index df6923f..78de464 100644 --- a/gcc/tree-nested.c +++ b/gcc/tree-nested.c @@ -1328,10 +1328,12 @@ convert_nonlocal_reference_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p, break; case GIMPLE_BIND: - if (!optimize && gimple_bind_block (stmt)) - note_nonlocal_block_vlas (info, gimple_bind_block (stmt)); + { + gimple_bind bind_stmt = stmt->as_a_gimple_bind (); + if (!optimize && gimple_bind_block (bind_stmt)) + note_nonlocal_block_vlas (info, gimple_bind_block (bind_stmt)); - for (tree var = gimple_bind_vars (stmt); var; var = DECL_CHAIN (var)) + for (tree var = gimple_bind_vars (bind_stmt); var; var = DECL_CHAIN (var)) if (TREE_CODE (var) == NAMELIST_DECL) { /* Adjust decls mentioned in NAMELIST_DECL. */ @@ -1352,7 +1354,7 @@ convert_nonlocal_reference_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p, *handled_ops_p = false; return NULL_TREE; - + } case GIMPLE_COND: wi->val_only = true; wi->is_lhs = false; @@ -1807,7 +1809,9 @@ convert_local_reference_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p, return NULL_TREE; case GIMPLE_BIND: - for (tree var = gimple_bind_vars (stmt); var; var = DECL_CHAIN (var)) + for (tree var = gimple_bind_vars (stmt->as_a_gimple_bind ()); + var; + var = DECL_CHAIN (var)) if (TREE_CODE (var) == NAMELIST_DECL) { /* Adjust decls mentioned in NAMELIST_DECL. */ @@ -2526,9 +2530,9 @@ finalize_nesting_tree_1 (struct nesting_info *root) /* If we created initialization statements, insert them. */ if (stmt_list) { - gimple bind; + gimple_bind bind; annotate_all_with_location (stmt_list, DECL_SOURCE_LOCATION (context)); - bind = gimple_seq_first_stmt (gimple_body (context)); + bind = gimple_seq_first_stmt_as_a_bind (gimple_body (context)); gimple_seq_add_seq (&stmt_list, gimple_bind_body (bind)); gimple_bind_set_body (bind, stmt_list); } @@ -2557,7 +2561,7 @@ finalize_nesting_tree_1 (struct nesting_info *root) if (root->debug_var_chain) { tree debug_var; - gimple scope; + gimple_bind scope; remap_vla_decls (DECL_INITIAL (root->context), root); @@ -2612,7 +2616,7 @@ finalize_nesting_tree_1 (struct nesting_info *root) pointer_map_destroy (id.cb.decl_map); } - scope = gimple_seq_first_stmt (gimple_body (root->context)); + scope = gimple_seq_first_stmt_as_a_bind (gimple_body (root->context)); if (gimple_bind_block (scope)) declare_vars (root->debug_var_chain, scope, true); else