Re: [PATCH 1/2][frontend] Add novector C++ pragma

[Jason Merrill <jason@redhat.com>]

On 7/19/23 11:15, Tamar Christina wrote:
> Hi All,
> 
> FORTRAN currently has a pragma NOVECTOR for indicating that vectorization should
> not be applied to a particular loop.
> 
> ICC/ICX also has such a pragma for C and C++ called #pragma novector.
> 
> As part of this patch series I need a way to easily turn off vectorization of
> particular loops, particularly for testsuite reasons.
> 
> This patch proposes a #pragma GCC novector that does the same for C++
> as gfortan does for FORTRAN and what ICX/ICX does for C++.
> 
> I added only some basic tests here, but the next patch in the series uses this
> in the testsuite in about ~800 tests.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master?
> 
> Thanks,
> Tamar
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.def (RANGE_FOR_STMT): Update comment.
> 	* cp-tree.h (RANGE_FOR_NOVECTOR): New.
> 	(cp_convert_range_for, finish_while_stmt_cond, finish_do_stmt,
> 	finish_for_cond): Add novector param.
> 	* init.cc (build_vec_init): Default novector to false.
> 	* method.cc (build_comparison_op): Likewise.
> 	* parser.cc (cp_parser_statement): Likewise.
> 	(cp_parser_for, cp_parser_c_for, cp_parser_range_for,
> 	cp_convert_range_for, cp_parser_iteration_statement,
> 	cp_parser_omp_for_loop, cp_parser_pragma): Support novector.
> 	(cp_parser_pragma_novector): New.
> 	* pt.cc (tsubst_expr): Likewise.
> 	* semantics.cc (finish_while_stmt_cond, finish_do_stmt,
> 	finish_for_cond): Likewise.
> 
> gcc/ChangeLog:
> 
> 	* doc/extend.texi: Document it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/vect/vect.exp (support vect- prefix).
> 	* g++.dg/vect/vect-novector-pragma.cc: New test.
> 
> --- inline copy of patch --
> diff --git a/gcc/cp/cp-tree.def b/gcc/cp/cp-tree.def
> index 0e66ca70e00caa1dc4beada1024ace32954e2aaf..c13c8ea98a523c4ef1c55a11e02d5da9db7e367e 100644
> --- a/gcc/cp/cp-tree.def
> +++ b/gcc/cp/cp-tree.def
> @@ -305,8 +305,8 @@ DEFTREECODE (IF_STMT, "if_stmt", tcc_statement, 4)
>   
>   /* Used to represent a range-based `for' statement. The operands are
>      RANGE_FOR_DECL, RANGE_FOR_EXPR, RANGE_FOR_BODY, RANGE_FOR_SCOPE,
> -   RANGE_FOR_UNROLL, and RANGE_FOR_INIT_STMT, respectively.  Only used in
> -   templates.  */
> +   RANGE_FOR_UNROLL, RANGE_FOR_NOVECTOR and RANGE_FOR_INIT_STMT,
> +   respectively.  Only used in templates.  */

This change is unnecessary; RANGE_FOR_NOVECTOR is a flag, not an operand.

>   DEFTREECODE (RANGE_FOR_STMT, "range_for_stmt", tcc_statement, 6)
>   
>   /* Used to represent an expression statement.  Use `EXPR_STMT_EXPR' to
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index dd3665c8ccf48a8a0b1ba2c06400fe50999ea240..8776e8f4cf8266ee715c3e7f943602fdb1acaf79 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -13658,7 +13660,13 @@ cp_parser_c_for (cp_parser *parser, tree scope, tree init, bool ivdep,
>   		       "%<GCC unroll%> pragma");
>         condition = error_mark_node;
>       }
> -  finish_for_cond (condition, stmt, ivdep, unroll);
> +  else if (novector)
> +    {
> +      cp_parser_error (parser, "missing loop condition in loop with "
> +		       "%<GCC novector%> pragma");
> +      condition = error_mark_node;
> +    }

Why is it a problem for a loop with novector to have no condition?  This 
error makes sense for the other pragmas that want to optimize based on 
the condition, it seems unneeded for this pragma.

> +
> +	cp_token *tok = pragma_tok;
> +
> +	do
>   	  {
> -	    tok = cp_lexer_consume_token (parser->lexer);
> -	    ivdep = cp_parser_pragma_ivdep (parser, tok);
> -	    tok = cp_lexer_peek_token (the_parser->lexer);
> +	    switch (cp_parser_pragma_kind (tok))
> +	      {
> +		case PRAGMA_IVDEP:
> +		  {
> +		    if (tok != pragma_tok)
> +		      tok = cp_lexer_consume_token (parser->lexer);
> +		    ivdep = cp_parser_pragma_ivdep (parser, tok);
> +		    tok = cp_lexer_peek_token (the_parser->lexer);
> +		    break;
> +		  }
> +		case PRAGMA_UNROLL:
> +		  {
> +		    if (tok != pragma_tok)
> +		      tok = cp_lexer_consume_token (parser->lexer);
> +		    unroll = cp_parser_pragma_unroll (parser, tok);
> +		    tok = cp_lexer_peek_token (the_parser->lexer);
> +		    break;
> +		  }
> +		case PRAGMA_NOVECTOR:
> +		  {
> +		    if (tok != pragma_tok)
> +		      tok = cp_lexer_consume_token (parser->lexer);
> +		    novector = cp_parser_pragma_novector (parser, tok);
> +		    tok = cp_lexer_peek_token (the_parser->lexer);
> +		    break;
> +		  }
> +		default:
> +		  gcc_unreachable ();

This unreachable seems to assert that if a pragma follows one of these 
pragmas, it must be another one of these pragmas?  That seems wrong; 
instead of hitting gcc_unreachable() in that case we should fall through 
to the diagnostic below.

I also think you could factor the peek out of the switch.

> diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma.cc
> new file mode 100644
> index 0000000000000000000000000000000000000000..cd5fb7ba9d4806f4d5e484ec68c707ea0e28ad7c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma.cc
> @@ -0,0 +1,68 @@
> +/* { dg-skip-if "incorrect syntax for c++98" { *-*-* } { "-std=c++98" } { "" } } */
> +/* { dg-do compile } */

This is usually expressed as { dg-do compile { target c++11 } }

Or you could run in C++98 mode but guard the range-for tests with
#if __cpp_range_based_for

Jason