This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Fix PR/18179 and use get_inner_reference in vectorizer: part 2
- From: Ira Rosen <IRAR at il dot ibm dot com>
- To: Richard Henderson <rth at redhat dot com>
- Cc: Dorit Naishlos <DORIT at il dot ibm dot com>, gcc-patches at gcc dot gnu dot org
- Date: Thu, 16 Dec 2004 10:50:48 +0200
- Subject: Re: [patch] Fix PR/18179 and use get_inner_reference in vectorizer: part 2
Richard Henderson <rth@redhat.com> wrote on 16/12/2004 10:19:13:
> On Tue, Dec 14, 2004 at 08:54:48AM +0200, Ira Rosen wrote:
> > + /* Stop conditions:
> > + 1. Constant. */
> > + if (host_integerp (expr, 1))
>
> You want TREE_CONSTANT, surely.
O.K.
>
> > + tree access_fn = analyze_scalar_evolution (loop, expr);
> > + if (access_fn == chrec_dont_know /* No access_fn. */
> > +
> > + /* Not enough information: may be not loop invariant.
> > + E.g., for a[b[i]], we get a[D], where D=b[i]. EXPR is D, its
> > + initial_condition is D, but it depends on i - loop's
induction
> > + variable. */
> > + || (((init = initial_condition_in_loop_num (access_fn,
loop->num))
> > + == expr)
> > + && !IS_EMPTY_STMT (def_stmt = SSA_NAME_DEF_STMT (init))
> > + && flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt)))
> > +
> > + /* Evolution is not constant. */
> > + || ((evolution = evolution_part_in_loop_num (access_fn,
loop->num))
> > + && !host_integerp (evolution, 0)))
> > + {
> > + *step = NULL_TREE;
> > + *misalign = NULL_TREE;
> > + *initial_offset = NULL_TREE;
> > + return false;
> > + }
>
> Please break up this conditional. Like so.
O.K.
>
> *step = NULL_TREE;
> *misalign = NULL_TREE;
> *initial_offset = NULL_TREE;
>
> Move these to the beginning of the function (at least before the
> SSA_NAME conditional, and then the various failure paths out of
> the function only need to "return false".
>
O.K.
> if (access_fn == chrec_dont_know)
> return false;
>
> init = initial_condition_in_loop_num (access_fn, loop->num);
> if (init != expr)
> {
> def_stmt = SSA_NAME_DEF_STMT (init);
> if (!IS_EMPTY_STMT (def_stmt)
> && flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt)))
> return false;
> }
>
> evolution = evolution_part_in_loop_num (access_fn, loop->num);
> if (evolution && !host_integerp (evolution, 0))
> return false;
>
> Resist the temptation to embed assignments inside conditionals.
> Three is really just over-the-top too much.
>
> Do you actually want TREE_CONSTANT there at the end too? Not
> having looked at what the interface of evolution_part_in_loop_num
> is, I'm not sure.
Evolution part is not necessary a constant.
>
> For the rest, let's just take as written that I'd like you to
> re-examine all other ocurrences of host_integerp and defend them.
O.K.
>
> > + /* Assumption: in MULT_EXPR the left operand is a variable
> and the right
> > + operand is a constant. Therefore, LEFT_OFFSET can be either a
variable
> > + or a constant, and RIGHT_OFFSET is always a constant. */
>
> On what do you base this assumption? Arrays of variable sized types
> will have no constant operand. Has something above this point already
> discarded this as a possibility?
I'll check that the right operand is a constant.
>
>
>
> r~
O.K. with these changes?
Ira