[PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jakub Jelinek]

Hi!

The PR84469 patch I've just posted regresses the for-21.C testcase,
when in OpenMP loop there are at least 2 associated loops and
in a template outer structured binding with non type dependent expression
is used in the expressions of some inner loop, we don't diagnose those
any longer, as the (weirdly worded) diagnostics was only done during
finish_id_expression -> mark_used which for the inner loop expressions
happens before the structured bindings are finalized.  When in templates,
mark_used doesn't diagnose uses of non-deduced variables, and if the
range for expression is type dependent, it is similarly diagnosed during
instantiation.  But newly with the PR84469 fix if the range for expression
is not type dependent, there is no place that would diagnose it, as during
instantiation the structured bindings are already deduced.

The following patch diagnoses it in that case during finish_omp_for (for
consistency with the same weird message).

I'll commit this to trunk if the other patch is approved and it passes
bootstrap/regtest.

2022-11-29  Jakub Jelinek  <jakub@redhat.com>

	PR c++/84469
	* semantics.cc: Define INCLUDE_MEMORY before including system.h.
	(struct finish_omp_for_data): New type.
	(finish_omp_for_decomps_r): New function.
	(finish_omp_for): Diagnose uses of non-type-dependent range for
	loop decompositions in inner OpenMP associated loops in templates.

	* g++.dg/gomp/for-21.C (f6): Adjust lines of expected diagnostics.
	* g++.dg/gomp/for-22.C: New test.

--- gcc/cp/semantics.cc.jj	2022-11-19 09:21:14.897436616 +0100
+++ gcc/cp/semantics.cc	2022-11-29 12:58:36.165771985 +0100
@@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.
 <http://www.gnu.org/licenses/>.  */
 
 #include "config.h"
+#define INCLUDE_MEMORY
 #include "system.h"
 #include "coretypes.h"
 #include "target.h"
@@ -10401,6 +10402,47 @@ handle_omp_for_class_iterator (int i, lo
   return false;
 }
 
+struct finish_omp_for_data {
+  std::unique_ptr<hash_set<tree>> decomps;
+  bool fail;
+  location_t loc;
+};
+
+/* Helper function for finish_omp_for.  Diagnose uses of structured
+   bindings of OpenMP collapsed loop range for loops in the associated
+   loops.  If not processing_template_decl, this is diagnosed by
+   finish_id_expression -> mark_used before the range for is deduced.
+   And if processing_template_decl and the range for expression is
+   type dependent, it is similarly diagnosed during instantiation.
+   Only when processing_template_decl and range for expression is
+   not type dependent, we wouldn't diagnose it at all, so do it
+   from finish_omp_for in that case.  */
+
+static tree
+finish_omp_for_decomps_r (tree *tp, int *, void *d)
+{
+  if (VAR_P (*tp)
+      && DECL_DECOMPOSITION_P (*tp)
+      && !type_dependent_expression_p (*tp)
+      && DECL_HAS_VALUE_EXPR_P (*tp))
+    {
+      tree v = DECL_VALUE_EXPR (*tp);
+      if (TREE_CODE (v) == ARRAY_REF
+	  && VAR_P (TREE_OPERAND (v, 0))
+	  && DECL_DECOMPOSITION_P (TREE_OPERAND (v, 0)))
+	{
+	  finish_omp_for_data *data = (finish_omp_for_data *) d;
+	  if (data->decomps->contains (TREE_OPERAND (v, 0)))
+	    {
+	      error_at (data->loc, "use of %qD before deduction of %<auto%>",
+			*tp);
+	      data->fail = true;
+	    }
+	}
+    }
+  return NULL_TREE;
+}
+
 /* Build and validate an OMP_FOR statement.  CLAUSES, BODY, COND, INCR
    are directly for their associated operands in the statement.  DECL
    and INIT are a combo; if DECL is NULL then INIT ought to be a
@@ -10419,6 +10461,7 @@ finish_omp_for (location_t locus, enum t
   int i;
   int collapse = 1;
   int ordered = 0;
+  finish_omp_for_data data;
 
   gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (initv));
   gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (condv));
@@ -10479,7 +10522,25 @@ finish_omp_for (location_t locus, enum t
 	elocus = EXPR_LOCATION (init);
 
       if (cond == global_namespace)
-	continue;
+	{
+	  gcc_assert (processing_template_decl);
+	  if (TREE_VEC_LENGTH (declv) > 1
+	      && VAR_P (decl)
+	      && DECL_DECOMPOSITION_P (decl)
+	      && !type_dependent_expression_p (decl))
+	    {
+	      gcc_assert (DECL_HAS_VALUE_EXPR_P (decl));
+	      tree v = DECL_VALUE_EXPR (decl);
+	      gcc_assert (TREE_CODE (v) == ARRAY_REF
+			  && VAR_P (TREE_OPERAND (v, 0))
+			  && DECL_DECOMPOSITION_P (TREE_OPERAND (v, 0)));
+	      if (!data.decomps)
+		data.decomps
+		  = std::unique_ptr<hash_set<tree>> (new hash_set<tree>);
+	      data.decomps->add (TREE_OPERAND (v, 0));
+	    }
+	  continue;
+	}
 
       if (cond == NULL)
 	{
@@ -10497,6 +10558,37 @@ finish_omp_for (location_t locus, enum t
       TREE_VEC_ELT (initv, i) = init;
     }
 
+  if (data.decomps)
+    {
+      data.fail = false;
+      data.loc = locus;
+      hash_set<tree> pset;
+      for (i = 0; i < TREE_VEC_LENGTH (declv); i++)
+	{
+	  init = TREE_VEC_ELT (initv, i);
+	  cond = TREE_VEC_ELT (condv, i);
+	  incr = TREE_VEC_ELT (incrv, i);
+	  data.loc = EXPR_LOC_OR_LOC (init, locus);
+	  cp_walk_tree (&init, finish_omp_for_decomps_r, &data, &pset);
+	  data.loc = EXPR_LOC_OR_LOC (cond, locus);
+	  cp_walk_tree (&cond, finish_omp_for_decomps_r, &data, &pset);
+	  data.loc = EXPR_LOC_OR_LOC (incr, locus);
+	  cp_walk_tree (&incr, finish_omp_for_decomps_r, &data, &pset);
+	}
+      if (orig_inits && !data.fail)
+	{
+	  tree orig_init;
+	  FOR_EACH_VEC_ELT (*orig_inits, i, orig_init)
+	    {
+	      data.loc = EXPR_LOC_OR_LOC (orig_init, locus);
+	      cp_walk_tree (&orig_init, finish_omp_for_decomps_r,
+			    &data, &pset);
+	    }
+	}
+      if (data.fail)
+	return NULL;
+    }
+
   if (orig_inits)
     {
       bool fail = false;
--- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2020-01-12 11:54:37.178401867 +0100
+++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-11-29 13:06:59.038410557 +0100
@@ -54,9 +54,9 @@ void
 f6 (S (&a)[10])
 {
   #pragma omp for collapse (2)
-  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
+  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" }
     for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
-      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
+      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
 }
 
 template <typename T>
--- gcc/testsuite/g++.dg/gomp/for-22.C.jj	2022-11-29 13:07:10.859237506 +0100
+++ gcc/testsuite/g++.dg/gomp/for-22.C	2022-11-29 13:09:50.743897003 +0100
@@ -0,0 +1,57 @@
+// { dg-do compile { target c++17 } }
+
+namespace std {
+  template<typename T> struct tuple_size;
+  template<int, typename> struct tuple_element;
+}
+
+struct A {
+  int i;
+  template <int I> int& get() { return i; }
+};
+
+template<> struct std::tuple_size<A> { static const int value = 3; };
+template<int I> struct std::tuple_element<I,A> { using type = int; };
+
+struct B {
+  A *begin();
+  A *end();
+};
+
+void
+f1 (B a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
+    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
+      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
+}
+
+template <int N>
+void
+f2 (B a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
+    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
+      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
+}
+
+template <typename T>
+void
+f3 (T a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
+    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
+      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
+}
+
+void
+test ()
+{
+  B b;
+  f1 (b);
+  f2 <0> (b);
+  f3 <B> (b);
+}

	Jakub

Re: [PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jason Merrill]

On 11/29/22 07:32, Jakub Jelinek wrote:
> Hi!
> 
> The PR84469 patch I've just posted regresses the for-21.C testcase,
> when in OpenMP loop there are at least 2 associated loops and
> in a template outer structured binding with non type dependent expression
> is used in the expressions of some inner loop, we don't diagnose those
> any longer, as the (weirdly worded) diagnostics was only done during
> finish_id_expression -> mark_used which for the inner loop expressions
> happens before the structured bindings are finalized.  When in templates,
> mark_used doesn't diagnose uses of non-deduced variables, and if the
> range for expression is type dependent, it is similarly diagnosed during
> instantiation.  But newly with the PR84469 fix if the range for expression
> is not type dependent, there is no place that would diagnose it, as during
> instantiation the structured bindings are already deduced.
> 
> The following patch diagnoses it in that case during finish_omp_for (for
> consistency with the same weird message).
> 
> I'll commit this to trunk if the other patch is approved and it passes
> bootstrap/regtest.
> 
> 2022-11-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/84469
> 	* semantics.cc: Define INCLUDE_MEMORY before including system.h.
> 	(struct finish_omp_for_data): New type.
> 	(finish_omp_for_decomps_r): New function.
> 	(finish_omp_for): Diagnose uses of non-type-dependent range for
> 	loop decompositions in inner OpenMP associated loops in templates.
> 
> 	* g++.dg/gomp/for-21.C (f6): Adjust lines of expected diagnostics.
> 	* g++.dg/gomp/for-22.C: New test.
> 
> --- gcc/cp/semantics.cc.jj	2022-11-19 09:21:14.897436616 +0100
> +++ gcc/cp/semantics.cc	2022-11-29 12:58:36.165771985 +0100
> @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.
>   <http://www.gnu.org/licenses/>.  */
>   
>   #include "config.h"
> +#define INCLUDE_MEMORY
>   #include "system.h"
>   #include "coretypes.h"
>   #include "target.h"
> @@ -10401,6 +10402,47 @@ handle_omp_for_class_iterator (int i, lo
>     return false;
>   }
>   
> +struct finish_omp_for_data {
> +  std::unique_ptr<hash_set<tree>> decomps;
> +  bool fail;
> +  location_t loc;
> +};
> +
> +/* Helper function for finish_omp_for.  Diagnose uses of structured
> +   bindings of OpenMP collapsed loop range for loops in the associated
> +   loops.  If not processing_template_decl, this is diagnosed by
> +   finish_id_expression -> mark_used before the range for is deduced.
> +   And if processing_template_decl and the range for expression is
> +   type dependent, it is similarly diagnosed during instantiation.
> +   Only when processing_template_decl and range for expression is
> +   not type dependent, we wouldn't diagnose it at all, so do it
> +   from finish_omp_for in that case.  */
> +
> +static tree
> +finish_omp_for_decomps_r (tree *tp, int *, void *d)
> +{
> +  if (VAR_P (*tp)
> +      && DECL_DECOMPOSITION_P (*tp)
> +      && !type_dependent_expression_p (*tp)
> +      && DECL_HAS_VALUE_EXPR_P (*tp))
> +    {
> +      tree v = DECL_VALUE_EXPR (*tp);
> +      if (TREE_CODE (v) == ARRAY_REF
> +	  && VAR_P (TREE_OPERAND (v, 0))
> +	  && DECL_DECOMPOSITION_P (TREE_OPERAND (v, 0)))
> +	{
> +	  finish_omp_for_data *data = (finish_omp_for_data *) d;
> +	  if (data->decomps->contains (TREE_OPERAND (v, 0)))
> +	    {
> +	      error_at (data->loc, "use of %qD before deduction of %<auto%>",
> +			*tp);
> +	      data->fail = true;
> +	    }
> +	}
> +    }
> +  return NULL_TREE;
> +}
> +
>   /* Build and validate an OMP_FOR statement.  CLAUSES, BODY, COND, INCR
>      are directly for their associated operands in the statement.  DECL
>      and INIT are a combo; if DECL is NULL then INIT ought to be a
> @@ -10419,6 +10461,7 @@ finish_omp_for (location_t locus, enum t
>     int i;
>     int collapse = 1;
>     int ordered = 0;
> +  finish_omp_for_data data;
>   
>     gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (initv));
>     gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (condv));
> @@ -10479,7 +10522,25 @@ finish_omp_for (location_t locus, enum t
>   	elocus = EXPR_LOCATION (init);
>   
>         if (cond == global_namespace)
> -	continue;
> +	{
> +	  gcc_assert (processing_template_decl);
> +	  if (TREE_VEC_LENGTH (declv) > 1
> +	      && VAR_P (decl)
> +	      && DECL_DECOMPOSITION_P (decl)
> +	      && !type_dependent_expression_p (decl))
> +	    {
> +	      gcc_assert (DECL_HAS_VALUE_EXPR_P (decl));
> +	      tree v = DECL_VALUE_EXPR (decl);
> +	      gcc_assert (TREE_CODE (v) == ARRAY_REF
> +			  && VAR_P (TREE_OPERAND (v, 0))
> +			  && DECL_DECOMPOSITION_P (TREE_OPERAND (v, 0)));
> +	      if (!data.decomps)
> +		data.decomps
> +		  = std::unique_ptr<hash_set<tree>> (new hash_set<tree>);
> +	      data.decomps->add (TREE_OPERAND (v, 0));
> +	    }
> +	  continue;
> +	}
>   
>         if (cond == NULL)
>   	{
> @@ -10497,6 +10558,37 @@ finish_omp_for (location_t locus, enum t
>         TREE_VEC_ELT (initv, i) = init;
>       }
>   
> +  if (data.decomps)
> +    {
> +      data.fail = false;
> +      data.loc = locus;
> +      hash_set<tree> pset;
> +      for (i = 0; i < TREE_VEC_LENGTH (declv); i++)
> +	{
> +	  init = TREE_VEC_ELT (initv, i);
> +	  cond = TREE_VEC_ELT (condv, i);
> +	  incr = TREE_VEC_ELT (incrv, i);
> +	  data.loc = EXPR_LOC_OR_LOC (init, locus);
> +	  cp_walk_tree (&init, finish_omp_for_decomps_r, &data, &pset);
> +	  data.loc = EXPR_LOC_OR_LOC (cond, locus);
> +	  cp_walk_tree (&cond, finish_omp_for_decomps_r, &data, &pset);
> +	  data.loc = EXPR_LOC_OR_LOC (incr, locus);
> +	  cp_walk_tree (&incr, finish_omp_for_decomps_r, &data, &pset);
> +	}
> +      if (orig_inits && !data.fail)
> +	{
> +	  tree orig_init;
> +	  FOR_EACH_VEC_ELT (*orig_inits, i, orig_init)
> +	    {
> +	      data.loc = EXPR_LOC_OR_LOC (orig_init, locus);
> +	      cp_walk_tree (&orig_init, finish_omp_for_decomps_r,
> +			    &data, &pset);
> +	    }
> +	}
> +      if (data.fail)
> +	return NULL;
> +    }
> +
>     if (orig_inits)
>       {
>         bool fail = false;
> --- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2020-01-12 11:54:37.178401867 +0100
> +++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-11-29 13:06:59.038410557 +0100
> @@ -54,9 +54,9 @@ void
>   f6 (S (&a)[10])
>   {
>     #pragma omp for collapse (2)
> -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" }
>       for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }

Hmm, this error is surprising: since the initializer is non-dependent, 
we should have deduced immediately.  I'd expect the same error as in the 
non-structured-binding cases, "* expression refers to iteration variable".

>   }
>   
>   template <typename T>
> --- gcc/testsuite/g++.dg/gomp/for-22.C.jj	2022-11-29 13:07:10.859237506 +0100
> +++ gcc/testsuite/g++.dg/gomp/for-22.C	2022-11-29 13:09:50.743897003 +0100
> @@ -0,0 +1,57 @@
> +// { dg-do compile { target c++17 } }
> +
> +namespace std {
> +  template<typename T> struct tuple_size;
> +  template<int, typename> struct tuple_element;
> +}
> +
> +struct A {
> +  int i;
> +  template <int I> int& get() { return i; }
> +};
> +
> +template<> struct std::tuple_size<A> { static const int value = 3; };
> +template<int I> struct std::tuple_element<I,A> { using type = int; };
> +
> +struct B {
> +  A *begin();
> +  A *end();
> +};
> +
> +void
> +f1 (B a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
> +}
> +
> +template <int N>
> +void
> +f2 (B a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
> +}
> +
> +template <typename T>
> +void
> +f3 (T a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> +}
> +
> +void
> +test ()
> +{
> +  B b;
> +  f1 (b);
> +  f2 <0> (b);
> +  f3 <B> (b);
> +}
> 
> 	Jakub
> 

Re: [PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jakub Jelinek]

On Tue, Nov 29, 2022 at 04:38:50PM -0500, Jason Merrill wrote:
> > --- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2020-01-12 11:54:37.178401867 +0100
> > +++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-11-29 13:06:59.038410557 +0100
> > @@ -54,9 +54,9 @@ void
> >   f6 (S (&a)[10])
> >   {
> >     #pragma omp for collapse (2)
> > -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> > +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" }
> >       for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> > -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> > +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
> 
> Hmm, this error is surprising: since the initializer is non-dependent, we
> should have deduced immediately.  I'd expect the same error as in the
> non-structured-binding cases, "* expression refers to iteration variable".

The reason was just to be consistent what is (unfortunately) emitted
in the other cases (!processing_template_decl or type dependent).
I guess I could try how much work would it be to deduce it sooner, but
generally it is pretty corner case, people rarely do this in OpenMP code.

	Jakub

Re: [PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jakub Jelinek]

On Tue, Nov 29, 2022 at 11:05:33PM +0100, Jakub Jelinek wrote:
> On Tue, Nov 29, 2022 at 04:38:50PM -0500, Jason Merrill wrote:
> > > --- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2020-01-12 11:54:37.178401867 +0100
> > > +++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-11-29 13:06:59.038410557 +0100
> > > @@ -54,9 +54,9 @@ void
> > >   f6 (S (&a)[10])
> > >   {
> > >     #pragma omp for collapse (2)
> > > -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> > > +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" }
> > >       for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> > > -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> > > +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
> > 
> > Hmm, this error is surprising: since the initializer is non-dependent, we
> > should have deduced immediately.  I'd expect the same error as in the
> > non-structured-binding cases, "* expression refers to iteration variable".
> 
> The reason was just to be consistent what is (unfortunately) emitted
> in the other cases (!processing_template_decl or type dependent).
> I guess I could try how much work would it be to deduce it sooner, but
> generally it is pretty corner case, people rarely do this in OpenMP code.

I had a look at that today, but it would be pretty hard.  The thing is
we must emit all the associated code for all the range for loops in
OpenMP loops at a different spot.  So, the only possibility I see would
be if we during parsing of a range for loop inside of the OpenMP loop nest
we don't do the cp_finish_omp_range_for stuff to avoid e.g. cp_finish_decl,
but instead
  build_x_indirect_ref (input_location, begin, RO_UNARY_STAR,
			NULL_TREE, tf_none)
and if that gives a non-dependent type, temporarily overwrite TREE_TYPE
of the decl and if it is structured binding, temporarily
++processing_template_decl and cp_finish_decomp, then after parsing all the
associated loop headers we revert that (and ditto for instantiation of
OpenMP loops).

	Jakub

Re: [PATCH] c++: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jason Merrill]

On 11/30/22 10:51, Jakub Jelinek wrote:
> On Tue, Nov 29, 2022 at 11:05:33PM +0100, Jakub Jelinek wrote:
>> On Tue, Nov 29, 2022 at 04:38:50PM -0500, Jason Merrill wrote:
>>>> --- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2020-01-12 11:54:37.178401867 +0100
>>>> +++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-11-29 13:06:59.038410557 +0100
>>>> @@ -54,9 +54,9 @@ void
>>>>    f6 (S (&a)[10])
>>>>    {
>>>>      #pragma omp for collapse (2)
>>>> -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
>>>> +  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" }
>>>>        for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
>>>> -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
>>>> +      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
>>>
>>> Hmm, this error is surprising: since the initializer is non-dependent, we
>>> should have deduced immediately.  I'd expect the same error as in the
>>> non-structured-binding cases, "* expression refers to iteration variable".
>>
>> The reason was just to be consistent what is (unfortunately) emitted
>> in the other cases (!processing_template_decl or type dependent).
>> I guess I could try how much work would it be to deduce it sooner, but
>> generally it is pretty corner case, people rarely do this in OpenMP code.
> 
> I had a look at that today, but it would be pretty hard.  The thing is
> we must emit all the associated code for all the range for loops in
> OpenMP loops at a different spot.  So, the only possibility I see would
> be if we during parsing of a range for loop inside of the OpenMP loop nest
> we don't do the cp_finish_omp_range_for stuff to avoid e.g. cp_finish_decl,
> but instead
>    build_x_indirect_ref (input_location, begin, RO_UNARY_STAR,
> 			NULL_TREE, tf_none)
> and if that gives a non-dependent type, temporarily overwrite TREE_TYPE
> of the decl and if it is structured binding, temporarily
> ++processing_template_decl and cp_finish_decomp, then after parsing all the
> associated loop headers we revert that (and ditto for instantiation of
> OpenMP loops).

It looks like we're already deducing the type for the underlying S 
variable in cp_convert_omp_range_for, we just aren't updating the types 
of the individual bindings.

Jason

[PATCH] c++, v2: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jakub Jelinek]

On Wed, Nov 30, 2022 at 01:52:08PM -0500, Jason Merrill wrote:
> It looks like we're already deducing the type for the underlying S variable
> in cp_convert_omp_range_for, we just aren't updating the types of the
> individual bindings.

You're right.  With this patch (still incremental against the base PR84469
patch) we get the nicer diagnostics in all cases.

Regtested successfully on x86_64-linux (g++ gomp.exp/goacc.exp/goacc-gomp.exp
and libgomp's c++.exp), ok for trunk (including the base patch)
if it passes full bootstrap/regtest?

2022-12-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/84469
gcc/c-family/
	* c-omp.cc (c_omp_is_loop_iterator): For range for with structured
	binding return TREE_VEC_LENGTH (d->declv) even if decl is equal
	to any of the structured binding decls.
gcc/cp/
	* parser.cc (cp_convert_omp_range_for): After do_auto_deduction if
	!processing_template_decl call cp_finish_decomp with
	processing_template_decl temporarily incremented.
gcc/testsuite/
	* g++.dg/gomp/for-21.C (f3, f6, f9): Adjust expected diagnostics.
	* g++.dg/gomp/for-22.C: New test.

--- gcc/c-family/c-omp.cc.jj	2022-10-04 10:36:46.515414485 +0200
+++ gcc/c-family/c-omp.cc	2022-12-01 10:57:56.365253302 +0100
@@ -1311,10 +1311,11 @@ c_omp_is_loop_iterator (tree decl, struc
     else if (TREE_CODE (TREE_VEC_ELT (d->declv, i)) == TREE_LIST
 	     && TREE_CHAIN (TREE_VEC_ELT (d->declv, i))
 	     && (TREE_CODE (TREE_CHAIN (TREE_VEC_ELT (d->declv, i)))
-		 == TREE_VEC)
-	     && decl == TREE_VEC_ELT (TREE_CHAIN (TREE_VEC_ELT (d->declv,
-						  i)), 2))
-      return TREE_VEC_LENGTH (d->declv);
+		 == TREE_VEC))
+      for (int j = 2;
+	   j < TREE_VEC_LENGTH (TREE_CHAIN (TREE_VEC_ELT (d->declv, i))); j++)
+	if (decl == TREE_VEC_ELT (TREE_CHAIN (TREE_VEC_ELT (d->declv, i)), j))
+	  return TREE_VEC_LENGTH (d->declv);
   return -1;
 }
 
--- gcc/cp/parser.cc.jj	2022-12-01 10:19:27.000000000 +0100
+++ gcc/cp/parser.cc	2022-12-01 10:21:30.760450093 +0100
@@ -43126,8 +43126,16 @@ cp_convert_omp_range_for (tree &this_pre
       tree t = build_x_indirect_ref (input_location, begin, RO_UNARY_STAR,
 				     NULL_TREE, tf_none);
       if (!error_operand_p (t))
-	TREE_TYPE (orig_decl) = do_auto_deduction (TREE_TYPE (orig_decl),
-						   t, auto_node);
+	{
+	  TREE_TYPE (orig_decl) = do_auto_deduction (TREE_TYPE (orig_decl),
+						     t, auto_node);
+	  if (decomp_first_name)
+	    {
+	      ++processing_template_decl;
+	      cp_finish_decomp (orig_decl, decomp_first_name, decomp_cnt);
+	      --processing_template_decl;
+	    }
+	}
     }
 
   tree v = make_tree_vec (decomp_cnt + 3);
--- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2022-11-30 10:29:09.332186135 +0100
+++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-12-01 11:05:40.888414600 +0100
@@ -24,9 +24,9 @@ void
 f3 (S (&a)[10])
 {
   #pragma omp for collapse (2)
-  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
-    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
-      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" }
+    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-2 }
 }
 
 template <int N>
@@ -54,9 +54,9 @@ void
 f6 (S (&a)[10])
 {
   #pragma omp for collapse (2)
-  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
-    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
-      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
+    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
 }
 
 template <typename T>
@@ -84,9 +84,9 @@ void
 f9 (U (&a)[10])
 {
   #pragma omp for collapse (2)
-  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
-    for (T l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
-      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
+    for (T l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
 }
 
 void
--- gcc/testsuite/g++.dg/gomp/for-22.C.jj	2022-12-01 10:17:47.117785354 +0100
+++ gcc/testsuite/g++.dg/gomp/for-22.C	2022-12-01 11:08:19.578078371 +0100
@@ -0,0 +1,57 @@
+// { dg-do compile { target c++17 } }
+
+namespace std {
+  template<typename T> struct tuple_size;
+  template<int, typename> struct tuple_element;
+}
+
+struct A {
+  int i;
+  template <int I> int& get() { return i; }
+};
+
+template<> struct std::tuple_size<A> { static const int value = 3; };
+template<int I> struct std::tuple_element<I,A> { using type = int; };
+
+struct B {
+  A *begin();
+  A *end();
+};
+
+void
+f1 (B a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .+1 }
+    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-1 }
+}
+
+template <int N>
+void
+f2 (B a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
+    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
+}
+
+template <typename T>
+void
+f3 (T a)
+{
+  #pragma omp for collapse (2)
+  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
+    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
+      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
+}
+
+void
+test ()
+{
+  B b;
+  f1 (b);
+  f2 <0> (b);
+  f3 <B> (b);
+}


	Jakub

Re: [PATCH] c++, v2: Incremental fix for g++.dg/gomp/for-21.C [PR84469]

[Jason Merrill]

On 12/1/22 05:32, Jakub Jelinek wrote:
> On Wed, Nov 30, 2022 at 01:52:08PM -0500, Jason Merrill wrote:
>> It looks like we're already deducing the type for the underlying S variable
>> in cp_convert_omp_range_for, we just aren't updating the types of the
>> individual bindings.
> 
> You're right.  With this patch (still incremental against the base PR84469
> patch) we get the nicer diagnostics in all cases.
> 
> Regtested successfully on x86_64-linux (g++ gomp.exp/goacc.exp/goacc-gomp.exp
> and libgomp's c++.exp), ok for trunk (including the base patch)
> if it passes full bootstrap/regtest?

OK, thanks.

> 2022-12-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/84469
> gcc/c-family/
> 	* c-omp.cc (c_omp_is_loop_iterator): For range for with structured
> 	binding return TREE_VEC_LENGTH (d->declv) even if decl is equal
> 	to any of the structured binding decls.
> gcc/cp/
> 	* parser.cc (cp_convert_omp_range_for): After do_auto_deduction if
> 	!processing_template_decl call cp_finish_decomp with
> 	processing_template_decl temporarily incremented.
> gcc/testsuite/
> 	* g++.dg/gomp/for-21.C (f3, f6, f9): Adjust expected diagnostics.
> 	* g++.dg/gomp/for-22.C: New test.
> 
> --- gcc/c-family/c-omp.cc.jj	2022-10-04 10:36:46.515414485 +0200
> +++ gcc/c-family/c-omp.cc	2022-12-01 10:57:56.365253302 +0100
> @@ -1311,10 +1311,11 @@ c_omp_is_loop_iterator (tree decl, struc
>       else if (TREE_CODE (TREE_VEC_ELT (d->declv, i)) == TREE_LIST
>   	     && TREE_CHAIN (TREE_VEC_ELT (d->declv, i))
>   	     && (TREE_CODE (TREE_CHAIN (TREE_VEC_ELT (d->declv, i)))
> -		 == TREE_VEC)
> -	     && decl == TREE_VEC_ELT (TREE_CHAIN (TREE_VEC_ELT (d->declv,
> -						  i)), 2))
> -      return TREE_VEC_LENGTH (d->declv);
> +		 == TREE_VEC))
> +      for (int j = 2;
> +	   j < TREE_VEC_LENGTH (TREE_CHAIN (TREE_VEC_ELT (d->declv, i))); j++)
> +	if (decl == TREE_VEC_ELT (TREE_CHAIN (TREE_VEC_ELT (d->declv, i)), j))
> +	  return TREE_VEC_LENGTH (d->declv);
>     return -1;
>   }
>   
> --- gcc/cp/parser.cc.jj	2022-12-01 10:19:27.000000000 +0100
> +++ gcc/cp/parser.cc	2022-12-01 10:21:30.760450093 +0100
> @@ -43126,8 +43126,16 @@ cp_convert_omp_range_for (tree &this_pre
>         tree t = build_x_indirect_ref (input_location, begin, RO_UNARY_STAR,
>   				     NULL_TREE, tf_none);
>         if (!error_operand_p (t))
> -	TREE_TYPE (orig_decl) = do_auto_deduction (TREE_TYPE (orig_decl),
> -						   t, auto_node);
> +	{
> +	  TREE_TYPE (orig_decl) = do_auto_deduction (TREE_TYPE (orig_decl),
> +						     t, auto_node);
> +	  if (decomp_first_name)
> +	    {
> +	      ++processing_template_decl;
> +	      cp_finish_decomp (orig_decl, decomp_first_name, decomp_cnt);
> +	      --processing_template_decl;
> +	    }
> +	}
>       }
>   
>     tree v = make_tree_vec (decomp_cnt + 3);
> --- gcc/testsuite/g++.dg/gomp/for-21.C.jj	2022-11-30 10:29:09.332186135 +0100
> +++ gcc/testsuite/g++.dg/gomp/for-21.C	2022-12-01 11:05:40.888414600 +0100
> @@ -24,9 +24,9 @@ void
>   f3 (S (&a)[10])
>   {
>     #pragma omp for collapse (2)
> -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .+1 }
> -    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-1 }
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" }
> +    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-2 }
>   }
>   
>   template <int N>
> @@ -54,9 +54,9 @@ void
>   f6 (S (&a)[10])
>   {
>     #pragma omp for collapse (2)
> -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> -    for (int l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
>   }
>   
>   template <typename T>
> @@ -84,9 +84,9 @@ void
>   f9 (U (&a)[10])
>   {
>     #pragma omp for collapse (2)
> -  for (auto [i, j, k] : a)			// { dg-error "use of 'i' before deduction of 'auto'" "" { target *-*-* } .-1 }
> -    for (T l = i; l < j; l += k)		// { dg-error "use of 'j' before deduction of 'auto'" }
> -      ;						// { dg-error "use of 'k' before deduction of 'auto'" "" { target *-*-* } .-3 }
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
> +    for (T l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
>   }
>   
>   void
> --- gcc/testsuite/g++.dg/gomp/for-22.C.jj	2022-12-01 10:17:47.117785354 +0100
> +++ gcc/testsuite/g++.dg/gomp/for-22.C	2022-12-01 11:08:19.578078371 +0100
> @@ -0,0 +1,57 @@
> +// { dg-do compile { target c++17 } }
> +
> +namespace std {
> +  template<typename T> struct tuple_size;
> +  template<int, typename> struct tuple_element;
> +}
> +
> +struct A {
> +  int i;
> +  template <int I> int& get() { return i; }
> +};
> +
> +template<> struct std::tuple_size<A> { static const int value = 3; };
> +template<int I> struct std::tuple_element<I,A> { using type = int; };
> +
> +struct B {
> +  A *begin();
> +  A *end();
> +};
> +
> +void
> +f1 (B a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .+1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-1 }
> +}
> +
> +template <int N>
> +void
> +f2 (B a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
> +}
> +
> +template <typename T>
> +void
> +f3 (T a)
> +{
> +  #pragma omp for collapse (2)
> +  for (auto [i, j, k] : a)			// { dg-error "initializer expression refers to iteration variable 'i'" "" { target *-*-* } .-1 }
> +    for (int l = i; l < j; l += k)		// { dg-error "condition expression refers to iteration variable 'j'" }
> +      ;						// { dg-error "increment expression refers to iteration variable 'k'" "" { target *-*-* } .-3 }
> +}
> +
> +void
> +test ()
> +{
> +  B b;
> +  f1 (b);
> +  f2 <0> (b);
> +  f3 <B> (b);
> +}
> 
> 
> 	Jakub
>