Re: [PATCH][TUPLES] Tuplifying tree-vectorize.c

[Diego Novillo <dnovillo at google dot com>]

On 6/23/08 6:31 PM, Doug Kwan (éæå) wrote:

>     This is a partial conversion of pass_vectorize, which proves to be
> too big for one week-end :).

Heh, no kidding :)


> @@ -144,12 +144,11 @@
>
> along with GCC; see the file COPYING3.
> #include "tree-data-ref.h"
> #include "tree-scalar-evolution.h"
> #include "input.h"
> +#include "hashtab.h"

Needs corresponding changes in Makefile.in


> @@ -580,9 +591,7 @@
>
> slpeel_update_phi_nodes_for_guard1 (edge
> set_current_def (current_new_name, PHI_RESULT (new_phi));
> bitmap_set_bit (*defs, SSA_NAME_VERSION (current_new_name));
> }
> -
> - set_phi_nodes (new_merge_bb, phi_reverse (phi_nodes (new_merge_bb)));
> }

Where are we setting the PHI nodes on NEW_MERGE_BB now?


> - if (node && CAN_HAVE_LOCATION_P (node) && EXPR_HAS_LOCATION (node)
> - && EXPR_FILENAME (node) && EXPR_LINENO (node))
> - return EXPR_LOC (node);
> + /* FIXME tuples. The original code check for both location
> + and file name. */
> + if (stmt && gimple_location (stmt) != UNKNOWN_LOC)
> + return gimple_location (stmt);

No need for the FIXME note.  Your test is the correct one.

> +static hashval_t
> +hash_gimple_stmt (const void *p)
> +{
> + return (hashval_t) p;
> +}
> +
> +static int
> +eq_gimple_stmt (const void *p1, const void *p2)
> +{
> + return p1 == p2;
> +}
> +
> +void
> +init_stmt_vec_info_htab (void)
> +{
> + gcc_assert (!stmt_vec_info_htab);
> + stmt_vec_info_htab = htab_create (10, hash_gimple_stmt, eq_gimple_stmt,
> + NULL);
> +}
> +
> +void
> +free_stmt_vec_info_htab (void)
> +{
> + gcc_assert (stmt_vec_info_htab);
> + htab_delete (stmt_vec_info_htab);
> + stmt_vec_info_htab = NULL;
> +} 

Need comment on all these (even if it is just one comment for all of them).

> +/* Error reporting helper for vect_is_simple_reduction below. */
> +static void
> +report_vect_op (gimple stmt, const char *msg)

Blank linke after comment.  Need to document STMT and MSG.

> + switch (get_gimple_rhs_class (code))
> + {
> + case GIMPLE_SINGLE_RHS:
> + operation = gimple_assign_rhs1 (stmt);
> + break;
> + case GIMPLE_UNARY_RHS:
> + operation = build1 (code, TREE_TYPE (gimple_assign_lhs (stmt)),
> + gimple_assign_rhs1 (stmt));

I don't like this.  Building an expression tree just to display it is 
too wasteful.  Better use print_gimple_stmt here, or just print the 
operands separately (maybe by factoring code in gimple-pretty-print.c).

The rest looks fine.


Diego.