This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [tuples] Adding switch support to the gimplifier


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]