On Mon, 2013-09-09 at 10:20 -0500, Bill Schmidt wrote:
> 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 <wschm...@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!
So, wouldn't it suffice to change t2 to fold_convert (sizetype, t2) in
the argument list to fold_build2? It's picking nits, but that would be
slightly more efficient.
Bill
>
> Bill
>
> >
> > Thanks.
> > bin
> >
> >
> >
> >
