Re: [PATCH 2/19][front-end] C/C++ front-end: add pragma GCC novector

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5427 bytes --]

On Fri, Jun 30, 2023, 12:18 PM Tamar Christina <Tamar.Christina@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
>
>