Re: [PING] SLSR for conditional candidates

[Richard Biener <rguenther at suse dot de>]

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'?

+  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.