c++: Further #pragma GCC unroll C++ fix [PR112795]

  Hi!

When committing the #pragma GCC unroll patch, I found I forgot one spot
for diagnosting the invalid unrolls - if #pragma GCC unroll argument is
dependent and the pragma is before a range for loop, the unroll tree (now,
before one converted form ushort) is saved into RANGE_FOR_UNROLL and
tsubst_stmt was RECURing on it, but didn't diagnose if it was invalid and
so we ICEd later in the middle-end when  ANNOTATE_EXPR had unexpected
argument.

The following patch fixes that.  Or should I create some helper function
(how to call it) and call it from all of cp_parser_pragma_unroll,
tsubst_stmt (here) and tsubst_expr (ANNOTATE_EXPR)?
Another option is diagnose it instead when we create the ANNOTATE_EXPRs,
but unfortunately that is in 3 different places.  And at least for the
non-template case we'd have worse location_t.

Bootstrapped/regtested on x86_64-linux and i686-linux.

2023-12-04  Jakub Jelinek  <jakub@redhat.com>

	PR c++/112795
	* pt.cc (tsubst_stmt) <case RANGE_FOR_STMT>: Perform RANGE_FOR_UNROLL
	value checking here as well.

	* g++.dg/ext/unroll-2.C: Use { target c++11 } instead of dg-skip-if for
	-std=gnu++98.
	* g++.dg/ext/unroll-3.C: Likewise.
	* g++.dg/ext/unroll-7.C: New test.
	* g++.dg/ext/unroll-8.C: New test.


	Jakub
  

On 12/5/23 01:55, Jakub Jelinek wrote:
> Hi!
> 
> When committing the #pragma GCC unroll patch, I found I forgot one spot
> for diagnosting the invalid unrolls - if #pragma GCC unroll argument is
> dependent and the pragma is before a range for loop, the unroll tree (now,
> before one converted form ushort) is saved into RANGE_FOR_UNROLL and
> tsubst_stmt was RECURing on it, but didn't diagnose if it was invalid and
> so we ICEd later in the middle-end when  ANNOTATE_EXPR had unexpected
> argument.
> 
> The following patch fixes that.  Or should I create some helper function
> (how to call it) and call it from all of cp_parser_pragma_unroll,
> tsubst_stmt (here) and tsubst_expr (ANNOTATE_EXPR)?

Please.  Maybe check_pragma_unroll? check_unroll_factor?

> Another option is diagnose it instead when we create the ANNOTATE_EXPRs,
> but unfortunately that is in 3 different places.  And at least for the
> non-template case we'd have worse location_t.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux.
> 
> 2023-12-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/112795
> 	* pt.cc (tsubst_stmt) <case RANGE_FOR_STMT>: Perform RANGE_FOR_UNROLL
> 	value checking here as well.
> 
> 	* g++.dg/ext/unroll-2.C: Use { target c++11 } instead of dg-skip-if for
> 	-std=gnu++98.
> 	* g++.dg/ext/unroll-3.C: Likewise.
> 	* g++.dg/ext/unroll-7.C: New test.
> 	* g++.dg/ext/unroll-8.C: New test.
> 
> --- gcc/cp/pt.cc.jj	2023-12-04 08:59:06.000000000 +0100
> +++ gcc/cp/pt.cc	2023-12-04 10:49:38.149254907 +0100
> @@ -18407,22 +18407,46 @@ tsubst_stmt (tree t, tree args, tsubst_f
>   					complain, in_decl, decomp);
>   	  }
>   
> +	tree unroll = RECUR (RANGE_FOR_UNROLL (t));
> +	if (unroll)
> +	  {
> +	    HOST_WIDE_INT lunroll;
> +	    if (type_dependent_expression_p (unroll))
> +	      ;
> +	    else if (!INTEGRAL_TYPE_P (TREE_TYPE (unroll))
> +		     || (!value_dependent_expression_p (unroll)
> +			 && (!tree_fits_shwi_p (unroll)
> +			     || (lunroll = tree_to_shwi (unroll)) < 0
> +			     || lunroll >= USHRT_MAX)))
> +	      {
> +		error_at (EXPR_LOCATION (RANGE_FOR_UNROLL (t)),
> +			  "%<#pragma GCC unroll%> requires an "
> +			  "assignment-expression that evaluates to a "
> +			  "non-negative integral constant less than %u",
> +			  USHRT_MAX);
> +		unroll = integer_one_node;
> +	      }
> +	    else if (TREE_CODE (unroll) == INTEGER_CST)
> +	      {
> +		unroll = fold_convert (integer_type_node, unroll);
> +		if (integer_zerop (unroll))
> +		  unroll = integer_one_node;
> +	      }
> +	  }
> +
>   	if (processing_template_decl)
>   	  {
>   	    RANGE_FOR_IVDEP (stmt) = RANGE_FOR_IVDEP (t);
> -	    RANGE_FOR_UNROLL (stmt) = RANGE_FOR_UNROLL (t);
> +	    RANGE_FOR_UNROLL (stmt) = unroll;
>   	    RANGE_FOR_NOVECTOR (stmt) = RANGE_FOR_NOVECTOR (t);
>   	    finish_range_for_decl (stmt, decl, expr);
>   	    if (decomp && decl != error_mark_node)
>   	      cp_finish_decomp (decl, decomp);
>   	  }
>   	else
> -	  {
> -	    tree unroll = RECUR (RANGE_FOR_UNROLL (t));
> -	    stmt = cp_convert_range_for (stmt, decl, expr, decomp,
> -					 RANGE_FOR_IVDEP (t), unroll,
> -					 RANGE_FOR_NOVECTOR (t));
> -	  }
> +	  stmt = cp_convert_range_for (stmt, decl, expr, decomp,
> +				       RANGE_FOR_IVDEP (t), unroll,
> +				       RANGE_FOR_NOVECTOR (t));
>   
>   	bool prev = note_iteration_stmt_body_start ();
>           RECUR (RANGE_FOR_BODY (t));
> --- gcc/testsuite/g++.dg/ext/unroll-2.C.jj	2020-01-12 11:54:37.172401958 +0100
> +++ gcc/testsuite/g++.dg/ext/unroll-2.C	2023-12-04 10:17:00.390997063 +0100
> @@ -1,6 +1,5 @@
> -// { dg-do compile }
> +// { dg-do compile { target c++11 } }
>   // { dg-options "-O2 -fdump-tree-cunrolli-details" }
> -// { dg-skip-if "range for" { *-*-* } { "-std=gnu++98" } { "" } }
>   
>   void
>   foo (int (&a)[8], int *b, int *c)
> --- gcc/testsuite/g++.dg/ext/unroll-3.C.jj	2020-01-12 11:54:37.172401958 +0100
> +++ gcc/testsuite/g++.dg/ext/unroll-3.C	2023-12-04 10:17:13.526813516 +0100
> @@ -1,6 +1,5 @@
> -// { dg-do compile }
> +// { dg-do compile { target c++11 } }
>   // { dg-options "-O2 -fdump-tree-cunrolli-details" }
> -// { dg-skip-if "range for" { *-*-* } { "-std=gnu++98" } { "" } }
>   
>   template <typename T>
>   void
> --- gcc/testsuite/g++.dg/ext/unroll-7.C.jj	2023-12-04 10:17:53.481255222 +0100
> +++ gcc/testsuite/g++.dg/ext/unroll-7.C	2023-12-04 10:39:23.258115349 +0100
> @@ -0,0 +1,45 @@
> +// PR c++/112795
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -fdump-tree-cunrolli-details" }
> +
> +void baz (int);
> +constexpr int n = 3;
> +constexpr int m = 7;
> +
> +template <typename T>
> +void
> +foo (int (&a)[3], T b)
> +{
> +#pragma GCC unroll(n)
> +  for (auto i : a)
> +    baz (i);
> +#pragma GCC unroll(m)
> +  for (auto i : b)
> +    baz (i);
> +}
> +
> +template <int N>
> +void
> +bar (int (&a)[N])
> +{
> +#pragma GCC unroll(N)
> +  for (auto i : a)
> +    baz (i);
> +}
> +
> +void
> +qux ()
> +{
> +  int a[3] = { 1, 2, 3 };
> +  int b[7] = { 4, 5, 6, 7, 8, 9, 10 };
> +  int c[6] = { 11, 12, 13, 14, 15, 16 };
> +  int d[10] = { 17, 18, 19, 20, 21, 22, 23, 24, 25, 26 };
> +  foo <int (&)[7]> (a, b);
> +  bar <6> (c);
> +  bar <10> (d);
> +}
> +
> +// { dg-final { scan-tree-dump "loop with 3 iterations completely unrolled" "cunrolli" } }
> +// { dg-final { scan-tree-dump "loop with 6 iterations completely unrolled" "cunrolli" } }
> +// { dg-final { scan-tree-dump "loop with 7 iterations completely unrolled" "cunrolli" } }
> +// { dg-final { scan-tree-dump "loop with 10 iterations completely unrolled" "cunrolli" } }
> --- gcc/testsuite/g++.dg/ext/unroll-8.C.jj	2023-12-04 10:17:56.763209358 +0100
> +++ gcc/testsuite/g++.dg/ext/unroll-8.C	2023-12-04 10:44:58.531217053 +0100
> @@ -0,0 +1,86 @@
> +// PR c++/112795
> +// { dg-do compile { target c++11 } }
> +
> +void
> +foo (int (&a)[3])
> +{
> +  #pragma GCC unroll 1.0f			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll 0xffffffffffffffffULL	// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll -42			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +}
> +
> +template <int N, typename U>
> +void
> +bar (U a)
> +{
> +  #pragma GCC unroll 1.0f			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll 0xffffffffffffffffULL	// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll -42			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +}
> +
> +template <typename T, int N, typename U>
> +void
> +baz (U a)
> +{
> +  #pragma GCC unroll (N + 1.0f)			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll (N + 0xffffffffffffffffULL)
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll (N - 42)
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) 1.0f)
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) 0xffffffffffffffffULL)
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) -42)
> +  for (auto i : a)
> +    ;
> +}
> +
> +template <typename T, int N, typename U>
> +void
> +qux (U a)
> +{
> +  #pragma GCC unroll (N + 1.0f)			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll (N + 0xffffffffffffffffULL)// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll (N - 42)			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) 1.0f)			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) 0xffffffffffffffffULL)// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +  #pragma GCC unroll ((T) -42)			// { dg-error "'#pragma GCC unroll' requires an assignment-expression that evaluates to a non-negative integral constant less than" }
> +  for (auto i : a)
> +    ;
> +}
> +
> +void
> +corge ()
> +{
> +  int a[3] = { 1, 2, 3 };
> +  qux <float, 0, int (&)[3]> (a);
> +}
> 
> 	Jakub
>
  

On Tue, Dec 05, 2023 at 07:55:37AM +0100, Jakub Jelinek wrote:
> Hi!
> 
> When committing the #pragma GCC unroll patch, I found I forgot one spot
> for diagnosting the invalid unrolls - if #pragma GCC unroll argument is
> dependent and the pragma is before a range for loop, the unroll tree (now,
> before one converted form ushort) is saved into RANGE_FOR_UNROLL and
> tsubst_stmt was RECURing on it, but didn't diagnose if it was invalid and
> so we ICEd later in the middle-end when  ANNOTATE_EXPR had unexpected
> argument.
> 
> The following patch fixes that.  Or should I create some helper function
> (how to call it) and call it from all of cp_parser_pragma_unroll,
> tsubst_stmt (here) and tsubst_expr (ANNOTATE_EXPR)?
> Another option is diagnose it instead when we create the ANNOTATE_EXPRs,
> but unfortunately that is in 3 different places.  And at least for the
> non-template case we'd have worse location_t.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux.
> 
> 2023-12-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/112795
> 	* pt.cc (tsubst_stmt) <case RANGE_FOR_STMT>: Perform RANGE_FOR_UNROLL
> 	value checking here as well.
> 
> 	* g++.dg/ext/unroll-2.C: Use { target c++11 } instead of dg-skip-if for
> 	-std=gnu++98.
> 	* g++.dg/ext/unroll-3.C: Likewise.
> 	* g++.dg/ext/unroll-7.C: New test.
> 	* g++.dg/ext/unroll-8.C: New test.
> 
> --- gcc/cp/pt.cc.jj	2023-12-04 08:59:06.000000000 +0100
> +++ gcc/cp/pt.cc	2023-12-04 10:49:38.149254907 +0100
> @@ -18407,22 +18407,46 @@ tsubst_stmt (tree t, tree args, tsubst_f
>  					complain, in_decl, decomp);
>  	  }
>  
> +	tree unroll = RECUR (RANGE_FOR_UNROLL (t));
> +	if (unroll)
> +	  {
> +	    HOST_WIDE_INT lunroll;
> +	    if (type_dependent_expression_p (unroll))
> +	      ;
> +	    else if (!INTEGRAL_TYPE_P (TREE_TYPE (unroll))
> +		     || (!value_dependent_expression_p (unroll)
> +			 && (!tree_fits_shwi_p (unroll)
> +			     || (lunroll = tree_to_shwi (unroll)) < 0
> +			     || lunroll >= USHRT_MAX)))
> +	      {
> +		error_at (EXPR_LOCATION (RANGE_FOR_UNROLL (t)),
> +			  "%<#pragma GCC unroll%> requires an "
> +			  "assignment-expression that evaluates to a "
> +			  "non-negative integral constant less than %u",
> +			  USHRT_MAX);
> +		unroll = integer_one_node;
> +	      }
> +	    else if (TREE_CODE (unroll) == INTEGER_CST)
> +	      {
> +		unroll = fold_convert (integer_type_node, unroll);
> +		if (integer_zerop (unroll))
> +		  unroll = integer_one_node;
> +	      }
> +	  }

As you note, this is exactly the same code we already have in the
ANNOTATE_EXPR case.  Maybe add check_and_convert_unroll value?

Marek
  

On Tue, Dec 05, 2023 at 10:21:18AM -0500, Marek Polacek wrote:
> On Tue, Dec 05, 2023 at 07:55:37AM +0100, Jakub Jelinek wrote:
> > Hi!
> > 
> > When committing the #pragma GCC unroll patch, I found I forgot one spot
> > for diagnosting the invalid unrolls - if #pragma GCC unroll argument is
> > dependent and the pragma is before a range for loop, the unroll tree (now,
> > before one converted form ushort) is saved into RANGE_FOR_UNROLL and
> > tsubst_stmt was RECURing on it, but didn't diagnose if it was invalid and
> > so we ICEd later in the middle-end when  ANNOTATE_EXPR had unexpected
> > argument.
> > 
> > The following patch fixes that.  Or should I create some helper function
> > (how to call it) and call it from all of cp_parser_pragma_unroll,
> > tsubst_stmt (here) and tsubst_expr (ANNOTATE_EXPR)?
> > Another option is diagnose it instead when we create the ANNOTATE_EXPRs,
> > but unfortunately that is in 3 different places.  And at least for the
> > non-template case we'd have worse location_t.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux.
> > 
> > 2023-12-04  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/112795
> > 	* pt.cc (tsubst_stmt) <case RANGE_FOR_STMT>: Perform RANGE_FOR_UNROLL
> > 	value checking here as well.
> > 
> > 	* g++.dg/ext/unroll-2.C: Use { target c++11 } instead of dg-skip-if for
> > 	-std=gnu++98.
> > 	* g++.dg/ext/unroll-3.C: Likewise.
> > 	* g++.dg/ext/unroll-7.C: New test.
> > 	* g++.dg/ext/unroll-8.C: New test.
> > 
> > --- gcc/cp/pt.cc.jj	2023-12-04 08:59:06.000000000 +0100
> > +++ gcc/cp/pt.cc	2023-12-04 10:49:38.149254907 +0100
> > @@ -18407,22 +18407,46 @@ tsubst_stmt (tree t, tree args, tsubst_f
> >  					complain, in_decl, decomp);
> >  	  }
> >  
> > +	tree unroll = RECUR (RANGE_FOR_UNROLL (t));
> > +	if (unroll)
> > +	  {
> > +	    HOST_WIDE_INT lunroll;
> > +	    if (type_dependent_expression_p (unroll))
> > +	      ;
> > +	    else if (!INTEGRAL_TYPE_P (TREE_TYPE (unroll))
> > +		     || (!value_dependent_expression_p (unroll)
> > +			 && (!tree_fits_shwi_p (unroll)
> > +			     || (lunroll = tree_to_shwi (unroll)) < 0
> > +			     || lunroll >= USHRT_MAX)))
> > +	      {
> > +		error_at (EXPR_LOCATION (RANGE_FOR_UNROLL (t)),
> > +			  "%<#pragma GCC unroll%> requires an "
> > +			  "assignment-expression that evaluates to a "
> > +			  "non-negative integral constant less than %u",
> > +			  USHRT_MAX);
> > +		unroll = integer_one_node;
> > +	      }
> > +	    else if (TREE_CODE (unroll) == INTEGER_CST)
> > +	      {
> > +		unroll = fold_convert (integer_type_node, unroll);
> > +		if (integer_zerop (unroll))
> > +		  unroll = integer_one_node;
> > +	      }
> > +	  }
> 
> As you note, this is exactly the same code we already have in the
> ANNOTATE_EXPR case.  Maybe add check_and_convert_unroll value?

I'm too late.  Never mind.

Marek