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

  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.


	Jakub
  

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
>
  

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
  

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
  

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