On Tue, Jun 20, 2017 at 03:06:56PM +0200, Martin Liška wrote:
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> + that is it's DECL_VALUE_EXPR. */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> + if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)
DECL_VALUE_EXPR testing is costly (it is a hash table lookup).
Therefore you should test DECL_HAS_VALUE_EXPR_P (*op) after checking
== PARM_DECL. And DECL_HAS_VALUE_EXPR_P should apply non-NULL
DECL_VALUE_EXPR.
That said, I wonder if we don't create DECL_VALUE_EXPR for PARM_DECLs in
other parts of the compiler, whether it wouldn't be safer to also test here
after == PARM_DECL and DECL_HAS_VALUE_EXPR_P check whether *op is in
addressable_params hash table.
> + {
> + *op = DECL_VALUE_EXPR (*op);
> + *walk_subtrees = 0;
> + }
> +
> + return NULL;
> +}
> +
> +/* For a given function FUN, rewrite all addressable parameters so that
> + a new automatic variable is introduced. Right after function entry
> + a parameter is assigned to the variable. */
> +
> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> + gimple *g;
> + gimple_seq stmts = NULL;
> + auto_vec<tree> addressable_params;
You don't really use the addressable_params vector anywhere, right?
Except for:
> +
> + for (tree arg = DECL_ARGUMENTS (current_function_decl);
> + arg; arg = DECL_CHAIN (arg))
> + {
> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> + {
> + TREE_ADDRESSABLE (arg) = 0;
> + /* The parameter is no longer addressable. */
> + tree type = TREE_TYPE (arg);
> + addressable_params.safe_push (arg);
pushing stuff into it and later
> + if (addressable_params.is_empty ())
> + return;
If you only need that, a bool flag if any params have been changed is
enough. But see above whether it wouldn't be safer to use a hash table
to verify it. Plus, I think it would be desirable to clear
DECL_HAS_VALUE_EXPR_P and SET_DECL_VALUE_EXPR to NULL afterwards
if (target_for_debug_bind (arg)) - whch can be done either the with vec
or with a hash table traversal, for that we don't care about the ordering.
> +
> + /* Create a new automatic variable. */
> + tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> + VAR_DECL, DECL_NAME (arg), type);
> + TREE_ADDRESSABLE (var) = 1;
> + DECL_ARTIFICIAL (var) = 1;
> + DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
This is 0 already from build_decl, IMHO no need to set it.
> + gimple_add_tmp_var (var);
> +
> + if (dump_file)
> + fprintf (dump_file,
> + "Rewriting parameter whose address is taken: %s\n",
> + IDENTIFIER_POINTER (DECL_NAME (arg)));
> +
> + SET_DECL_VALUE_EXPR (arg, var);
But obviously you miss setting DECL_HAS_VALUE_EXPR_P here.
> + /* Assign value of parameter to newly created variable. */
> + if ((TREE_CODE (type) == COMPLEX_TYPE
> + || TREE_CODE (type) == VECTOR_TYPE))
> + {
> + /* We need to create a SSA name that will be used for the
> + assignment. */
Why don't you just set DECL_GIMPLE_REG_P (arg) = 1; for
COMPLEX_TYPE/VECTOR_TYPE? The arg is going to be only used to copy it into
the new var. And then just use get_or_create_ssa_default_def,
regardless of whether if is complex/vector or other.
> + /* Replace all usages of PARM_DECLs with the newly
> + created variable VAR. */
> + basic_block bb;
> + FOR_EACH_BB_FN (bb, fun)
> + {
> + gimple_stmt_iterator gsi;
> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + gimple *stmt = gsi_stmt (gsi);
> + gimple_stmt_iterator it = gsi_for_stmt (stmt);
> + walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
> + }
> + for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
> + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
> + {
> + hash_set<tree> visited_nodes;
> + walk_tree (gimple_phi_arg_def_ptr (phi, i),
> + rewrite_usage_of_param, NULL, &visited_nodes);
> + }
Doesn't walk_gimple_stmt on the PHI handle this?
Jakub
