Re: [PATCH, middle-end] Ping: Switch initializations conversion

[Diego Novillo <dnovillo at google dot com>]

On Wed, Jun 4, 2008 at 08:41, Martin Jambor <mjambor@suse.cz> wrote:

> 2008-06-04  Martin Jambor  <mjambor@suse.cz>
>        * Makefile.in (tree-switch-conversion.o): Add.
>        (OBJS-common): Add tree-swtch-conversion.o.
>        * passes.c (init_optimization_passes): Add pass_convert_switch.
>        * tree-pass.h: (pass_convert_switch): Add.
>        * tree-switch-conversion.c: New file.
>        * gcc.dg/tree-ssa/cswtch.c: New testcase.
>        * common.opt (ftree-cswtch): New option.
>        * params.h (PARAM_SWITCH_CONVERSION_BRANCH_RATIO): New parameter.
>        * params.def (PARAM_SWITCH_CONVERSION_BRANCH_RATIO): New parameter.
>        * opts.c (decode_options): Set flag_tree_switch_conversion when
>        optimization level is >= 2.
>        * doc/invoke.texi (Optimize Options): Added description of
>        -ftree-swtch-conversion and switch-conversion-max-branch-ratio
>        parameter.

Apologies again for having taking so long in reviewing this.  I
appreciate your patience.  The patch looks fine and the code is
easy to follow, thanks.  I only have some minor formatting
comments below.

Would you be willing to convert this pass to tuples after
committing it to mainline?  It doesn't look like the conversion
will be too complex.


> +There are further constraints.  Specifically, the range of values across all
> +case labels must not be bigger than SWITCH_CONVERSION_BRANCH_RATIO (default
> +eight) times the number of the actual switch branches.
> +*/

Closing comment goes in previous line.

> +/* Checks whether the range given by individual case statements isn't too big
> +   and whether the number of branches actually satisfy the size of the new
> +   array.  */
> +static bool
> +check_range (tree swtch)

Blank line after comment.  Argument SWTCH needs documenting.

> +  gcc_assert (info.range_size);
> +  if (!host_integerp (info.range_size, 1))
> +    {
> +      info.reason = "index range way too large or otherwise unusable.\n";
> +      return false;
> +    }
> +
> +  if ((unsigned HOST_WIDE_INT) tree_low_cst (info.range_size, 1)
> +      > ((unsigned) branch_num * SWITCH_CONVERSION_BRANCH_RATIO))
> +    {
> +      info.reason = "the maximum range-branch ratio exceeded.";
> +      return false;

The first info.reason string has a '\n' but the second doesn't.
Is this intentional?

> +    }
> +
> +  return true;
> +}
> +
> +/* Checks the given cs switch case whether it is suitable for conversion

s/cs/CS/

> +   (whether all but the default basic blocks are empty and so on).  If it is,
> +   adds the case to the branch list along with values for the defined variables
> +   and returns true.  Otherwise returns false.  */
> +static bool
> +check_process_case (tree cs)

Vertical spacing after comment.

> +  if (CASE_LOW (cs) == NULL_TREE)
> +    {
> +      /* default branch */

s/branch/branch.  /

> +/* check_final_bb() checks whether all required values in phi nodes in final_bb
> +   are constants.  Required values are those that correspond to a basic block
> +   which is a part of the examined switch statement.  It returns true if the
> +   phi nodes are OK, otherwise false.  */
> +static bool
> +check_final_bb (void)

Vertical spacing after comment.  Happens in all the other
functions.

> +/* free_temp_arrays() frees the arrays created by create_temp_arrays().  The

No need to reference the function name in the comment.  Something
like "Free the arrays created by create_temp_arrays...".

> +/* build_constructors() populates the vectors in the constructors array with
> +   future contents of the static arrays.  The vectors are populated in the
> +   order of phi nodes.  */
> +static void
> +build_constructors (tree swtch)

SWTCH needs commenting.

> +/* build_one_array() creates an appropriate array type and declaration and
> +   assembles a static array variable.  It also creates a load statement that
> +   initializes the variable in question with a value from the static array.  */
> +static void
> +build_one_array (tree swtch, int num, tree arr_index_type, tree phi, tree tidx)

All these arguments need documentation.  Several other functions
have the same problem.


Diego.