RE: [PATCH GCC]Catch more MEM_REFs sharing common addressing part in gimple strength reduction

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

On Mon, 2013-09-09 at 14:25 +0800, bin.cheng wrote:
> Thanks for reviewing, I will correct all stupid spelling problem in the next version of patch.
> 
> On Mon, Sep 9, 2013 at 8:15 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> >
> >>>+   int (i * S).
> >>>+   Otherwise, just return double int zero.  */
> >
> > This is sufficient, since you are properly checking the next_interp
> > chain.  Another possible form would be
> >
> >     X = (B + i) * 1,
> >
> > but if this is present, then one of the forms you're checking for should
> > also be present, so there's no need to check the MULT_CANDs.
> I'm not very sure here since I didn't check MULT_CAND in the patch.  Could you please explain more about this?

Sorry, perhaps I shouldn't have mentioned it.  I was simply stating
that, although a candidate representing B + i could be represented with
a CAND_MULT as shown, there is no need for you to check it (as you
don't) since there will also be a corresponding CAND_ADD in one of the
other forms.  Since you are walking the next_interp chain, this works.

In other words, the code is fine as is.  I was just thinking out loud
about other candidate types.

> 
> >
> >>>+
> >>>+static double_int
> >>>+backtrace_base_for_ref (tree *pbase)
> >>>+{
> >>>+  tree base_in = *pbase;
> >>>+  slsr_cand_t base_cand;
> >>>+
> >>>+  STRIP_NOPS (base_in);
> >>>+  if (TREE_CODE (base_in) != SSA_NAME)
> >>>+    return tree_to_double_int (integer_zero_node);
> >>>+
> >>>+  base_cand = base_cand_from_table (base_in);
> >>>+
> >>>+  while (base_cand && base_cand->kind != CAND_PHI)
> >>>+    {
> >>>+      if (base_cand->kind == CAND_ADD
> >>>+       && base_cand->index.is_one ()
> >>>+       && TREE_CODE (base_cand->stride) == INTEGER_CST)
> >>>+     {
> >>>+       /* X = B + (1 * S), S is integer constant.  */
> >>>+       *pbase = base_cand->base_expr;
> >>>+       return tree_to_double_int (base_cand->stride);
> >>>+     }
> >>>+      else if (base_cand->kind == CAND_ADD
> >>>+            && TREE_CODE (base_cand->stride) == INTEGER_CST
> >>>+            && integer_onep (base_cand->stride))
> >>>+        {
> >>>+       /* X = B + (i * S), S is integer one.  */
> >>>+       *pbase = base_cand->base_expr;
> >>>+       return base_cand->index;
> >>>+     }
> >>>+
> >>>+      if (base_cand->next_interp)
> >>>+     base_cand = lookup_cand (base_cand->next_interp);
> >>>+      else
> >>>+     base_cand = NULL;
> >>>+    }
> >>>+
> >>>+  return tree_to_double_int (integer_zero_node);
> >>>+}
> >>>+
> >>> /* Look for the following pattern:
> >>>
> >>>     *PBASE:    MEM_REF (T1, C1)
> >>>@@ -767,8 +818,15 @@ slsr_process_phi (gimple phi, bool speed)
> >>>
> >>>     *PBASE:    T1
> >>>     *POFFSET:  MULT_EXPR (T2, C3)
> >>>-    *PINDEX:   C1 + (C2 * C3) + C4  */
> >>>+    *PINDEX:   C1 + (C2 * C3) + C4
> >>>
> >>>+   When T2 is recorded by an CAND_ADD in the form of (T2' + C5), It
> >                              ^                                      ^
> >                              a                                      it
> >
> >>>+   will be further restructured to:
> >>>+
> >>>+    *PBASE:    T1
> >>>+    *POFFSET:  MULT_EXPR (T2', C3)
> >>>+    *PINDEX:   C1 + (C2 * C3) + C4 + (C5 * C3)  */
> >>>+
> >>> static bool
> >>> restructure_reference (tree *pbase, tree *poffset, double_int
> > *pindex,
> >>>                     tree *ptype)
> >>>@@ -777,7 +835,7 @@ restructure_reference (tree *pbase, tree *poffset,
> >>>   double_int index = *pindex;
> >>>   double_int bpu = double_int::from_uhwi (BITS_PER_UNIT);
> >>>   tree mult_op0, mult_op1, t1, t2, type;
> >>>-  double_int c1, c2, c3, c4;
> >>>+  double_int c1, c2, c3, c4, c5;
> >>>
> >>>   if (!base
> >>>       || !offset
> >>>@@ -823,11 +881,12 @@ restructure_reference (tree *pbase, tree
> > *poffset,
> >>>     }
> >>>
> >>>   c4 = index.udiv (bpu, FLOOR_DIV_EXPR);
> >>>+  c5 = backtrace_base_for_ref (&t2);
> >>>
> >>>   *pbase = t1;
> >>>-  *poffset = fold_build2 (MULT_EXPR, sizetype, t2,
> >>>-                       double_int_to_tree (sizetype, c3));
> >>>-  *pindex = c1 + c2 * c3 + c4;
> >>>+  *poffset = size_binop (MULT_EXPR, fold_convert (sizetype, t2),
> >>>+                      double_int_to_tree (sizetype, c3));
> >
> > I am not sure why you changed this call.  fold_build2 is a more
> > efficient call than size_binop.  size_binop makes several checks that
> > will fail in this case, and then calls fold_build2_loc, right?  Not a
> > big deal but seems like changing it back would be better.  Perhaps I'm
> > missing something (as usual ;).
> I rely on size_binop to convert T2 into sizetype, because T2' may be in other kind of type.  Otherwise there will be ssa_verify error later.

OK, I see now.  I had thought this was handled by fold_build2, but
apparently not.  I guess all T2's formerly handled were already sizetype
as expected.  Thanks for the explanation!

Bill

> 
> Thanks.
> bin
> 
> 
> 
>