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

[Ian Lance Taylor <iant at google dot com>]

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.

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.

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

Ian