This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [tuples] Adding switch support to the gimplifier
- From: Diego Novillo <dnovillo at google dot com>
- To: Christopher Matthews <chrismatthews at google dot com>
- Cc: Aldy Hernandez <aldyh at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Wed, 11 Jul 2007 09:14:06 -0400
- Subject: Re: [tuples] Adding switch support to the gimplifier
- References: <5794c4c80707101951n6d5eb374vb1ca81e812eb1a53@mail.gmail.com>
On 7/10/07 10:51 PM, Christopher Matthews wrote:
> case_label_expr which has the high and low values. It might be
> necessary to make a gs_case_label to deal with this in a more
> reasonable way.
Yeah, GS_CASE_LABEL that inherits from GS_LABEL sounds like the way to go.
> * tree-pretty-print.c (dump_generic_node): Removed space before default
> label colon.
> * tree.h (sort_case_labels): Changed type to gimple vector.
> * gimplify.c (gimplify_ctx): Changed to a gimple vector.
> (compare_case_labels): Changed to work with gimpe statement labels with
> CASE_LABEL_EXPRs inside.
s/gimpe/gimple/
> * gimple-vec.h: Added.
This is missing from your patch. You have to 'svn add' it and then make
sure diffoptions contains -N. However, in this case, I don't think we
should add a new header file. If this file just contains the VEC
definitions, let's just add those to gimple-ir.h directly. No need to
have a new header file.
> -extern void sort_case_labels (tree);
> +extern void sort_case_labels (VEC(gimple,heap)*);
Space before '*'.
> + pp_string (buffer, "switch (");
> + dump_generic_node (buffer, gs_switch_index (gs), spc, flags, true);
> + pp_string (buffer, ") labels {");
I'd rather dump this as 'switch (index) <lab1, lab2, ... >'
> @@ -264,6 +288,15 @@ dump_gs_cond (pretty_printer *buffer, gi
> pp_character (buffer, '}');
> }
>
> +/* Dump a GS_LABEL tuple. */
Missing documentation on arguments.
> +
> +static void
> +dump_gs_label (pretty_printer *buffer, gimple gs, int spc, int flags) {
Brace on next line.
> + TDF_* in tree.h). ISSTMT specifics if there should be a ";" printed
> + after this GS statement. */
Let's not dump semicolons after GIMPLE statements. I forgot to take ';'
out of the dumps when I added this.
> void
> -dump_gimple_stmt (pretty_printer *buffer, gimple gs, int spc, int flags)
> +dump_gimple_expr (pretty_printer *buffer, gimple gs, int spc, int flags,
> + bool isStmt)
No. There are no GIMPLE expressions, just statements. We may have
GIMPLE operands in the future, when/if we convert tcc_reference,
tcc_declaration and tcc_const to tuples.
> -
> - pp_character (buffer, ';');
> + if (isStmt)
> + pp_character (buffer, ';');
This is not needed now. Just newline_and_indent() here.
> +void
> +dump_gimple_stmt (pretty_printer *buffer, gimple gs, int spc, int flags)
> +{
> + dump_gimple_expr (buffer, gs, spc, flags, true);
> +}
This is not needed.
> @@ -425,19 +426,12 @@ gs_build_resx (int region)
> gs_resx_set_region (p, region);
> return p;
> }
> +/* The helper for constructing a gimple switch statement. */
Vertical spacing before comment. Arguments need documentation (even if
it's just "X, Y and Z are as in gs_build_switch".
> +extern gimple gs_build_switch (unsigned int, tree, gimple, ...);
> +extern gimple gs_build_switch_vec (tree, gimple, VEC(gimple,heap)*);
Space before '*'.
> -static inline tree
> +static inline gimple
> gs_switch_default_label (gimple gs)
> {
> GS_CHECK (gs, GS_SWITCH);
> - return gs->gs_switch.labels[0];
> + return (gimple)gs->gs_switch.labels[0];
No. The labels stored inside the gs_switch are the actual tree labels,
not the GS_LABEL statements. This is like the GS_COND statement, we
store the labels directly. The GS_LABEL statement just acts as a
wrapper over the labels in the IL stream. But the referring
conditionals have the actual labels inside of them.
This will probably simplify the rest of your patch, as well.
> +static enum gimplify_status gimplify_statement_list (tree *, gs_seq);
No need to forward declare gimplify_statement_list if the whole function
can just be moved before the first use. Only forward declare if you
have a call cycle.
> + gs_seq switch_body_seq = ggc_alloc (sizeof (struct gs_sequence));
No need to allocate a new sequence if you declare it as a 'struct
gs_sequence'. Since, we're just going to add this sequence to PRE_P,
there's no need to allocate it on GC memory. Also, you're mixing
declarations with code.