On Thu, 2013-05-02 at 13:29 +0200, Richard Biener wrote:
> On Mon, 29 Apr 2013, Bill Schmidt wrote:
>
> > Half-hearted ping for
> > http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01291.html ...
> >
> > I promise this is the last major code dump for SLSR. ;)
>
>
>
> + if (!operand_equal_p (arg_cand->stride, integer_one_node, 0))
> + return;
>
> !integer_onep (arg_cand->stride);
>
> + arg_stmt = SSA_NAME_DEF_STMT (arg);
> +
> + if (arg_stmt
> + && gimple_code (arg_stmt) != GIMPLE_NOP)
> + arg_bb = gimple_bb (arg_stmt);
> + else
> + arg_bb = ENTRY_BLOCK_PTR->next_bb;
>
> if (SSA_NAME_IS_DEFAULT_DEF (arg))
> arg_bb = single_succ (ENTRY_BLOCK_PTR);
> else
> arg_bb = gimple_bb (SSA_NAME_DEF_STMT (arg));
>
> don't use ->next_bb - the ordering of BBs is arbitrary. No idea
> why you needed to look for !arg_stmt.
>
> + /* If the basis name and the candidate's LHS have incompatible
> + types, introduce a cast. */
> + if (!types_compatible_p (target_type,
> + TREE_TYPE (basis_name)))
> + basis_name = introduce_cast_before_cand (c, target_type,
> + basis_name, var);
>
> use !useless_type_conversion_p (target_type, TREE_TYPE (basis_name))
> if you are looking whether the op can be used where target_type is
> requested (as opposed to where it can be assigned _from_ a target_type
> thing).
>
> + tree rhs1 = gimple_assign_rhs1 (c->cand_stmt);
> + tree rhs2 = gimple_assign_rhs2 (c->cand_stmt);
> + if (cand_code != NEGATE_EXPR
>
> are you sure you are not accessing out-of-bounds for the
> single-operand NEGATE_EXPR case?
>
> +/* Return the index in the increment vector of the given INCREMENT. */
> +
> +static inline unsigned
> +incr_vec_index (double_int increment)
> +{
> + unsigned i;
> +
> + for (i = 0; i < incr_vec_len && increment != incr_vec[i].incr; i++)
> + ;
>
> I smell quadratic complexity here. Is the incr_vec maybe better
> sorted by 'incr'?
Good point. We already have the potential for a problem here (in
record_increment, a linear search is used to update the count for the
increment). I was presuming here that the number of increments for a
candidate group would be a small finite number, which is generally true,
though I suspect a pathological case could be built. Realistic?
Doubtful, but we should plan for the worst.
I can fix this, but I think I should do it as a separate patch prior to
committing this one, then replace this code with a binary search when
the size of incr_vec is larger than a small constant.
I agree with all your other comments. Thanks for the review!
Bill
>
> + basis_type = TREE_TYPE (basis_name);
> + lazy_create_slsr_reg (var, basis_type);
> + lhs = make_ssa_name (*var, NULL);
>
> btw, lazy_create_slsr_reg should go away now as we have
> the notion of "anonymous" SSA names (SSA names without underlying
> VAR_DECLs). Just do
>
> lhs = make_ssa_name (basis_type, NULL);
>
> here. You can followup with a patch to remove lazy_create_slsr_reg.
> Another possibility is to use make_temp_ssa_name which has an
> additional char *name argument:
>
> lhs = make_temp_ssa_name (basis_type, NULL, "slsr");
>
> which then makes the SSA names show as slsr_N in dumps.
>
> + int cost = 0;
> + slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi));
>
> - gcc_assert (i < incr_vec_len);
> - return i;
> + for (i = 0; i < gimple_phi_num_args (phi); i++)
> + {
> + tree arg = gimple_phi_arg_def (phi, i);
> +
> + if (!operand_equal_p (arg, phi_cand->base_expr, 0))
>
> generally SSA names can be compared with == / != directly.
> PHI results are always SSA names so no need to dispatch
> to operand_equal_p here.
>
> Otherwise the patch looks ok.
>
> Thanks,
> Richard.
>
