Patchwork libcpp: registering both a pragma and a pragma namespace with the same name

login
register
mail settings
Submitter Aldy Hernandez
Date April 10, 2013, 10:24 p.m.
Message ID <5165E691.8020907@redhat.com>
Download mbox | patch
Permalink /patch/235498/
State New
Headers show

Comments

Aldy Hernandez - April 10, 2013, 10:24 p.m.
Hi Tom.  Hi folks.

We've asked Balaji to rewrite the <#pragma simd> handling for cilkplus 
as we currently do for OMP, etc, in init_pragma().

The cilkplus branch currently has something like:

       cpp_register_deferred_pragma (parse_in, "simd", "",
                                   PRAGMA_SIMD_EMPTY, true, false);
       cpp_register_deferred_pragma (parse_in, "simd", "assert",
	                            PRAGMA_SIMD_ASSERT, true, false);
       cpp_register_deferred_pragma (parse_in, "simd", "noassert",
	                            PRAGMA_SIMD_NOASSERT, true, false);
       cpp_register_deferred_pragma (parse_in, "simd", "vectorlength",
	                            PRAGMA_SIMD_VECTORLENGTH, true, false);

Notice that #pragma simd can be both a pragma name space, and also a 
lone pragma with no arguments:

	#pragma simd assert
		-or-
	#pragma simd

It seems like the code in libcpp's do_pragma(), specifically disallows 
this.  If we're looking at a possible pragma name space, the next 
expected token is a CPP_NAME.

Is there a way to handle this scenario with the current infrastructure? 
  If not, is something like the attached (untested) patch reasonable?

Aldy
Andrew Pinski - April 11, 2013, 12:16 a.m.
On Wed, Apr 10, 2013 at 3:24 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi Tom.  Hi folks.
>
> We've asked Balaji to rewrite the <#pragma simd> handling for cilkplus as we
> currently do for OMP, etc, in init_pragma().
>
> The cilkplus branch currently has something like:
>
>       cpp_register_deferred_pragma (parse_in, "simd", "",
>                                   PRAGMA_SIMD_EMPTY, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "assert",
>                                     PRAGMA_SIMD_ASSERT, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "noassert",
>                                     PRAGMA_SIMD_NOASSERT, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "vectorlength",
>                                     PRAGMA_SIMD_VECTORLENGTH, true, false);

What about just registering simd as the pragma and then look for the
right keyword after that?  Like diagnostic is handled?

Thanks,
Andrew Pinski

>
> Notice that #pragma simd can be both a pragma name space, and also a lone
> pragma with no arguments:
>
>         #pragma simd assert
>                 -or-
>         #pragma simd
>
> It seems like the code in libcpp's do_pragma(), specifically disallows this.
> If we're looking at a possible pragma name space, the next expected token is
> a CPP_NAME.
>
> Is there a way to handle this scenario with the current infrastructure?  If
> not, is something like the attached (untested) patch reasonable?
>
> Aldy

On Wed, Apr 10, 2013 at 3:24 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi Tom.  Hi folks.
>
> We've asked Balaji to rewrite the <#pragma simd> handling for cilkplus as we
> currently do for OMP, etc, in init_pragma().
>
> The cilkplus branch currently has something like:
>
>       cpp_register_deferred_pragma (parse_in, "simd", "",
>                                   PRAGMA_SIMD_EMPTY, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "assert",
>                                     PRAGMA_SIMD_ASSERT, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "noassert",
>                                     PRAGMA_SIMD_NOASSERT, true, false);
>       cpp_register_deferred_pragma (parse_in, "simd", "vectorlength",
>                                     PRAGMA_SIMD_VECTORLENGTH, true, false);
>
> Notice that #pragma simd can be both a pragma name space, and also a lone
> pragma with no arguments:
>
>         #pragma simd assert
>                 -or-
>         #pragma simd
>
> It seems like the code in libcpp's do_pragma(), specifically disallows this.
> If we're looking at a possible pragma name space, the next expected token is
> a CPP_NAME.
>
> Is there a way to handle this scenario with the current infrastructure?  If
> not, is something like the attached (untested) patch reasonable?
>
> Aldy
Jakub Jelinek - April 11, 2013, 5:32 a.m.
On Wed, Apr 10, 2013 at 05:16:17PM -0700, Andrew Pinski wrote:
> On Wed, Apr 10, 2013 at 3:24 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > Hi Tom.  Hi folks.
> >
> > We've asked Balaji to rewrite the <#pragma simd> handling for cilkplus as we
> > currently do for OMP, etc, in init_pragma().
> >
> > The cilkplus branch currently has something like:
> >
> >       cpp_register_deferred_pragma (parse_in, "simd", "",
> >                                   PRAGMA_SIMD_EMPTY, true, false);
> >       cpp_register_deferred_pragma (parse_in, "simd", "assert",
> >                                     PRAGMA_SIMD_ASSERT, true, false);
> >       cpp_register_deferred_pragma (parse_in, "simd", "noassert",
> >                                     PRAGMA_SIMD_NOASSERT, true, false);
> >       cpp_register_deferred_pragma (parse_in, "simd", "vectorlength",
> >                                     PRAGMA_SIMD_VECTORLENGTH, true, false);
> 
> What about just registering simd as the pragma and then look for the
> right keyword after that?  Like diagnostic is handled?

Yeah, the above is definitely wrong.  Just
  if (flag_cilkplus)
    cpp_register_deferred_pragma (parse_in, NULL, "simd", PRAGMA_SIMD, true, false);
and parse the clauses in c/c-parser.c and cp/parser.c, look at how OpenMP
pragmas are parsed (those have the "omp" space, you just use NULL, otherwise
it is not any different).

Also check the standard whether cpp expansion is allowed or not and on what
exactly.  Like:
#define S simd
#define V vectorlength

#pragma simd vectorlength(8)
for (i = 0; i < 16; i++)
  ;

vs.

#pragma simd V(8)
for (i = 0; i < 16; i++)
  ;

vs.

#pragma S V(8)
for (i = 0; i < 16; i++)
  ;

(that's the two last arguments to cpp_register_deferred_pragma).

	Jakub
Aldy Hernandez - April 11, 2013, 12:10 p.m.
On 04/11/13 00:32, Jakub Jelinek wrote:
> On Wed, Apr 10, 2013 at 05:16:17PM -0700, Andrew Pinski wrote:
>> On Wed, Apr 10, 2013 at 3:24 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>>> Hi Tom.  Hi folks.
>>>
>>> We've asked Balaji to rewrite the <#pragma simd> handling for cilkplus as we
>>> currently do for OMP, etc, in init_pragma().
>>>
>>> The cilkplus branch currently has something like:
>>>
>>>        cpp_register_deferred_pragma (parse_in, "simd", "",
>>>                                    PRAGMA_SIMD_EMPTY, true, false);
>>>        cpp_register_deferred_pragma (parse_in, "simd", "assert",
>>>                                      PRAGMA_SIMD_ASSERT, true, false);
>>>        cpp_register_deferred_pragma (parse_in, "simd", "noassert",
>>>                                      PRAGMA_SIMD_NOASSERT, true, false);
>>>        cpp_register_deferred_pragma (parse_in, "simd", "vectorlength",
>>>                                      PRAGMA_SIMD_VECTORLENGTH, true, false);
>>
>> What about just registering simd as the pragma and then look for the
>> right keyword after that?  Like diagnostic is handled?
>
> Yeah, the above is definitely wrong.  Just
>    if (flag_cilkplus)
>      cpp_register_deferred_pragma (parse_in, NULL, "simd", PRAGMA_SIMD, true, false);

Well, the thing is that you can't just use NULL for "#pragma simd" as a 
pragma and then define "#pragma simd assert" to use "simd" as a pragma 
namespace.  I had already tried that:

   if (flag_enable_cilk)
     {
       cpp_register_deferred_pragma (parse_in, NULL, "simd",
                                   PRAGMA_SIMD_EMPTY, true, false);
       cpp_register_deferred_pragma (parse_in, "simd", "assert",
	                            PRAGMA_SIMD_ASSERT, true, false);
       // etc
     }

In which case, we ICE because we're registering simd as both a pragma 
and a pragma namespace:

     cpp_error (pfile, CPP_DL_ICE,
	       "registering \"%s\" as both a pragma and a pragma namespace",
	       NODE_NAME (node));

 > and parse the clauses in c/c-parser.c and cp/parser.c, look at how OpenMP
 > pragmas are parsed (those have the "omp" space, you just use NULL, 
otherwise
 > it is not any different).

I see Balaji is already parsing the simd pragmas in c_parser_pragma.
Jakub Jelinek - April 11, 2013, 12:36 p.m.
On Thu, Apr 11, 2013 at 07:10:46AM -0500, Aldy Hernandez wrote:
> >Yeah, the above is definitely wrong.  Just
> >   if (flag_cilkplus)
> >     cpp_register_deferred_pragma (parse_in, NULL, "simd", PRAGMA_SIMD, true, false);
> 
> Well, the thing is that you can't just use NULL for "#pragma simd"
> as a pragma and then define "#pragma simd assert" to use "simd" as a
> pragma namespace.  I had already tried that:

My understanding is that the pragma is #pragma simd, it isn't a namespace,
and assert, vectorlength etc. are clauses.
http://software.intel.com/sites/products/documentation/studio/composer/en-us/2011Update/compiler_c/cref_cls/common/cppref_pragma_simd.htm
#pragma simd [clause[ [,] clause]...]

Thus, you'd parse it as PRAGMA_SIMD, then you'd just parse all the
(optional) clauses for it.

Say for OpenMP 4.0 #pragma omp simd (which is IMHO different just by using
a non-NULL namespace).  Upon seeing PRAGMA_OMP_SIMD (in your case
PRAGMA_SIMD), the C++ parser in cp_parser_pragma just calls a function to
handle parsing of it, for PRAGMA_OMP_SIMD on gomp-4_0-branch that is
cp_parser_omp_construct (after checking that the pragma doesn't appear
outside of function context (== pragma_external).  That then just
calls cp_parser_omp_simd which uses a helper function
(cp_parser_omp_all_clauses) to parse the clauses and newline at the end
of the pragma line, then whatever else needs to be parsed after it (for
cycle nest).

	Jakub
Aldy Hernandez - April 11, 2013, 12:54 p.m.
On 04/11/13 07:36, Jakub Jelinek wrote:
> On Thu, Apr 11, 2013 at 07:10:46AM -0500, Aldy Hernandez wrote:
>>> Yeah, the above is definitely wrong.  Just
>>>    if (flag_cilkplus)
>>>      cpp_register_deferred_pragma (parse_in, NULL, "simd", PRAGMA_SIMD, true, false);
>>
>> Well, the thing is that you can't just use NULL for "#pragma simd"
>> as a pragma and then define "#pragma simd assert" to use "simd" as a
>> pragma namespace.  I had already tried that:
>
> My understanding is that the pragma is #pragma simd, it isn't a namespace,
> and assert, vectorlength etc. are clauses.
> http://software.intel.com/sites/products/documentation/studio/composer/en-us/2011Update/compiler_c/cref_cls/common/cppref_pragma_simd.htm
> #pragma simd [clause[ [,] clause]...]
>
> Thus, you'd parse it as PRAGMA_SIMD, then you'd just parse all the
> (optional) clauses for it.

Ughh, you are correct.  Sorry for the noise.

Balaji, I assume you have enough to continue?

Thanks y'all.
Aldy

>
> Say for OpenMP 4.0 #pragma omp simd (which is IMHO different just by using
> a non-NULL namespace).  Upon seeing PRAGMA_OMP_SIMD (in your case
> PRAGMA_SIMD), the C++ parser in cp_parser_pragma just calls a function to
> handle parsing of it, for PRAGMA_OMP_SIMD on gomp-4_0-branch that is
> cp_parser_omp_construct (after checking that the pragma doesn't appear
> outside of function context (== pragma_external).  That then just
> calls cp_parser_omp_simd which uses a helper function
> (cp_parser_omp_all_clauses) to parse the clauses and newline at the end
> of the pragma line, then whatever else needs to be parsed after it (for
> cycle nest).
>
> 	Jakub
>
Iyer, Balaji V - April 11, 2013, 2:16 p.m.
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Aldy Hernandez
> Sent: Thursday, April 11, 2013 8:55 AM
> To: Jakub Jelinek
> Cc: Andrew Pinski; Tom Tromey; Iyer, Balaji V; gcc-patches
> Subject: Re: libcpp: registering both a pragma and a pragma namespace with the
> same name
> 
> On 04/11/13 07:36, Jakub Jelinek wrote:
> > On Thu, Apr 11, 2013 at 07:10:46AM -0500, Aldy Hernandez wrote:
> >>> Yeah, the above is definitely wrong.  Just
> >>>    if (flag_cilkplus)
> >>>      cpp_register_deferred_pragma (parse_in, NULL, "simd",
> >>> PRAGMA_SIMD, true, false);
> >>
> >> Well, the thing is that you can't just use NULL for "#pragma simd"
> >> as a pragma and then define "#pragma simd assert" to use "simd" as a
> >> pragma namespace.  I had already tried that:
> >
> > My understanding is that the pragma is #pragma simd, it isn't a
> > namespace, and assert, vectorlength etc. are clauses.
> > http://software.intel.com/sites/products/documentation/studio/composer
> > /en-us/2011Update/compiler_c/cref_cls/common/cppref_pragma_simd.htm
> > #pragma simd [clause[ [,] clause]...]
> >
> > Thus, you'd parse it as PRAGMA_SIMD, then you'd just parse all the
> > (optional) clauses for it.
> 
> Ughh, you are correct.  Sorry for the noise.
> 
> Balaji, I assume you have enough to continue?

I believe so. Thank you Jakub, Andrew and Aldy!

Sincerely,

Balaji V. Iyer.

> 
> Thanks y'all.
> Aldy
> 
> >
> > Say for OpenMP 4.0 #pragma omp simd (which is IMHO different just by
> > using a non-NULL namespace).  Upon seeing PRAGMA_OMP_SIMD (in your
> > case PRAGMA_SIMD), the C++ parser in cp_parser_pragma just calls a
> > function to handle parsing of it, for PRAGMA_OMP_SIMD on
> > gomp-4_0-branch that is cp_parser_omp_construct (after checking that
> > the pragma doesn't appear outside of function context (==
> > pragma_external).  That then just calls cp_parser_omp_simd which uses
> > a helper function
> > (cp_parser_omp_all_clauses) to parse the clauses and newline at the
> > end of the pragma line, then whatever else needs to be parsed after it
> > (for cycle nest).
> >
> > 	Jakub
> >

Patch

diff --git a/libcpp/directives.c b/libcpp/directives.c
index 65b2034..d09d2a4 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1373,7 +1373,26 @@  do_pragma (cpp_reader *pfile)
 	  if (token->type == CPP_NAME)
 	    p = lookup_pragma_entry (p->u.space, token->val.node.node);
 	  else
-	    p = NULL;
+	    {
+	      /* See if we can handle pragmas that are defined both as
+		 a pragma namespace, and as an argumentless pragma.
+		 For example:
+
+		 #pragma simd vectorlength
+		 #pragma simd			// empty argument
+	      */
+	      if (token->type == CPP_EOF)
+		{
+		  const cpp_hashnode *node;
+		  node = cpp_lookup (pfile, UC "", 0);
+		  if (node)
+		    p = lookup_pragma_entry (p->u.space, node);
+		  else
+		    p = NULL;
+		}
+	      else
+		p = NULL;
+	    }
 	  if (allow_name_expansion)
 	    pfile->state.prevent_expansion++;
 	  count = 2;