On Thu, 2 May 2013, Bill Schmidt wrote:
> 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.
Or as a followup - the code is already there. Another possibility
is to limit the number of constant increments you track (basically
give up and do not track further ones), that makes the operation
O(1).
Richard.
> 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.
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
