Re: [patch] Fix PR/18179 and use get_inner_reference in vectorizer: part 2

[Ira Rosen <IRAR at il dot ibm dot com>]

Thank you very much for the review and explanations.


> > +   /* Stop conditions:
> > +      1. Constant.  */
> > +   if (host_integerp (expr, 1))
>
> You want TREE_CONSTANT, surely.

Fixed.

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

Done.

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

I changed it to
      if (evolution && TREE_CODE (evolution) != INTEGER_CST)
since I saw that evolution can be also REAL_CST.

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

I removed all the occurrences of host_integerp, I replaced them with
TREE_CONSTANT
or with TREE_CODE check as above. I removed checks for overflow, since we
just combine
different offsets/steps/misalignments, that I don't think can overflow.

> > +       /* 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 check that RIGHT_OFFSET is constant for now.

>
>
> r~

Thanks,
Ira

Patch 2 (new):
(See attached file: patch2New)

Diff between old patch 2 and the new one:
(See attached file: patch2.diff)

Attachment: patch2New
Description: Binary data

Attachment: patch2.diff
Description: Binary data