Re: [PATCH PR69489/01]Improve tree ifcvt by storing/tracking DR against its innermost loop bahavior if possible

["Bin.Cheng" <amker.cheng@gmail.com>]

On Wed, Mar 16, 2016 at 12:20 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, Mar 16, 2016 at 10:59 AM, Bin Cheng <Bin.Cheng@arm.com> wrote:
> > Hi,
> > ......
> > Bootstrap and test on x86_64 and AArch64.  Is it OK, not sure if it's GCC 7?
>
> Hmm.
Hi,
Thanks for reviewing.
>
> +  equal_p = true;
> +  if (e1->base_address && e2->base_address)
> +    equal_p &= operand_equal_p (e1->base_address, e2->base_address, 0);
> +  if (e1->offset && e2->offset)
> +    equal_p &= operand_equal_p (e1->offset, e2->offset, 0);
>
> surely better to return false early.
>
> I think we don't want this in tree-data-refs.h also because of ...
>
> @@ -615,15 +619,29 @@
> hash_memrefs_baserefs_and_store_DRs_read_written_info
> (data_reference_p a)
>    data_reference_p *master_dr, *base_master_dr;and REALPART) before creating the DR (or adjust the equality function
and hashing
>    tree ref = DR_REF (a);
>    tree base_ref = DR_BASE_OBJECT (a);
> +  innermost_loop_behavior *innermost = &DR_INNERMOST (a);
>    tree ca = bb_predicate (gimple_bb (DR_STMT (a)));
>    bool exist1, exist2;
>
> -  while (TREE_CODE (ref) == COMPONENT_REF
> -        || TREE_CODE (ref) == IMAGPART_EXPR
> -        || TREE_CODE (ref) == REALPART_EXPR)
> -    ref = TREE_OPERAND (ref, 0);
> +  /* If reference in DR has innermost loop behavior and it is not
> +     a compound memory reference, we store it to innermost_DR_map,
> +     otherwise to ref_DR_map.  */
> +  if (TREE_CODE (ref) == COMPONENT_REF
> +      || TREE_CODE (ref) == IMAGPART_EXPR
> +      || TREE_CODE (ref) == REALPART_EXPR
> +      || !(DR_BASE_ADDRESS (a) || DR_OFFSET (a)
> +          || DR_INIT (a) || DR_STEP (a) || DR_ALIGNED_TO (a)))
> +    {
> +      while (TREE_CODE (ref) == COMPONENT_REF
> +            || TREE_CODE (ref) == IMAGPART_EXPR
> +            || TREE_CODE (ref) == REALPART_EXPR)
> +       ref = TREE_OPERAND (ref, 0);
> +
> +      master_dr = &ref_DR_map->get_or_insert (ref, &exist1);
> +    }
> +  else
> +    master_dr = &innermost_DR_map->get_or_insert (innermost, &exist1);
>
> we don't want an extra hashmap but replace ref_DR_map entirely.  So we'd need to
> strip outermost non-variant handled-components (COMPONENT_REF, IMAGPART
> and REALPART) before creating the DR (or adjust the equality function
> and hashing
> to disregard them which means subtracting their offset from DR_INIT.
I am not sure if I understand correctly.  But for component reference,
it is the base object that we want to record/track.  For example,

  for (i = 0; i < N; i++) {
    m = *data++;

    m1 = p1->x - m;
    m2 = p2->x + m;

    p3->y = (m1 >= m2) ? p1->y : p2->y;

    p1++;
    p2++;
    p3++;
  }
We want to infer that reads of p1/p2 in condition statement won't trap
because there are unconditional reads of the structures, though the
unconditional reads are actual of other sub-objects.  Here it is the
invariant part of address that we want to track.
Also illustrated by this example, we can't rely on data-ref analyzer
here.  Because in gathering/scattering cases, the address could be not
affine at all.
>
> To adjust the references we collect you'd maybe could use a callback
> to get_references_in_stmt
> to adjust them.
>
> OTOH post-processing the DRs in if_convertible_loop_p_1 can be as simple as
Is this a part of the method you suggested above, or is it an
alternative one?  If it's the latter, then I have below questions
embedded.
>
> Index: tree-if-conv.c
> ===================================================================
> --- tree-if-conv.c      (revision 234215)
> +++ tree-if-conv.c      (working copy)
> @@ -1235,6 +1220,38 @@ if_convertible_loop_p_1 (struct loop *lo
>
>    for (i = 0; refs->iterate (i, &dr); i++)
>      {
> +      tree *refp = &DR_REF (dr);
> +      while ((TREE_CODE (*refp) == COMPONENT_REF
> +             && TREE_OPERAND (*refp, 2) == NULL_TREE)
> +            || TREE_CODE (*refp) == IMAGPART_EXPR
> +            || TREE_CODE (*refp) == REALPART_EXPR)
> +       refp = &TREE_OPERAND (*refp, 0);
> +      if (refp != &DR_REF (dr))
> +       {
> +         tree saved_base = *refp;
> +         *refp = integer_zero_node;
> +
> +         if (DR_INIT (dr))
> +           {
> +             tree poffset;
> +             int punsignedp, preversep, pvolatilep;
> +             machine_mode pmode;
> +             HOST_WIDE_INT pbitsize, pbitpos;
> +             get_inner_reference (DR_REF (dr), &pbitsize, &pbitpos, &poffset,
> +                                  &pmode, &punsignedp, &preversep, &pvolatilep,
> +                                  false);
> +             gcc_assert (poffset == NULL_TREE);
> +
> +             DR_INIT (dr)
> +               = wide_int_to_tree (ssizetype,
> +                                   wi::sub (DR_INIT (dr),
> +                                            pbitpos / BITS_PER_UNIT));
> +           }
> +
> +         *refp = saved_base;
> +         DR_REF (dr) = *refp;
> +       }
Looks to me the code is trying to resolve difference between two (or
more) component references, which is DR_INIT in the code.  But DR_INIT
is not the only thing needs to be handled.  For a structure containing
two sub-arrays, DR_OFFSET may be different too.
Actually I did try to replace ref_DR_map.  For memory reference
doesn't have innermost affine behavior, we need to modify DR fields by
populating it with artificial data.  This results in some kind of
over-designed code.  IMHO, modifying some DR structures outside of
data-ref analyzer isn't that good.  After comparing the two methods, I
think may be this one is better, of course with the cost of two
different maps.

I might be misunderstanding your idea, so please correct if I was wrong.

Thanks,
bin
> +
>        dr->aux = XNEW (struct ifc_dr);
>        DR_BASE_W_UNCONDITIONALLY (dr) = false;
>        DR_RW_UNCONDITIONALLY (dr) = false;
>
>
> Can you add a testcase for the vectorization?
>
> Richard.
>