Hi Jason,
Thanks for the review. I only now realized I should have split them between C
and C++.
Will do so on the respins.
>
> On 6/28/23 09:41, 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
> > and C++ as gfortan does for FORTRAN and what ICX/ICX does for C and 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/c-family/ChangeLog:
> >
> > * c-pragma.h (enum pragma_kind): Add PRAGMA_NOVECTOR.
> > * c-pragma.cc (init_pragma): Use it.
> >
> > gcc/c/ChangeLog:
> >
> > * c-parser.cc (c_parser_while_statement, c_parser_do_statement,
> > c_parser_for_statement, c_parser_statement_after_labels,
> > c_parse_pragma_novector, c_parser_pragma): Wire through novector
> and
> > default to false.
>
> I'll let the C maintainers review the C changes.
>
> > 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.
> > * tree-core.h (struct tree_base): Add lang_flag_7 and reduce spare0.
> > * tree.h (TREE_LANG_FLAG_7): New.
>
> This doesn't seem necessary; I think only flags 1 and 6 are currently used in
> RANGE_FOR_STMT.
Ah fair, I thought every option needed to occupy a specific bit. I'll try to
re-use one.
>
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/vect/vect-novector-pragma.cc: New test.
> > * gcc.dg/vect/vect-novector-pragma.c: New test.
> >
> > --- inline copy of patch --
> >...
> > @@ -13594,7 +13595,8 @@ cp_parser_condition (cp_parser* parser)
> > not included. */
> >
> > static tree
> > -cp_parser_for (cp_parser *parser, bool ivdep, unsigned short unroll)
> > +cp_parser_for (cp_parser *parser, bool ivdep, unsigned short unroll,
> > + bool novector)
>
> I wonder about combining the ivdep and novector parameters here and in
> other functions? Up to you.
As in, combine them in e.g. a struct?
>
> > @@ -49613,17 +49633,33 @@ cp_parser_pragma (cp_parser *parser,
> enum pragma_context context, bool *if_p)
> > break;
> > }
> > const bool ivdep = cp_parser_pragma_ivdep (parser, pragma_tok);
> > - unsigned short unroll;
> > + unsigned short unroll = 0;
> > + bool novector = false;
> > cp_token *tok = cp_lexer_peek_token (the_parser->lexer);
> > - if (tok->type == CPP_PRAGMA
> > - && cp_parser_pragma_kind (tok) == PRAGMA_UNROLL)
> > +
> > + while (tok->type == CPP_PRAGMA)
> > {
> > - tok = cp_lexer_consume_token (parser->lexer);
> > - unroll = cp_parser_pragma_unroll (parser, tok);
> > - tok = cp_lexer_peek_token (the_parser->lexer);
> > + switch (cp_parser_pragma_kind (tok))
> > + {
> > + case PRAGMA_UNROLL:
> > + {
> > + 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:
> > + {
> > + 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 ();
> > + }
> > }
>
> Repeating this pattern three times for the three related pragmas is too much;
> please combine the three cases into one.
Sure, I had some trouble combing them before because of the initial token being
consumed, but think I know a way.
Thanks for the review, will send updated split patch Monday.
Cheers,
Tamar
>
> Jason
