[gomp4.1] C++ iterators with #omp ordered depend(sink:)

[Jakub Jelinek]

Hi!

CCing Jason on a tsubst issue below.

On Wed, Jul 15, 2015 at 06:05:06PM -0700, Aldy Hernandez wrote:
> As I said on IRC, this looks ugly, but I can see why you wouldn't want the
> extra word on all loop variants.  I've implemented it as requested.

Thanks.

> commit f55eced4ac6b045101a90914a8f27e99d26cfddf
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Tue Jul 14 19:23:09 2015 -0700
> 
>     	* gimplify.c (gimplify_omp_for): Use OMP_FOR_ORIG_DECLS.
>     	* tree.def (omp_for): Add new operand.
>     	* tree.h (OMP_FOR_ORIG_DECLS): New macro.
>     c-family/
>     	* c-common.h (c_finish_omp_for): Add argument.
>     	* c-omp.c (c_finish_omp_for): Set OMP_FOR_ORIG_DECLS.
>     cp/
>     	* cp-tree.h (finish_omp_for): Add new argument.
>     	* parser.c (cp_parser_omp_for_loop): Pass new argument.
>     	* pt.c (tsubst_omp_for_iterator): New argument orig_declv.
>     	Set OMP_FOR_ORIG_DECLS from orig_declv if available.
>     	(tsubst_expr): Pass new vector to tsubst_omp_for_iterator.
>     	* semantics.c (finish_omp_for): Pass original DECLs to
>     	c_finish_omp_for.
>     	Set OMP_FOR_ORIG_DECLS.
>     c/
>     	* c-parser.c (c_parser_omp_for_loop): Pass new argument to
>     	c_finish_omp_for.

Can you please add the 
	testsuite/
additions to ChangeLog too?

> @@ -13828,6 +13828,16 @@ tsubst_omp_for_iterator (tree t, int i, tree declv, tree initv,
>  
>    init = TREE_VEC_ELT (OMP_FOR_INIT (t), i);
>    gcc_assert (TREE_CODE (init) == MODIFY_EXPR);
> +
> +  if (orig_declv && OMP_FOR_ORIG_DECLS (t))
> +    {
> +      tree o = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (t), i);
> +      tree spec = retrieve_local_specialization (o);
> +      if (spec)
> +	o = spec;

Why doesn't o = RECUR (o); work here?  Or tsubst_copy?
>From my experience tsubst_expr can result in (here undesirable) convert_from_reference
if the decl has type of e.g. template parameter and you instantiate with
some reference type.  But, that can only happen if the decl is dependent,
see below:

> @@ -14468,6 +14479,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
>  	if (OMP_FOR_INIT (t) != NULL_TREE)
>  	  {
>  	    declv = make_tree_vec (TREE_VEC_LENGTH (OMP_FOR_INIT (t)));
> +	    if (TREE_CODE (t) == OMP_FOR)
> +	      orig_declv = make_tree_vec (TREE_VEC_LENGTH (OMP_FOR_INIT (t)));

I'd have expected to guard this also with
	if (TREE_CODE (t) == OMP_FOR && OMP_FOR_ORIG_DECLS (t) != NULL_TREE)

> @@ -7302,6 +7303,9 @@ finish_omp_for (location_t locus, enum tree_code code, tree declv, tree initv,
>        TREE_VEC_ELT (initv, i) = init;
>      }
>  
> +  if (!orig_declv || !TREE_VEC_LENGTH (orig_declv))
> +    orig_declv = copy_node (declv);

When is TREE_VEC_LENGTH 0?  Furthermore, I'd do this only after the
if (dependent_omp_for_p ())

> +
>    if (dependent_omp_for_p (declv, initv, condv, incrv))
>      {
>        tree stmt;
> @@ -7325,6 +7329,8 @@ finish_omp_for (location_t locus, enum tree_code code, tree declv, tree initv,
>        OMP_FOR_BODY (stmt) = body;
>        OMP_FOR_PRE_BODY (stmt) = pre_body;
>        OMP_FOR_CLAUSES (stmt) = clauses;
> +      if (code == OMP_FOR)
> +	OMP_FOR_ORIG_DECLS (stmt) = orig_declv;

And leave this out.  If something is dependent, we don't really modify it
before instantiation, thus there is no need to make a copy.  During
instantiation another finish_omp_for will be called, then declv, initv,
condv, incrv won't be dependent and we can orig_declv = copy_node (declv).

> diff --git a/gcc/testsuite/c-c++-common/gomp/sink-2.c b/gcc/testsuite/c-c++-common/gomp/sink-2.c
> new file mode 100644
> index 0000000..7a075d4
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/sink-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +
> +void bar (int *);
> +
> +void
> +foo ()
> +{
> +  int i,j;
> +#pragma omp parallel for ordered(1)
> +  for (i=0; i < 100; ++i)
> +    {
> +#pragma omp ordered depend(sink:i-1)
> +    bar(&i);

Can you please add #pragma omp ordered depend(source) below the bar call?

	Jakub