Re: [PATCH] PR29145 - be more restrict about restrict qualifier

[Dorit Nuzman <DORIT at il dot ibm dot com>]

Ian Lance Taylor <iant@google.com> wrote on 15/01/2007 22:12:24:

> Dorit Nuzman <DORIT@il.ibm.com> writes:
>
> > Ian Lance Taylor <iant@google.com> wrote on 09/01/2007 16:09:45:
> > > Dorit Nuzman <DORIT@il.ibm.com> writes:
> > >
> > > >    /* An instruction writing through a restricted pointer is
> > > "independent" of any
> > > > -     instruction reading or writing through a different pointer,
> > > in the same
> > > > -     block/scope.  */
> > > > -  else if ((TYPE_RESTRICT (type_a) && !DR_IS_READ (dra))
> > > > -      || (TYPE_RESTRICT (type_b) && !DR_IS_READ (drb)))
> > > > +     instruction reading or writing through a different
> > > restricted pointer,
> > > > +     in the same block/scope.  */
> > > > +  else if (TYPE_RESTRICT (type_a)
> > > > +      &&  TYPE_RESTRICT (type_b)
> > > > +      && (!DR_IS_READ (drb) || !DR_IS_READ (dra))
> > > > +      && TREE_CODE (DR_BASE_ADDRESS (dra)) == SSA_NAME
> > > > +      && (decl_a = SSA_NAME_VAR (DR_BASE_ADDRESS (dra)))
> > > > +      && TREE_CODE (decl_a) == PARM_DECL
> > > > +      && TREE_CODE (DECL_CONTEXT (decl_a)) == FUNCTION_DECL
> > > > +      && TREE_CODE (DR_BASE_ADDRESS (drb)) == SSA_NAME
> > > > +      && (decl_b = SSA_NAME_VAR (DR_BASE_ADDRESS (drb)))
> > > > +      && TREE_CODE (decl_b) == PARM_DECL
> > > > +      && TREE_CODE (DECL_CONTEXT (decl_b)) == FUNCTION_DECL
> > > > +      && DECL_CONTEXT (decl_a) == DECL_CONTEXT (decl_b))
> > >
> > > You should probably be using DECL_BASED_ON_RESTRICT_P and
> > > DECL_GET_RESTRICT_BASE here.  See find_base_decl in alias.c.
> > >
> >
> > so I experimented with adding something like this to the above if-stmt:
> >             if (TREE_CODE (decl_a) == VAR_DECL
> >                 && DECL_BASED_ON_RESTRICT_P (decl_a)
> >                 && (decl_a = DECL_GET_RESTRICT_BASE (decl_a))
> >                   ....
> > but it didn't make a differnce - at least for the vectorizer testcases
-
> > DECL_BASED_ON_RESTRICT_P is never true... I'd rather add such code only
if
> > I can also add a testcase for which it makes a difference.
>
> In my experience DECL_BASED_ON_RESTRICT_P matters because
> gimplification creates temporaries based on the restrict qualified
> parameters.  Perhaps that is not happening in your test case for some
> reason.
>
> In particular, the bit will be set for decls returned by
> get_formal_tmp_var when those variables are based on an expression
> which is based on a restrict qualifed pointer.  The expression may be
> trivial, i.e., a direct reference to the pointer.
>

that's what I have in my testcase. e.g. vect-80.c:

int
main1 (float * __restrict__ pa, float * __restrict__ pb, float
*__restrict__ pc)
{
  int i;
  float *q = pb + 4;

  for (i = 0; i < N; i++)
    {
      pa[i] = q[i] * pc[i];
    }

  ...

  return 0;
}

I'd expect q to be marked as DECL_BASED_ON_RESTRICT_P, but it didn't seem
to happen.


> The upshot is that if you don't use DECL_BASED_ON_RESTRICT_P, then you
> will miss some cases which should be caught, merely because the
> restrict qualifier will be attached to the real variable but not to
> the temporary variable.
>

Understood. I certainly agree that it would be desired to use
DECL_BASED_ON_RESTRICT_P (especially if it can be shown to work on a
certain testcase), but, at least for the record - even without it this
patch is correct, although too conservative.

> > > In fact, come to think of it, you could probably just call
> > > get_alias_set for both types.  That should handle restrict qualified
> > > pointers correctly.
> > >
> >
> > I also experimented with adding the following (after the call to
> > may_alias_p):
> >
> > +
> > +  else if (!alias_sets_might_conflict_p (get_alias_set (type_a),
> > get_alias_set (type_b)))
> > +    {
> > +      *differ_p = true;
> > +      return true;
> > +    }
> > +
> >
> > I don't know if this is what you meant, or if it is safe; In any case:
> > 1) it doesn't handle restrict. That's the whole point - the alias
analysis
> > can't handle restrict at the moment, which is why we had to resort to
these
> > extra checks in tree-data-ref.c
>
> Alias sets do handle restrict, though; see get_alias_set in alias.c.
> I believe you are correct that tree level alias analysis does not
> currently use restrict, but it does crop up at the level of alias
> sets.
>
> > 2) as a result 3 testcases (that don't use the restrict qualifier at
all)
> > started to get vectorized - vect[43,49,53].c.  Note that a preceding
call
> > to may_alias_p was not able to conclude that the accesses in question
don't
> > alias, so I'm not sure if the above check is correct. e.g., in
vect-43.c,
> > the alias analysis says that:
> >       pa_7(D), its value escapes, points-to anything
> > where's the above check is able to resolve the aliasing problem...
>
> You shouldn't call alias_sets_might_conflict_p; you should call
> alias_sets_conflict_p.  (Frankly I don't understand the naming
> convention here.)
>

I tried that - it gave the same outcome - testcases vect-44.c, vect-50.c,
vect-95.c and pr16105.c (all using restrict pointers) do not get vectorized
if I drop the special handling of restrict and add a call to
alias_sets_conflict_p instead.  On the other hand - testcases
vect-[43,49,53].c do get vectorized although there's no restrict there and
the pointers actually escape.

thanks,
dorit

> Ian