On Fri, Jun 30, 2023, 12:18 PM Tamar Christina <tamar.christ...@arm.com>
wrote:
> 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?
>
I was thinking in an int or enum.
>
> > > @@ -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
>
>
