[PING][GOMP4][PATCH] SIMD-enabled functions (formerly Elemental functions) for C++

[Iyer, Balaji V]

Hello Everyone,
Did anyone get a chance to look into this?

Thanks,

Balaji V. Iyer.

> -Original Message-
> From: Iyer, Balaji V
> Sent: Monday, December 23, 2013 11:51 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Jakub Jelinek
> Subject: [PING][GOMP4][PATCH] SIMD-enabled functions (formerly
> Elemental functions) for C++
> 
> Ping!
> 
> -Balaji V. Iyer.
> 
> > -Original Message-
> > From: Iyer, Balaji V
> > Sent: Thursday, December 19, 2013 1:12 PM
> > To: Jakub Jelinek
> > Cc: 'Aldy Hernandez (al...@redhat.com)'; 'gcc-patches@gcc.gnu.org'
> > Subject: RE: [GOMP4][PATCH] SIMD-enabled functions (formerly
> Elemental
> > functions) for C++
> >
> > Hi Jakub,
> > Attached, please find a fixed patch. I have answered your questions
> > below. Is this OK for trunk?
> >
> > Here are the ChangeLog entries:
> > Gcc/cp/ChangeLog
> > 2013-12-19  Balaji V. Iyer  
> >
> > * parser.c (cp_parser_direct_declarator): When Cilk Plus is enabled
> > see if there is an attribute after function decl.  If so, then
> > parse them now.
> > (cp_parser_late_return_type_opt): Handle parsing of Cilk Plus SIMD
> > enabled function late parsing.
> > (cp_parser_gnu_attribute_list): Parse all the tokens for the vector
> > attribute for a SIMD-enabled function.
> > (cp_parser_omp_all_clauses): Skip parsing to the end of pragma when
> > the function is used by SIMD-enabled function (indicated by NULL
> > pragma token).   Added 3 new clauses: PRAGMA_CILK_CLAUSE_MASK,
> > PRAGMA_CILK_CLAUSE_NOMASK and
> > PRAGMA_CILK_CLAUSE_VECTORLENGTH
> > (cp_parser_cilk_simd_vectorlength): Modified this function to handle
> > vectorlength clause in SIMD-enabled function and #pragma SIMD's
> > vectorlength clause.  Added a new bool parameter to differentiate
> > between the two.
> > (cp_parser_cilk_simd_fn_vector_attrs): New function.
> > (is_cilkplus_vector_p): Likewise.
> > (cp_parser_late_parsing_elem_fn_info): Likewise.
> > (cp_parser_omp_clause_name): Added a check for "mask," "nomask"
> > and "vectorlength" clauses when Cilk Plus is enabled.
> > (cp_parser_omp_clause_linear): Added a new parameter of type bool
> > and emit a sorry message when step size is a parameter.
> > * parser.h (cp_parser::cilk_simd_fn_info): New field.
> >
> > Testsuite/ChangeLog
> > 2013-12-19  Balaji V. Iyer  
> >
> > * g++.dg/cilk-plus/cilk-plus.exp: Called the C/C++ common tests for
> > SIMD enabled function.
> > * g++.dg/cilk-plus/ef_test.C: New test.
> > * c-c++-common/cilk-plus/vlength_errors.c: Added new dg-error tags
> > to differenciate C error messages from C++ ones.
> >
> > Thanks,
> >
> > Balaji V. Iyer.
> >
> > > -Original Message-
> > > From: Jakub Jelinek [mailto:ja...@redhat.com]
> > > Sent: Thursday, December 19, 2013 2:23 AM
> > > To: Iyer, Balaji V
> > > Cc: 'Aldy Hernandez (al...@redhat.com)'; 'gcc-patches@gcc.gnu.org'
> > > Subject: Re: [GOMP4][PATCH] SIMD-enabled functions (formerly
> > Elemental
> > > functions) for C++
> > >
> > > On Wed, Dec 18, 2013 at 11:36:04PM +, Iyer, Balaji V wrote:
> > > > --- a/gcc/cp/decl2.c
> > > > +++ b/gcc/cp/decl2.c
> > > > @@ -1124,6 +1124,10 @@ is_late_template_attribute (tree attr, tree
> > decl)
> > > >&& is_attribute_p ("omp declare simd", name))
> > > >  return true;
> > > >
> > > > +  /* Ditto as above for Cilk Plus SIMD-enabled function attributes.
> > > > + */  if (flag_enable_cilkplus && is_attribute_p ("cilk simd
> > > > + function",
> > > name))
> > > > +return true;
> > >
> > > Why?  It doesn't have any argument, why it should be processed late?
> > >
> >
> > Fixed.
> >
> > > > @@ -17097,6 +17102,14 @@ cp_parser_direct_declarator (cp_parser*
> > > > parser,
> > > >
> > > >   attrs = cp_parser_std_attribute_spec_seq (parser);
> > > >
> > > > + /* In here, we handle cases where attribute is used 
> > > > after
> > > > +the function declaration.  For example:
> 

[PING][GOMP4][PATCH] SIMD-enabled functions (formerly Elemental functions) for C++

[Iyer, Balaji V]

Ping!

-Balaji V. Iyer.

> -Original Message-
> From: Iyer, Balaji V
> Sent: Thursday, December 19, 2013 1:12 PM
> To: Jakub Jelinek
> Cc: 'Aldy Hernandez (al...@redhat.com)'; 'gcc-patches@gcc.gnu.org'
> Subject: RE: [GOMP4][PATCH] SIMD-enabled functions (formerly Elemental
> functions) for C++
> 
> Hi Jakub,
>   Attached, please find a fixed patch. I have answered your questions
> below. Is this OK for trunk?
> 
> Here are the ChangeLog entries:
> Gcc/cp/ChangeLog
> 2013-12-19  Balaji V. Iyer  
> 
> * parser.c (cp_parser_direct_declarator): When Cilk Plus is enabled
> see if there is an attribute after function decl.  If so, then
> parse them now.
> (cp_parser_late_return_type_opt): Handle parsing of Cilk Plus SIMD
> enabled function late parsing.
> (cp_parser_gnu_attribute_list): Parse all the tokens for the vector
> attribute for a SIMD-enabled function.
> (cp_parser_omp_all_clauses): Skip parsing to the end of pragma when
> the function is used by SIMD-enabled function (indicated by NULL
> pragma token).   Added 3 new clauses: PRAGMA_CILK_CLAUSE_MASK,
> PRAGMA_CILK_CLAUSE_NOMASK and
> PRAGMA_CILK_CLAUSE_VECTORLENGTH
> (cp_parser_cilk_simd_vectorlength): Modified this function to handle
> vectorlength clause in SIMD-enabled function and #pragma SIMD's
> vectorlength clause.  Added a new bool parameter to differentiate
> between the two.
> (cp_parser_cilk_simd_fn_vector_attrs): New function.
> (is_cilkplus_vector_p): Likewise.
> (cp_parser_late_parsing_elem_fn_info): Likewise.
> (cp_parser_omp_clause_name): Added a check for "mask," "nomask"
> and "vectorlength" clauses when Cilk Plus is enabled.
> (cp_parser_omp_clause_linear): Added a new parameter of type bool
> and emit a sorry message when step size is a parameter.
> * parser.h (cp_parser::cilk_simd_fn_info): New field.
> 
> Testsuite/ChangeLog
> 2013-12-19  Balaji V. Iyer  
> 
> * g++.dg/cilk-plus/cilk-plus.exp: Called the C/C++ common tests for
> SIMD enabled function.
> * g++.dg/cilk-plus/ef_test.C: New test.
> * c-c++-common/cilk-plus/vlength_errors.c: Added new dg-error tags
> to differenciate C error messages from C++ ones.
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> > -Original Message-
> > From: Jakub Jelinek [mailto:ja...@redhat.com]
> > Sent: Thursday, December 19, 2013 2:23 AM
> > To: Iyer, Balaji V
> > Cc: 'Aldy Hernandez (al...@redhat.com)'; 'gcc-patches@gcc.gnu.org'
> > Subject: Re: [GOMP4][PATCH] SIMD-enabled functions (formerly
> Elemental
> > functions) for C++
> >
> > On Wed, Dec 18, 2013 at 11:36:04PM +, Iyer, Balaji V wrote:
> > > --- a/gcc/cp/decl2.c
> > > +++ b/gcc/cp/decl2.c
> > > @@ -1124,6 +1124,10 @@ is_late_template_attribute (tree attr, tree
> decl)
> > >&& is_attribute_p ("omp declare simd", name))
> > >  return true;
> > >
> > > +  /* Ditto as above for Cilk Plus SIMD-enabled function attributes.
> > > + */  if (flag_enable_cilkplus && is_attribute_p ("cilk simd
> > > + function",
> > name))
> > > +return true;
> >
> > Why?  It doesn't have any argument, why it should be processed late?
> >
> 
> Fixed.
> 
> > > @@ -17097,6 +17102,14 @@ cp_parser_direct_declarator (cp_parser*
> > > parser,
> > >
> > > attrs = cp_parser_std_attribute_spec_seq (parser);
> > >
> > > +   /* In here, we handle cases where attribute is used after
> > > +  the function declaration.  For example:
> > > +  void func (int x) __attribute__((vector(..)));  */
> > > +   if (flag_enable_cilkplus
> > > +   && cp_lexer_next_token_is_keyword (parser->lexer,
> > > +  RID_ATTRIBUTE))
> > > + attrs = chainon (cp_parser_gnu_attributes_opt (parser),
> > > +  attrs);
> > > late_return = (cp_parser_late_return_type_opt
> > >(parser, declarator,
> > > memfn ? cv_quals : -1));
> >
> > Doesn't this change the grammar (for all attributes, not just Cilk+
> > specific
> > ones) just based on whether -fcilkplus has been specified or not?
> >
> 
> OK. Fixed this by making it parse tentatively (sort of similar to how you 
> parse
> attributes after labels (line #9584))
> 
> > > @@ -17820,10 +17833,14 @@ cp_parser_late_return_type_opt
> > (cp_parser* parser, cp_declarator *declarator,
> > >&& declarator
> > >&& declarator->kind == cdk_id);
> > >
> > > +  bool cilk_simd_fn_vector_p = (parser->cilk_simd_fn_info
> > > +&& declarator
> > > +&& declarator->kind == cdk_id);
> >
> > Formatting looks wrong, put = on the next line and align && right
> > below parser.
> 
> Fixed.
> 
> > > +
> > > +cp_omp_declare_simd_data info;
> >

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Joseph S. Myers]

On Mon, 16 Dec 2013, Jakub Jelinek wrote:

> On Mon, Dec 16, 2013 at 09:41:43PM +, Iyer, Balaji V wrote:
> > --- gcc/c/c-parser.c(revision 205759)
> > +++ gcc/c/c-parser.c(working copy)
> > @@ -208,6 +208,12 @@
> >/* True if we are in a context where the Objective-C "Property attribute"
> >   keywords are valid.  */
> >BOOL_BITFIELD objc_property_attr_context : 1;
> > +
> > +  /* Cilk Plus specific parser/lexer information.  */
> > +
> > +  /* Buffer to hold all the tokens from parsing the vector attribute for 
> > the
> > + SIMD-enabled functions (formerly known as elemental functions).  */
> > +  vec  *cilk_simd_fn_tokens;
> >  } c_parser;
> 
> Joseph, is this ok for you?

Fine with me.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Wed, Dec 18, 2013 at 02:32:54PM +, Iyer, Balaji V wrote:
> > Yes, though I still want to optimize it a little bit (generate thunks and/or
> > aliases when desirable/possible), but that only affects exported 
> > entry-points
> > for OpenMP, for Cilk+ the code matches more the Intel ABI paper and
> > generates only one ISA variant (and expects to parse processor clause for
> > other ISA variants), rather than emitting all 3.
> 
> So, install it into gomp4 branch then?

No, please test against trunk and commit there.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: Jakub Jelinek [mailto:ja...@zalov.cz]
> Sent: Wednesday, December 18, 2013 1:31 AM
> To: Iyer, Balaji V
> Cc: Joseph S. Myers; Aldy Hernandez (al...@redhat.com); 'gcc-
> patc...@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On Tue, Dec 17, 2013 at 11:38:48PM +, Iyer, Balaji V wrote:
> > > What I meant is
> > >   if (((mask >> PRAGMA_CILK_CLAUSE_VECTORLENGTH) & 1) != 0)
> > > is_cilk_simd_fn = true;
> > > (note, for 32-bit HWI targets, omp_clause_mask is a class and not
> > > all arithmetic is actually supported on it, so better limit yourself
> > > to forms used elsewhere already).
> > >
> >
> > I have a better idea.. The where string, if it is "SIMD-enabled
> > functions attribute" will indicate that it is a Cilk Plus SIMD-enabled 
> > function.
> > So, if I do a check for that, then I don't have to do any of this mask
> > anding.
> >
> > This is what I am talking about:
> >
> >   if (where && !strcmp (where, "SIMD-enabled functions attribute"))
> > is_cilk_simd_fn = false;
> 
> But this is more expensive and the string really is meant for diagnostics
> messages, so I'd strongly prefer the above mask check instead.
> Ok with that change.
> 

OK, will make this fix.

> > From what I understood, all the #pragma omp declare simd work are
> pushed into trunk right?
> 
> Yes, though I still want to optimize it a little bit (generate thunks and/or
> aliases when desirable/possible), but that only affects exported entry-points
> for OpenMP, for Cilk+ the code matches more the Intel ABI paper and
> generates only one ISA variant (and expects to parse processor clause for
> other ISA variants), rather than emitting all 3.

So, install it into gomp4 branch then?

> 
>   Jakub

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Tue, Dec 17, 2013 at 11:38:48PM +, Iyer, Balaji V wrote:
> > What I meant is
> >   if (((mask >> PRAGMA_CILK_CLAUSE_VECTORLENGTH) & 1) != 0)
> > is_cilk_simd_fn = true;
> > (note, for 32-bit HWI targets, omp_clause_mask is a class and not all
> > arithmetic is actually supported on it, so better limit yourself to forms 
> > used
> > elsewhere already).
> > 
> 
> I have a better idea.. The where string, if it is "SIMD-enabled functions
> attribute" will indicate that it is a Cilk Plus SIMD-enabled function. 
> So, if I do a check for that, then I don't have to do any of this mask
> anding.
> 
> This is what I am talking about:
> 
>   if (where && !strcmp (where, "SIMD-enabled functions attribute"))
> is_cilk_simd_fn = false;

But this is more expensive and the string really is meant for diagnostics
messages, so I'd strongly prefer the above mask check instead.
Ok with that change.

> From what I understood, all the #pragma omp declare simd work are pushed into 
> trunk right?

Yes, though I still want to optimize it a little bit (generate thunks
and/or aliases when desirable/possible), but that only affects exported
entry-points for OpenMP, for Cilk+ the code matches more the Intel ABI
paper and generates only one ISA variant (and expects to parse processor
clause for other ISA variants), rather than emitting all 3.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Jakub Jelinek
> Sent: Tuesday, December 17, 2013 4:26 PM
> To: Iyer, Balaji V
> Cc: Joseph S. Myers; Aldy Hernandez (al...@redhat.com); 'gcc-
> patc...@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On Tue, Dec 17, 2013 at 09:13:05PM +, Iyer, Balaji V wrote:
> > @@ -10418,6 +10528,12 @@
> >step = c_parser_expression (parser).value;
> >mark_exp_read (step);
> >step = c_fully_fold (step, false, NULL);
> > +  if (is_cilk_simd_fn && TREE_CODE (step) == PARM_DECL)
> > +   {
> > + sorry ("using parameters for % step is not supported yet "
> > +"in this release");
> 
> I meant actually
>   sorry ("using parameters for % step is not supported yet");
> 
> > @@ -10933,8 +11051,14 @@
> >   c_name = "aligned";
> >   break;
> > case PRAGMA_OMP_CLAUSE_LINEAR:
> > - clauses = c_parser_omp_clause_linear (parser, clauses);
> > - c_name = "linear";
> > + {
> > +   bool is_cilk_simd_fn = false;
> > +   if ((mask & PRAGMA_CILK_CLAUSE_VECTORLENGTH) == 0)
> > + is_cilk_simd_fn = true;
> 
> I don't think this will work.  PRAGMA_CILK_CLAUSE_VECTORLENGTH is
> something like 40 I think, so testing whether the mask doesn't include copyin
> and default clauses is not what you wanted probably.
> What I meant is
>   if (((mask >> PRAGMA_CILK_CLAUSE_VECTORLENGTH) & 1) != 0)
> is_cilk_simd_fn = true;
> (note, for 32-bit HWI targets, omp_clause_mask is a class and not all
> arithmetic is actually supported on it, so better limit yourself to forms used
> elsewhere already).
> 

I have a better idea.. The where string, if it is "SIMD-enabled functions 
attribute" will indicate that it is a Cilk Plus SIMD-enabled function.  So, if 
I do a check for that, then I don't have to do any of this mask anding.

This is what I am talking about:

  if (where && !strcmp (where, "SIMD-enabled functions attribute"))
is_cilk_simd_fn = false;



> > @@ -12754,10 +12882,20 @@
> >  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
> >vec clauses)
> >  {
> > +
> 
> Please remove this extra vertical space.
> 
> Otherwise looks good to me, just not sure where do you handle processor
> clause (or how Cilk+ simd clones specify the ISA they want to use).

Processor clause is an ICC only clause and thus GCC won't have it. The ISA 
clause (renamed to architecture), I am planning to handle in the next release 
along with parameters for step-size for linear


Can I install this? If so, can I push it to trunk? From what I understood, all 
the #pragma omp declare simd work are pushed into trunk right?

> 
>   Jakub
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 205759)
+++ gcc/c-family/c-common.c (working copy)
@@ -771,6 +771,8 @@
  handle_returns_nonnull_attribute, false },
   { "omp declare simd",   0, -1, true,  false, false,
  handle_omp_declare_simd_attribute, false },
+  { "cilk simd function", 0, -1, true,  false, false,
+ handle_omp_declare_simd_attribute, false },
   { "omp declare target", 0, 0, true, false, false,
  handle_omp_declare_target_attribute, false },
   { "bnd_variable_size",  0, 0, true,  false, false,
Index: gcc/c-family/c-pragma.h
===
--- gcc/c-family/c-pragma.h (revision 205759)
+++ gcc/c-family/c-pragma.h (working copy)
@@ -104,20 +104,21 @@
   PRAGMA_OMP_CLAUSE_THREAD_LIMIT,
   PRAGMA_OMP_CLAUSE_TO,
   PRAGMA_OMP_CLAUSE_UNIFORM,
-  PRAGMA_OMP_CLAUSE_UNTIED
+  PRAGMA_OMP_CLAUSE_UNTIED,
+  
+  /* Clauses for Cilk Plus SIMD-enabled function.  */
+  PRAGMA_CILK_CLAUSE_NOMASK,
+  PRAGMA_CILK_CLAUSE_MASK,
+  PRAGMA_CILK_CLAUSE_VECTORLENGTH,
+  PRAGMA_CILK_CLAUSE_NONE = PRAGMA_OMP_CLAUSE_NONE,
+  PRAGMA_CILK_CLAUSE_LINEAR = PRAGMA_OMP_CLAUSE_LINEAR,
+  PRAGMA_CILK_CLAUSE_PRIVATE = PRAGMA_OMP_CLAUSE_PRIVATE,
+  PRAGMA_CILK_CLAUSE_FIRSTPRIVATE = PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
+  PRAGMA_CILK_CLAUSE_LASTPRIVATE = PRAGMA_OMP_CLAUSE_LASTPRIVATE,
+  PRAGMA_CILK_CLAUSE_REDUCTION = PRAGMA_OMP_CLAUSE_REDUCTION,
+  PRAGMA_CILK_CLAUSE_UNIFORM = PRAGMA_OMP_CLAUSE_UNIFORM
 } pragma_omp_clause;
 
-/* All Cilk Plus #pragma omp clau

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Tue, Dec 17, 2013 at 09:13:05PM +, Iyer, Balaji V wrote:
> @@ -10418,6 +10528,12 @@
>step = c_parser_expression (parser).value;
>mark_exp_read (step);
>step = c_fully_fold (step, false, NULL);
> +  if (is_cilk_simd_fn && TREE_CODE (step) == PARM_DECL)
> + {
> +   sorry ("using parameters for % step is not supported yet "
> +  "in this release");

I meant actually
sorry ("using parameters for % step is not supported yet");

> @@ -10933,8 +11051,14 @@
> c_name = "aligned";
> break;
>   case PRAGMA_OMP_CLAUSE_LINEAR:
> -   clauses = c_parser_omp_clause_linear (parser, clauses);
> -   c_name = "linear";
> +   {
> + bool is_cilk_simd_fn = false;
> + if ((mask & PRAGMA_CILK_CLAUSE_VECTORLENGTH) == 0)
> +   is_cilk_simd_fn = true;

I don't think this will work.  PRAGMA_CILK_CLAUSE_VECTORLENGTH
is something like 40 I think, so testing whether the mask doesn't
include copyin and default clauses is not what you wanted probably.
What I meant is
  if (((mask >> PRAGMA_CILK_CLAUSE_VECTORLENGTH) & 1) != 0)
is_cilk_simd_fn = true;
(note, for 32-bit HWI targets, omp_clause_mask is a class and not
all arithmetic is actually supported on it, so better limit yourself
to forms used elsewhere already).

> @@ -12754,10 +12882,20 @@
>  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
>  vec clauses)
>  {
> +

Please remove this extra vertical space.

Otherwise looks good to me, just not sure where do you handle processor
clause (or how Cilk+ simd clones specify the ISA they want to use).

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Jakub,
Attached, please find a fixed patch. I have fixed all the issues you 
have mentioned below. I have also answered your questions below. Is this OK for 
trunk/branch?

Here are the ChangeLog entries:
gcc/ChangeLog
2013-12-17  Balaji V. Iyer  

* omp-low.c (simd_clone_clauses_extract): Replaced the string
"cilk simd elemental" with "cilk simd function."

gcc/c-family/ChangeLog
2013-12-17  Balaji V. Iyer  

* c-common.c (c_common_attribute_table): Added "cilk simd function"
attribute.
* c-pragma.h (enum pragma_cilk_clause): Remove.
(enum pragma_omp_clause):  Added the following fields:
PRAGMA_CILK_CLAUSE_NOMASK, PRAGMA_CILK_CLAUSE_MASK,
PRAGMA_CILK_CLAUSE_VECTORLENGTH, PRAGMA_CILK_CLAUSE_NONE,
PRAGMA_CILK_CLAUSE_LINEAR, PRAGMA_CILK_CLAUSE_PRIVATE,
PRAGMA_CILK_CLAUSE_FIRSTPRIVATE, PRAGMA_CILK_CLAUSE_LASTPRIVATE,
PRAGMA_CILK_CLAUSE_UNIFORM.

gcc/c/ChangeLog
2013-12-17  Balaji V. Iyer  

* c-parser.c (struct c_parser::cilk_simd_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if cilk_simd_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_cilk_clause_vectorlength): Modified function to be shared
between SIMD-enabled functions and #pragma simd.  Added new parameter.
(c_parser_cilk_all_clauses): Modified the usage of the function
c_parser_cilk_clause_vectorlength as mentioned above.
(c_parser_cilk_simd_fn_vector_attrs): New function.
(c_finish_cilk_simd_fn_tokens): Likewise.
(is_cilkplus_vector_p): Likewise.
(c_parser_omp_clause_name): Added checking for "vectorlength,"
"nomask," and "mask" strings in clause name.
(c_parser_omp_all_clauses): Added 3 new case statements:
PRAGMA_CILK_CLAUSE_VECTORLENGTH, PRAGMA_CILK_CLAUSE_MASK and
PRAGMA_CILK_CLAUSE_NOMASK.
(c_parser_attributes): Added a cilk_simd_fn_tokens parameter.  Added a
check for vector attribute and if so call the function
c_parser_cilk_simd_fn_vector_attrs.  Also, when Cilk plus is enabled,
called the function c_finish_cilk_simd_fn_tokens.
(c_finish_omp_declare_simd): Added a check if cilk_simd_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.
(c_parser_omp_clause_linear): Added a new bool parm. is_cilk_simd_fn.
Added a check when step is a parameter and flag it as error.
(CILK_SIMD_FN_CLAUSE_MASK): New #define.
(c_parser_cilk_clause_name): Changed pragma_cilk_clause to
pragma_omp_clause.

gcc/cp/ChangeLog
2013-12-17  Balaji V. Iyer  

* parser.c (cp_parser_cilk_simd_clause_name): Changed cilk_clause_name
to omp_clause_name.

gcc/testsuite/ChangeLog 
2013-12-17  Balaji V. Iyer  

* c-c++-common/cilk-plus/SE/ef_test.c: New test.
* c-c++-common/cilk-plus/SE/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/SE/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error2.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error3.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.


Thanks,

Balaji V. Iyer.

> -Original Message-
> From: Jakub Jelinek [mailto:ja...@redhat.com]
> Sent: Tuesday, December 17, 2013 1:25 PM
> To: Iyer, Balaji V
> Cc: Joseph S. Myers; Aldy Hernandez (al...@redhat.com); 'gcc-
> patc...@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> Hi!
> 
> On Tue, Dec 17, 2013 at 05:23:43PM +, Iyer, Balaji V wrote:
> > +/* Returns name of the next clause in Cilk Plus SIMD-enabled function's
> > +   attribute.
> > +   If the clause is not recognized PRAGMA_OMP_CLAUSE_NONE is
> returned and
> > +   the token is not consumed.  Otherwise appropriate
> pragma_omp_clause is
> > +   returned and the token is consumed.  */
> > +
> > +static pragma_omp_clause
> > +c_parser_cilk_simd_fn_clause_name (c_parser *parser) {
> > +  pragma_omp_clause result = PRAGMA_OMP_CLAUSE_NONE;
> > +
> > +  if (c_parser_next_token_is_not (parser, CPP_NAME))
> > +return result;
> > +
> > +  const char *p = IDENTIFIER_POINTER (c_parser_peek_token
> > + (parser)->value);  if (!strcmp (p, "vectorlength"))
> > +result = PRAGMA_CILK_CLAUSE_VECTORLENGTH;  else if (!strcmp (p,
> > + "uniform"))
> > +result = PRAGMA_CILK_CLAUSE_UNIFORM;  else if (!strcmp (p,
> > + "linear"))
> > +result = PRAGMA_CILK

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

Hi!

On Tue, Dec 17, 2013 at 05:23:43PM +, Iyer, Balaji V wrote:
> +/* Returns name of the next clause in Cilk Plus SIMD-enabled function's
> +   attribute.
> +   If the clause is not recognized PRAGMA_OMP_CLAUSE_NONE is returned and
> +   the token is not consumed.  Otherwise appropriate pragma_omp_clause is
> +   returned and the token is consumed.  */
> +
> +static pragma_omp_clause
> +c_parser_cilk_simd_fn_clause_name (c_parser *parser)
> +{
> +  pragma_omp_clause result = PRAGMA_OMP_CLAUSE_NONE;
> +
> +  if (c_parser_next_token_is_not (parser, CPP_NAME))
> +return result;
> +  
> +  const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
> +  if (!strcmp (p, "vectorlength"))
> +result = PRAGMA_CILK_CLAUSE_VECTORLENGTH;
> +  else if (!strcmp (p, "uniform"))
> +result = PRAGMA_CILK_CLAUSE_UNIFORM;
> +  else if (!strcmp (p, "linear"))
> +result = PRAGMA_CILK_CLAUSE_LINEAR;
> +  else if (!strcmp (p, "mask"))
> +result = PRAGMA_CILK_CLAUSE_MASK;
> +  else if (!strcmp (p, "nomask"))
> +result = PRAGMA_CILK_CLAUSE_UNMASK;
> +
> +  if (result != PRAGMA_OMP_CLAUSE_NONE)
> +c_parser_consume_token (parser);
> +  return result;
> +}

No, this isn't what I meant.  I meant that you add the 3 new clause names
to c_parser_omp_clause_name (and use PRAGMA_CILK_* for those).

> +  if (token->type == CPP_NAME
> +   && TREE_CODE (token->value) == IDENTIFIER_NODE)   
> + if (!strcmp (IDENTIFIER_POINTER (token->value), "vectorlength"))
> +   {
> + if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))
> +   {
> + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
> + return;
> +   }
> + else
> +   continue;
> +   }

Why do you need this at all?  I'd expect you just remove this whole if and
the c_parser_cilk_clause_vectorlength function, and instead just parse
vectorlength normally when you see PRAGMA_CILK_CLAUSE_VECTORLENGTH.

> +   sorry ("using parameters for % step is not supported "
> +  "in this release");

... is not supported yet".

> -  c_kind = c_parser_omp_clause_name (parser);
> + 
> +  if (mask == CILK_SIMD_FN_CLAUSE_MASK)
> + c_kind = c_parser_cilk_simd_fn_clause_name (parser);
> +  else
> + c_kind = c_parser_omp_clause_name (parser);

Please revert this.

> @@ -10933,7 +11088,8 @@
> c_name = "aligned";
> break;
>   case PRAGMA_OMP_CLAUSE_LINEAR:
> -   clauses = c_parser_omp_clause_linear (parser, clauses);
> +   clauses = c_parser_omp_clause_linear
> + (parser, clauses, mask == CILK_SIMD_FN_CLAUSE_MASK);

Please test for some particular bit in the mask, not == on the masks.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Jakub,
Please see attached patch and my answers to your questions below.

Aldy, I have made a couple changes to #pragma simd routines, can you 
please give me your blessing on those?

Thanks,

Balaji V. Iyer.

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Jakub Jelinek
> Sent: Monday, December 16, 2013 5:01 PM
> To: Iyer, Balaji V; Joseph S. Myers
> Cc: Aldy Hernandez; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On Mon, Dec 16, 2013 at 09:41:43PM +, Iyer, Balaji V wrote:
> > --- gcc/c/c-parser.c(revision 205759)
> > +++ gcc/c/c-parser.c(working copy)
> > @@ -208,6 +208,12 @@
> >/* True if we are in a context where the Objective-C "Property attribute"
> >   keywords are valid.  */
> >BOOL_BITFIELD objc_property_attr_context : 1;
> > +
> > +  /* Cilk Plus specific parser/lexer information.  */
> > +
> > +  /* Buffer to hold all the tokens from parsing the vector attribute for 
> > the
> > + SIMD-enabled functions (formerly known as elemental functions).
> > + */  vec  *cilk_simd_fn_tokens;
> >  } c_parser;
> 
> Joseph, is this ok for you?
> 
> > +/* Returns true of NAME is an IDENTIFIER_NODE with identiifer "vector,"
> > +   "__vector" or "__vector__."  */
> > +
> > +static bool
> 
> static inline bool
> 

Fixed.

> > +is_cilkplus_vector_p (tree name)
> > +{
> > +  if (flag_enable_cilkplus
> > +  && (simple_cst_equal (name, get_identifier ("vector")) == 1
> > + || simple_cst_equal (name, get_identifier ("__vector")) == 1
> > + || simple_cst_equal (name, get_identifier ("__vector__")) == 1))
> > +return true;
> > +  return false;
> > +}
> 
> Why not just
>   return flag_enable_cilkplus && is_attribute_p ("vector", name); ?

Fixed.

> 
> > +#define CILK_SIMD_FN_CLAUSE_MASK   \
> > +   ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH)
>   \
> > +   | (OMP_CLAUSE_MASK_1 <<
> PRAGMA_OMP_CLAUSE_NOTINBRANCH))
> 
> I thought you'd instead add there PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK (or
> similar).
> 

Yes, I renamed them to PRAGMA_CILK_CLAUSE_*

> > +  if (token->type == CPP_NAME
> > + && TREE_CODE (token->value) == IDENTIFIER_NODE)
> > +   if (simple_cst_equal (token->value,
> > + get_identifier ("vectorlength")) == 1)
> 
> Why the simple_cst_equal + get_identifier?  I mean, strcmp on
> IDENTIFIER_POINTER should be cheaper, and done elsewhere in c-parser.c.
> 

Fixed.

> > + {
> > +   if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))
> 
> Why do you parse it here?  Just parse it when parsing other clauses, and only
> during parsing of vectorlength clause create OMP_CLAUSE_SAFELEN out of it
> (after all the verifications).
> 
> > +  if (is_cilk_simd_fn && TREE_CODE (step) == PARM_DECL)
> > +   {
> > + error_at (clause_loc, "using parameters for % step is "
> > +   "not supported in this release");
> > + step = integer_one_node;
> 
> That would be sorry, not error_at.
> 

Fixed.

> >here = c_parser_peek_token (parser)->location;
> > -  c_kind = c_parser_omp_clause_name (parser);
> > +
> > +  if (mask == CILK_SIMD_FN_CLAUSE_MASK)
> 
> Ugh, no.
> 
> > +   c_kind = c_parser_cilk_simd_fn_clause_name (parser);
> > +  else
> > +   c_kind = c_parser_omp_clause_name (parser);
> 
> Always parse it the same, just use PRAGMA_CILK_CLAUSE_* for the Cilk+
> specific clauses.

Well, if  I don't have a different "*_clause_name" function, then I have to 
modify the c_parser_omp_clause_name to add the support for things like 
"vectorlength", "mask and "unmask." Am I right?

Now, if I do that, then if we compile the following 2 lines:

#pragma omp declare simd vectorlength (4)
void foo () 

with -fcilkplus, they will be parsed correctly, when it should give an error.

> >
> >switch (c_kind)
> >

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: Jakub Jelinek [mailto:ja...@redhat.com]
> Sent: Tuesday, December 17, 2013 1:18 AM
> To: Iyer, Balaji V
> Cc: Joseph S. Myers; Aldy Hernandez; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On Tue, Dec 17, 2013 at 03:51:14AM +, Iyer, Balaji V wrote:
> > Hi Jakub,
> > I will work on this, but I need a couple clarifications about some of
> your comments. Please see below:
> >
> > > > +#define CILK_SIMD_FN_CLAUSE_MASK   \
> > > > +   ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 <<
> > > PRAGMA_OMP_CLAUSE_NOTINBRANCH))
> > >
> > > I thought you'd instead add there
> PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> > > PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK
> (or similar).
> > >
> >
> > I looked at OpenACC implementation and they seem to use the
> OMP_CLAUSE_* (line # 11174 in c-parser.c)
> 
> It uses just PRAGMA_OMP_CLAUSE_NONE, which really means no clauses at
> all (I
> think it is for now).
> 
> > Also, If I created CILK_CLAUSE_* variants, I have to re-create another
> function similar to c_parser_omp_all_clauses, whose workings will be
> identical to the c_parser_omp_all_clauses. Is that OK with you?
> 
> No, I'd remove enum pragma_cilk_clause altogether and fold it into the end
> of
> pragma_omp_clause, as:
>   PRAGMA_CILK_CLAUSE_VECTORLENGTH,
>   PRAGMA_CILK_CLAUSE_MASK,
>   PRAGMA_CILK_CLAUSE_NOMASK,
>   PRAGMA_CILK_CLAUSE_NONE = PRAGMA_OMP_CLAUSE_NONE,
>   PRAGMA_CILK_CLAUSE_LINEAR = PRAGMA_OMP_CLAUSE_LINEAR,
>   PRAGMA_CILK_CLAUSE_PRIVATE = PRAGMA_OMP_CLAUSE_PRIVATE,
>   PRAGMA_CILK_CLAUSE_FIRSTPRIVATE =
> PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
>   PRAGMA_CILK_CLAUSE_LASTPRIVATE =
> PRAGMA_OMP_CLAUSE_LASTPRIVATE,
>   PRAGMA_CILK_CLAUSE_REDUCTION =
> PRAGMA_OMP_CLAUSE_REDUCTION
> so that you can use it in the same bitmasks.
> 
> That way, you don't have to change anything in c_parser_omp_all_clauses,
> just add handling of the 3 clauses that don't have OpenMP counterparts.

I think it sort of makes sense to me now.  I will work on this.

Oh, VECTORLENGTH in SIMD-enabled function is same as SIMDLEN in OMP4

And,

MASK = INBRANCH
NOMASK = NOTINBRANCH.


Thanks,

Balaji V. Iyer.



> 
>   Jakub

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Thomas Schwinge]

Hi!

On Tue, 17 Dec 2013 11:27:51 +0100, Jakub Jelinek  wrote:
> On Tue, Dec 17, 2013 at 11:03:12AM +0100, Thomas Schwinge wrote:
> > My understanding/reasoning is that PRAGMA_OMP_* just literally represents
> > a parser token of a pragma line (see the one-to-one translation in
> > c-parser.c:c_parser_omp_clause_name, for example).  This means that
> > »#pragma omp parallel copyin ([...])« and »#pragma acc parallel copyin
> > ([...])« can share the same PRAGMA_OMP_CLAUSE_COPYIN, even though it
> > means something different to both of them; PRAGMA_OMP_CLAUSE_* alone
> > doesn't convey any meaning (apart from the token/"string" used in the
> > pragma line), and it gets its meaning only if interpreted as part of a
> > Open* construct/directive.  Just like many other tokens only get their
> > semantic meaning when parsed inside a specific language construct.  For
> > OpenACC, the disambiguation, that is, translation from
> > PRAGMA_OMP_CLAUSE_* to OMP_CLAUSE_*...
> > 
> > > That way, you don't have to change anything in c_parser_omp_all_clauses,
> > > just add handling of the 3 clauses that don't have OpenMP counterparts.
> > 
> > ... then indeed happens in a new c_parser_oacc_all_clauses, which parses
> > all the applicable PRAGMA_OMP_CLAUSE_* according to the OpenACC
> > semantics.
> 
> Unlike OpenACC, Cilk+ for the vector attribute has pretty much the OpenMP
> syntax, with just a few exceptions (in particular, 3 clauses have different
> names (and there are extra requirements for vectorlength?) and for linear
> there is an extension on the Cilk+ side.  So, duplicating the
> c_parser_*all_clauses in that case is IMHO not needed, the mask specifies
> which clauses are allowed in the particular construct and the only case
> which needs disambiguation (linear clauses' step) can be disambiguated
> by checking if some Cilk+ specific clause is in the mask (already the
> clause splitting code uses such tests).

Right, if they're that similar, I agree that's the way to go.

> If OpenACC clauses have different names from the OpenMP/Cilk+ ones, I don't
> see why you would need a new *_all_clauses function, just supply a different
> mask

OpenACC clauses share some clause names and their semantics with OpenMP,
and some new ones, but there are also several that have the same name
(such as said copyin) but with a different meaning.

For example for the copyin case, my understanding is that I can either
re-use the existing PRAGMA_OMP_CLAUSE_COPYIN but then need to interpret
it differently in an OpenACC context, and thus need a new
c_parser_oacc_all_clauses (or add some ugly »if ([inside OpenACC
directive]) { [...] } else { [existing OpenMP code]}«.  Alternatively, I
have to add a new PRAGMA_OMP_CLAUSE_OACC_COPYIN, and can then use the
existing c_parser_omp_all_clauses.  Due to the perceived one-to-one
mapping between the existing PRAGMA_OMP_CLAUSE_* and the tokens (such as
"copyin"), the former seemed more appropriate to me, as detailed above.


> > For example, said PRAGMA_OMP_CLAUSE_COPYIN is translated to
> > OMP_CLAUSE_MAP with OMP_CLAUSE_MAP_TO, and the (new)
> > PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYOUT (which is only interpreted/valid
> > inside OpenACC contexts) is translated to OMP_CLAUSE_MAP with (new)
> > OMP_CLAUSE_MAP_PRESENT_OR_FROM (which is only interpreted/valid inside
> > OpenACC contexts).
> 
> This is weird, because present or {alloc,from,to,fromto} is the OpenMP
> behavior, so I'd expect you would be adding a bit for the other, non-OpenMP
> compatible behavior instead.

I'm currently working on this.  Per my current understanding, in the
front end and middle end (gimplify.c, omp-low.c), the handling of
present_or_* vs. their "normal" variants is the same for OpenACC, and the
difference is only apparent once they're interpreted in the runtime
library, which is free to decide which way round to interpret the
present_or bit.  Anyway, you're absolutely right that I should preserve
the same meaning of the map kinds, such as OMP_CLAUSE_MAP_FROM, for both
the OpenACC (meaning present_or_copyout) and OpenMP (meaning map from)
entry points, to avoid confusion at this level.  Especially if their
semantics are exactly the same (to be checked), and especially given
we're trying to converge on the shared infrastructure, as I'm advocating
it myself...


I hope to post some code soon, which will hopefully help to display my
ideas.


Grüße,
 Thomas


pgpQt0tAbCTgn.pgp
Description: PGP signature

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Tue, Dec 17, 2013 at 11:03:12AM +0100, Thomas Schwinge wrote:
> > > Also, If I created CILK_CLAUSE_* variants, I have to re-create another 
> > > function similar to c_parser_omp_all_clauses, whose workings will be 
> > > identical to the c_parser_omp_all_clauses. Is that OK with you?
> > 
> > No, I'd remove enum pragma_cilk_clause altogether and fold it into the end 
> > of
> > pragma_omp_clause, as:
> >   PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> >   PRAGMA_CILK_CLAUSE_MASK,
> >   PRAGMA_CILK_CLAUSE_NOMASK,
> >   PRAGMA_CILK_CLAUSE_NONE = PRAGMA_OMP_CLAUSE_NONE,
> >   PRAGMA_CILK_CLAUSE_LINEAR = PRAGMA_OMP_CLAUSE_LINEAR,
> >   PRAGMA_CILK_CLAUSE_PRIVATE = PRAGMA_OMP_CLAUSE_PRIVATE,
> >   PRAGMA_CILK_CLAUSE_FIRSTPRIVATE = PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
> >   PRAGMA_CILK_CLAUSE_LASTPRIVATE = PRAGMA_OMP_CLAUSE_LASTPRIVATE,
> >   PRAGMA_CILK_CLAUSE_REDUCTION = PRAGMA_OMP_CLAUSE_REDUCTION
> > so that you can use it in the same bitmasks.
> 
> Hmm, indeed my inclination (and what I have implemented in my working
> trees) has been to literally re-use the existing PRAGMA_OMP_* ones for
> OpenACC, without adding new aliasesm, and extend/add new ones as
> required.

The aliases would be only if they are needed, I understood that
those are already used in #pragma simd parsing.  Surely, if they are renamed
to PRAGMA_OMP_* where they have counterparts, the aliases aren't needed.

> My understanding/reasoning is that PRAGMA_OMP_* just literally represents
> a parser token of a pragma line (see the one-to-one translation in
> c-parser.c:c_parser_omp_clause_name, for example).  This means that
> »#pragma omp parallel copyin ([...])« and »#pragma acc parallel copyin
> ([...])« can share the same PRAGMA_OMP_CLAUSE_COPYIN, even though it
> means something different to both of them; PRAGMA_OMP_CLAUSE_* alone
> doesn't convey any meaning (apart from the token/"string" used in the
> pragma line), and it gets its meaning only if interpreted as part of a
> Open* construct/directive.  Just like many other tokens only get their
> semantic meaning when parsed inside a specific language construct.  For
> OpenACC, the disambiguation, that is, translation from
> PRAGMA_OMP_CLAUSE_* to OMP_CLAUSE_*...
> 
> > That way, you don't have to change anything in c_parser_omp_all_clauses,
> > just add handling of the 3 clauses that don't have OpenMP counterparts.
> 
> ... then indeed happens in a new c_parser_oacc_all_clauses, which parses
> all the applicable PRAGMA_OMP_CLAUSE_* according to the OpenACC
> semantics.

Unlike OpenACC, Cilk+ for the vector attribute has pretty much the OpenMP
syntax, with just a few exceptions (in particular, 3 clauses have different
names (and there are extra requirements for vectorlength?) and for linear
there is an extension on the Cilk+ side.  So, duplicating the
c_parser_*all_clauses in that case is IMHO not needed, the mask specifies
which clauses are allowed in the particular construct and the only case
which needs disambiguation (linear clauses' step) can be disambiguated
by checking if some Cilk+ specific clause is in the mask (already the
clause splitting code uses such tests).

If OpenACC clauses have different names from the OpenMP/Cilk+ ones, I don't
see why you would need a new *_all_clauses function, just supply a different
mask (unless we run out of the 64-bits in the bitmask, then we'd need extra
steps, perhaps start using real bitmasks or something).

> For example, said PRAGMA_OMP_CLAUSE_COPYIN is translated to
> OMP_CLAUSE_MAP with OMP_CLAUSE_MAP_TO, and the (new)
> PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYOUT (which is only interpreted/valid
> inside OpenACC contexts) is translated to OMP_CLAUSE_MAP with (new)
> OMP_CLAUSE_MAP_PRESENT_OR_FROM (which is only interpreted/valid inside
> OpenACC contexts).

This is weird, because present or {alloc,from,to,fromto} is the OpenMP
behavior, so I'd expect you would be adding a bit for the other, non-OpenMP
compatible behavior instead.

Jakub

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Thomas Schwinge]

Hi!

For reference, here's my rationale for OpenACC on this topic:

On Tue, 17 Dec 2013 07:17:31 +0100, Jakub Jelinek  wrote:
> On Tue, Dec 17, 2013 at 03:51:14AM +, Iyer, Balaji V wrote:
> > Hi Jakub,   
> > I will work on this, but I need a couple clarifications about some of 
> > your comments. Please see below:
> > 
> > > > +#define CILK_SIMD_FN_CLAUSE_MASK   \
> > > > +   ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH)
> > >   \
> > > > +   | (OMP_CLAUSE_MASK_1 <<
> > > PRAGMA_OMP_CLAUSE_NOTINBRANCH))
> > > 
> > > I thought you'd instead add there PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> > > PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK (or
> > > similar).
> > > 
> > 
> > I looked at OpenACC implementation and they seem to use the OMP_CLAUSE_* 
> > (line # 11174 in c-parser.c)
> 
> It uses just PRAGMA_OMP_CLAUSE_NONE, which really means no clauses at all (I
> think it is for now).

Right, that's only for now.

> > Also, If I created CILK_CLAUSE_* variants, I have to re-create another 
> > function similar to c_parser_omp_all_clauses, whose workings will be 
> > identical to the c_parser_omp_all_clauses. Is that OK with you?
> 
> No, I'd remove enum pragma_cilk_clause altogether and fold it into the end of
> pragma_omp_clause, as:
>   PRAGMA_CILK_CLAUSE_VECTORLENGTH,
>   PRAGMA_CILK_CLAUSE_MASK,
>   PRAGMA_CILK_CLAUSE_NOMASK,
>   PRAGMA_CILK_CLAUSE_NONE = PRAGMA_OMP_CLAUSE_NONE,
>   PRAGMA_CILK_CLAUSE_LINEAR = PRAGMA_OMP_CLAUSE_LINEAR,
>   PRAGMA_CILK_CLAUSE_PRIVATE = PRAGMA_OMP_CLAUSE_PRIVATE,
>   PRAGMA_CILK_CLAUSE_FIRSTPRIVATE = PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
>   PRAGMA_CILK_CLAUSE_LASTPRIVATE = PRAGMA_OMP_CLAUSE_LASTPRIVATE,
>   PRAGMA_CILK_CLAUSE_REDUCTION = PRAGMA_OMP_CLAUSE_REDUCTION
> so that you can use it in the same bitmasks.

Hmm, indeed my inclination (and what I have implemented in my working
trees) has been to literally re-use the existing PRAGMA_OMP_* ones for
OpenACC, without adding new aliasesm, and extend/add new ones as
required.

My understanding/reasoning is that PRAGMA_OMP_* just literally represents
a parser token of a pragma line (see the one-to-one translation in
c-parser.c:c_parser_omp_clause_name, for example).  This means that
»#pragma omp parallel copyin ([...])« and »#pragma acc parallel copyin
([...])« can share the same PRAGMA_OMP_CLAUSE_COPYIN, even though it
means something different to both of them; PRAGMA_OMP_CLAUSE_* alone
doesn't convey any meaning (apart from the token/"string" used in the
pragma line), and it gets its meaning only if interpreted as part of a
Open* construct/directive.  Just like many other tokens only get their
semantic meaning when parsed inside a specific language construct.  For
OpenACC, the disambiguation, that is, translation from
PRAGMA_OMP_CLAUSE_* to OMP_CLAUSE_*...

> That way, you don't have to change anything in c_parser_omp_all_clauses,
> just add handling of the 3 clauses that don't have OpenMP counterparts.

... then indeed happens in a new c_parser_oacc_all_clauses, which parses
all the applicable PRAGMA_OMP_CLAUSE_* according to the OpenACC
semantics.

For example, said PRAGMA_OMP_CLAUSE_COPYIN is translated to
OMP_CLAUSE_MAP with OMP_CLAUSE_MAP_TO, and the (new)
PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYOUT (which is only interpreted/valid
inside OpenACC contexts) is translated to OMP_CLAUSE_MAP with (new)
OMP_CLAUSE_MAP_PRESENT_OR_FROM (which is only interpreted/valid inside
OpenACC contexts).


Grüße,
 Thomas


pgpplSmD1S2Jc.pgp
Description: PGP signature

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Tue, Dec 17, 2013 at 03:51:14AM +, Iyer, Balaji V wrote:
> Hi Jakub, 
>   I will work on this, but I need a couple clarifications about some of 
> your comments. Please see below:
> 
> > > +#define CILK_SIMD_FN_CLAUSE_MASK \
> > > + ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)
> > \
> > > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)
> > \
> > > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)
> > \
> > > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH)
> > \
> > > + | (OMP_CLAUSE_MASK_1 <<
> > PRAGMA_OMP_CLAUSE_NOTINBRANCH))
> > 
> > I thought you'd instead add there PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> > PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK (or
> > similar).
> > 
> 
> I looked at OpenACC implementation and they seem to use the OMP_CLAUSE_* 
> (line # 11174 in c-parser.c)

It uses just PRAGMA_OMP_CLAUSE_NONE, which really means no clauses at all (I
think it is for now).

> Also, If I created CILK_CLAUSE_* variants, I have to re-create another 
> function similar to c_parser_omp_all_clauses, whose workings will be 
> identical to the c_parser_omp_all_clauses. Is that OK with you?

No, I'd remove enum pragma_cilk_clause altogether and fold it into the end of
pragma_omp_clause, as:
  PRAGMA_CILK_CLAUSE_VECTORLENGTH,
  PRAGMA_CILK_CLAUSE_MASK,
  PRAGMA_CILK_CLAUSE_NOMASK,
  PRAGMA_CILK_CLAUSE_NONE = PRAGMA_OMP_CLAUSE_NONE,
  PRAGMA_CILK_CLAUSE_LINEAR = PRAGMA_OMP_CLAUSE_LINEAR,
  PRAGMA_CILK_CLAUSE_PRIVATE = PRAGMA_OMP_CLAUSE_PRIVATE,
  PRAGMA_CILK_CLAUSE_FIRSTPRIVATE = PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
  PRAGMA_CILK_CLAUSE_LASTPRIVATE = PRAGMA_OMP_CLAUSE_LASTPRIVATE,
  PRAGMA_CILK_CLAUSE_REDUCTION = PRAGMA_OMP_CLAUSE_REDUCTION
so that you can use it in the same bitmasks.

That way, you don't have to change anything in c_parser_omp_all_clauses,
just add handling of the 3 clauses that don't have OpenMP counterparts.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Jakub,   
I will work on this, but I need a couple clarifications about some of 
your comments. Please see below:

> > +#define CILK_SIMD_FN_CLAUSE_MASK   \
> > +   ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)
>   \
> > +   | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH)
>   \
> > +   | (OMP_CLAUSE_MASK_1 <<
> PRAGMA_OMP_CLAUSE_NOTINBRANCH))
> 
> I thought you'd instead add there PRAGMA_CILK_CLAUSE_VECTORLENGTH,
> PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK (or
> similar).
> 

I looked at OpenACC implementation and they seem to use the OMP_CLAUSE_* (line 
# 11174 in c-parser.c)

Also, If I created CILK_CLAUSE_* variants, I have to re-create another function 
similar to c_parser_omp_all_clauses, whose workings will be identical to the 
c_parser_omp_all_clauses. Is that OK with you?

More below.

> > +  if (token->type == CPP_NAME
> > + && TREE_CODE (token->value) == IDENTIFIER_NODE)
> > +   if (simple_cst_equal (token->value,
> > + get_identifier ("vectorlength")) == 1)
> 
> Why the simple_cst_equal + get_identifier?  I mean, strcmp on
> IDENTIFIER_POINTER should be cheaper, and done elsewhere in c-parser.c.
> 
> > + {
> > +   if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))
> 
> Why do you parse it here?  Just parse it when parsing other clauses, and only
> during parsing of vectorlength clause create OMP_CLAUSE_SAFELEN out of it
> (after all the verifications).
> 
> >here = c_parser_peek_token (parser)->location;
> > -  c_kind = c_parser_omp_clause_name (parser);
> > +
> > +  if (mask == CILK_SIMD_FN_CLAUSE_MASK)
> 
> Ugh, no.
> 
> > +   c_kind = c_parser_cilk_simd_fn_clause_name (parser);
> > +  else
> > +   c_kind = c_parser_omp_clause_name (parser);
> 

... Having a separate clause_name function seem to work and will do the mapping 
from Cilk Plus clause to OMP's right before calling the functions to handle it.

> Always parse it the same, just use PRAGMA_CILK_CLAUSE_* for the Cilk+
> specific clauses.
> >
> >switch (c_kind)
> > {
> > @@ -10933,7 +11092,8 @@
> >   c_name = "aligned";
> >   break;
> > case PRAGMA_OMP_CLAUSE_LINEAR:
> > - clauses = c_parser_omp_clause_linear (parser, clauses);
> > + clauses = c_parser_omp_clause_linear
> > +   (parser, clauses, mask == CILK_SIMD_FN_CLAUSE_MASK);
> 
> Again, this is too ugly.  Perhaps check if (mask &
> PRAGMA_CILK_CLAUSE_VECTORLENGTH) != 0 or similar.
> 

In this spot, I am looking for linear () clause, and I want do differentiate 
between OMP and CIlk Plus. The only way I know of to do it here is to check if 
the mask is a Cilk Plus one or OMP one (thus checking mask == 
CILK_SIMD_FN_CLAUSE_MASK). So how does anding mask with vectorlength do the 
trick?


Thanks,

Balaji V. Iyer.

>   Jakub

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Mon, Dec 16, 2013 at 09:41:43PM +, Iyer, Balaji V wrote:
> --- gcc/c/c-parser.c  (revision 205759)
> +++ gcc/c/c-parser.c  (working copy)
> @@ -208,6 +208,12 @@
>/* True if we are in a context where the Objective-C "Property attribute"
>   keywords are valid.  */
>BOOL_BITFIELD objc_property_attr_context : 1;
> +
> +  /* Cilk Plus specific parser/lexer information.  */
> +
> +  /* Buffer to hold all the tokens from parsing the vector attribute for the
> + SIMD-enabled functions (formerly known as elemental functions).  */
> +  vec  *cilk_simd_fn_tokens;
>  } c_parser;

Joseph, is this ok for you?

> +/* Returns true of NAME is an IDENTIFIER_NODE with identiifer "vector,"
> +   "__vector" or "__vector__."  */
> +
> +static bool

static inline bool

> +is_cilkplus_vector_p (tree name)
> +{ 
> +  if (flag_enable_cilkplus  
> +  && (simple_cst_equal (name, get_identifier ("vector")) == 1
> +   || simple_cst_equal (name, get_identifier ("__vector")) == 1
> +   || simple_cst_equal (name, get_identifier ("__vector__")) == 1))
> +return true;
> +  return false;
> +}

Why not just
  return flag_enable_cilkplus && is_attribute_p ("vector", name);
?

> +#define CILK_SIMD_FN_CLAUSE_MASK \
> + ( (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_SIMDLEN)  \
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_LINEAR)   \
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)  \
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_INBRANCH) \
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_NOTINBRANCH))

I thought you'd instead add there PRAGMA_CILK_CLAUSE_VECTORLENGTH,
PRAGMA_CILK_CLAUSE_MASK and PRAGMA_CILK_CLAUSE_NOMASK (or similar).

> +  if (token->type == CPP_NAME
> +   && TREE_CODE (token->value) == IDENTIFIER_NODE)   
> + if (simple_cst_equal (token->value,
> +   get_identifier ("vectorlength")) == 1)

Why the simple_cst_equal + get_identifier?  I mean,
strcmp on IDENTIFIER_POINTER should be cheaper, and done elsewhere
in c-parser.c.

> +   {
> + if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))

Why do you parse it here?  Just parse it when parsing other clauses,
and only during parsing of vectorlength clause create OMP_CLAUSE_SAFELEN
out of it (after all the verifications).

> +  if (is_cilk_simd_fn && TREE_CODE (step) == PARM_DECL)
> + {
> +   error_at (clause_loc, "using parameters for % step is "
> + "not supported in this release");
> +   step = integer_one_node;

That would be sorry, not error_at.

>here = c_parser_peek_token (parser)->location;
> -  c_kind = c_parser_omp_clause_name (parser);
> + 
> +  if (mask == CILK_SIMD_FN_CLAUSE_MASK)

Ugh, no.

> + c_kind = c_parser_cilk_simd_fn_clause_name (parser);
> +  else
> + c_kind = c_parser_omp_clause_name (parser);

Always parse it the same, just use PRAGMA_CILK_CLAUSE_* for
the Cilk+ specific clauses.
>  
>switch (c_kind)
>   {
> @@ -10933,7 +11092,8 @@
> c_name = "aligned";
> break;
>   case PRAGMA_OMP_CLAUSE_LINEAR:
> -   clauses = c_parser_omp_clause_linear (parser, clauses);
> +   clauses = c_parser_omp_clause_linear
> + (parser, clauses, mask == CILK_SIMD_FN_CLAUSE_MASK);

Again, this is too ugly.  Perhaps check if
(mask & PRAGMA_CILK_CLAUSE_VECTORLENGTH) != 0 or similar.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Jakub,
Attached, please find a fixed patch. I have also answered your 
questions below. Is this Ok for trunk/branch?

Here are the ChangeLog entries:

Gcc/ChangeLog
2013-12-16  Balaji V. Iyer  

* omp-low.c (simd_clone_clauses_extract): Replaced the string
"cilk simd elemental" with "cilk simd function."

Gcc/c-family/ChangeLog
2013-12-16  Balaji V. Iyer  

* c-common.c (c_common_attribute_table): Added "cilk simd function" 
attribute.

Gcc/c/ChangeLog
2013-12-16  Balaji V. Iyer  

* c-parser.c (struct c_parser::cilk_simd_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if cilk_simd_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_cilk_clause_vectorlength): Modified function to be shared
between SIMD-enabled functions and #pragma simd.  Changed return-type
to bool and added new parameter.
(c_parser_cilk_all_clauses): Modified the usage of the function
c_parser_cilk_clause_vectorlength as mentioned above.
(c_parser_cilk_simd_fn_vector_attrs): New function.
(c_finish_cilk_simd_fn_tokens): Likewise.
(c_parser_cilk_simd_fn_clause_name): Likewise.
(is_cilkplus_vector_p): Likewise.
(c_parser_attributes): Added a cilk_simd_fn_tokens parameter.  Added a
check for vector attribute and if so call the function
c_parser_cilk_simd_fn_vector_attrs.  Also, when Cilk plus is 
enabled,
called the function c_finish_cilk_simd_fn_tokens.
(c_finish_omp_declare_simd): Added a check if cilk_simd_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.
(c_parser_omp_clause_linear): Added a new bool parm. is_cilk_simd_fn.
Added a check when step is a parameter and flag it as error.
(CILK_SIMD_FN_CLAUSE_MASK): New #define.

Gcc/testsuite/ChangeLog
2013-12-16  Balaji V. Iyer  

* c-c++-common/cilk-plus/SE/ef_test.c: New test.
* c-c++-common/cilk-plus/SE/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/SE/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error2.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error3.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.

Thanks,

Balaji V. Iyer.

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Jakub Jelinek
> Sent: Monday, December 16, 2013 11:52 AM
> To: Iyer, Balaji V
> Cc: Aldy Hernandez; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On Fri, Dec 13, 2013 at 06:37:22PM +, Iyer, Balaji V wrote:
> > @@ -3765,6 +3777,93 @@
> >return attr_name;
> >  }
> >
> > +/* Parses the vector attribute of SIMD enabled functions in Cilk Plus.
> > +   VEC_TOKEN is the "vector" token that is replaced with "simd" and
> > +   pushed into the token list.
> > +   Syntax:
> > +   vector
> > +   vector ().  */
> > +
> > +static void
> > +c_parser_cilk_simd_fn_vector_attrs (c_parser *parser, c_token
> > +vec_token) {
> > +  gcc_assert (simple_cst_equal (vec_token.value,
> > +get_identifier ("vector")) == 1);
> > +  int paren_scope = 0;
> > +  /* Replace the vector keyword with SIMD.  */
> > +  vec_token.value = get_identifier ("simd");
> > +  vec_safe_push (parser->cilk_simd_fn_tokens, vec_token);
> 
> So, why don't you just push the "vector" token as is?
> 

OK. Did that.

> > +  if (simple_cst_equal (value, get_identifier ("mask")) == 1)
> > +token->value = get_identifier ("inbranch");
> > +  else if (simple_cst_equal (value, get_identifier ("nomask")) == 
> > 1)
> > +token->value = get_identifier ("notinbranch");
> > +  else if (simple_cst_equal (value,
> > + get_identifier ("vectorlength")) == 1)
> > +{
> > +  if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))
> > +{
> > +  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 
> > NULL);
> > +  return;
> > +}
> > +  else
> > +continue;
> > +}
> 
> This I don't like at all, tweaking tokens is just too ugly.
>

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Jakub Jelinek]

On Fri, Dec 13, 2013 at 06:37:22PM +, Iyer, Balaji V wrote:
> @@ -3765,6 +3777,93 @@
>return attr_name;
>  }
>  
> +/* Parses the vector attribute of SIMD enabled functions in Cilk Plus.
> +   VEC_TOKEN is the "vector" token that is replaced with "simd" and
> +   pushed into the token list. 
> +   Syntax:
> +   vector
> +   vector ().  */
> +
> +static void
> +c_parser_cilk_simd_fn_vector_attrs (c_parser *parser, c_token vec_token)
> +{
> +  gcc_assert (simple_cst_equal (vec_token.value,
> +get_identifier ("vector")) == 1);
> +  int paren_scope = 0;
> +  /* Replace the vector keyword with SIMD.  */
> +  vec_token.value = get_identifier ("simd");
> +  vec_safe_push (parser->cilk_simd_fn_tokens, vec_token);

So, why don't you just push the "vector" token as is?

> +  if (simple_cst_equal (value, get_identifier ("mask")) == 1)
> +token->value = get_identifier ("inbranch");
> +  else if (simple_cst_equal (value, get_identifier ("nomask")) == 1)
> +token->value = get_identifier ("notinbranch");
> +  else if (simple_cst_equal (value,
> + get_identifier ("vectorlength")) == 1)
> +{
> +  if (!c_parser_cilk_clause_vectorlength (parser, NULL, true))
> +{
> +  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
> +  return;
> +}
> +  else
> +continue;
> +}

This I don't like at all, tweaking tokens is just too ugly.
I'd prefer if you just keep the tokens as is, and simply:

>while (parser->tokens_avail > 3)
>  {
> @@ -12792,11 +12922,22 @@

Allow "vector" (and I think you need to also allow __vector and __vector__,
don't you?) here too, for flag_enable_cilkplus.

BTW, can you as a followup rename flag_enable_cilkplus to flag_cilkplus?


>c_parser_consume_token (parser);
>parser->in_pragma = true;
>  
> -  tree c = c_parser_omp_all_clauses (parser, 
> OMP_DECLARE_SIMD_CLAUSE_MASK,
> -  "#pragma omp declare simd");
> +  tree c = NULL_TREE;
> +  if (is_cilkplus_cilk_simd_fn) 
> + c = c_parser_omp_all_clauses (parser, OMP_DECLARE_SIMD_CLAUSE_MASK,
> +   "SIMD-enabled functions attribute");

You'd use different mask here that would not include the clauses Cilk+
doesn't have (inbranch/notinbranch/safelen) and include the one Cilk+ has,
and make sure you handle parsing those.  And just parsing of say mask clause
tokens would create OMP_CLAUSE_INBRANCH clause.

BTW, I don't see how you handle the Cilk+ specific enhancement to linear
clause, that you can refer to some parameter instead.  The question is how
would c_parser_omp_clause_linear know whether it is now parsing OpenMP
clauses or Cilk+ vector attribute tokens.  Plus there are no testcases for
that (though, it will likely need some further omp-low.c and vectorizer
changes), but at least Aldy has reserved a bit and created macro for it
on the clause.

Jakub

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
> Sent: Friday, December 13, 2013 12:40 PM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> > +  /* Two CPP_EOF token is added as a safety-net since the normal C
> > + front-end has two token look-ahead.  */
> 
> "Two CPP_EOF tokens are added"...  Also, "safety net" are two words, not
> one hyphenated one.
> 
> Otherwise, I'm fine with the present patch.  It's up to Jakub or another 
> global
> reviewer to give the final approval.

Here is the fixed patch with the above changes. 


So, is it OK for trunk/branch?

Thanks,

Balaji V. Iyer.
> 
> Aldy
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 205759)
+++ gcc/c-family/c-common.c (working copy)
@@ -771,6 +771,8 @@
  handle_returns_nonnull_attribute, false },
   { "omp declare simd",   0, -1, true,  false, false,
  handle_omp_declare_simd_attribute, false },
+  { "cilk simd function", 0, -1, true,  false, false,
+ handle_omp_declare_simd_attribute, false },
   { "omp declare target", 0, 0, true, false, false,
  handle_omp_declare_target_attribute, false },
   { "bnd_variable_size",  0, 0, true,  false, false,
Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c(revision 205759)
+++ gcc/c/c-parser.c(working copy)
@@ -208,6 +208,12 @@
   /* True if we are in a context where the Objective-C "Property attribute"
  keywords are valid.  */
   BOOL_BITFIELD objc_property_attr_context : 1;
+
+  /* Cilk Plus specific parser/lexer information.  */
+
+  /* Buffer to hold all the tokens from parsing the vector attribute for the
+ SIMD-enabled functions (formerly known as elemental functions).  */
+  vec  *cilk_simd_fn_tokens;
 } c_parser;
 
 
@@ -1246,6 +1252,7 @@
 static void c_parser_cilk_simd (c_parser *);
 static bool c_parser_cilk_verify_simd (c_parser *, enum pragma_context);
 static tree c_parser_array_notation (location_t, c_parser *, tree, tree);
+static bool c_parser_cilk_clause_vectorlength (c_parser *, tree *, bool);
 
 /* Parse a translation unit (C90 6.7, C99 6.9).
 
@@ -1647,7 +1654,8 @@
C_DTR_NORMAL, &dummy);
   if (declarator == NULL)
{
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
c_finish_omp_declare_simd (parser, NULL_TREE, NULL_TREE,
   omp_declare_simd_clauses);
  c_parser_skip_to_end_of_block_or_statement (parser);
@@ -1734,7 +1742,8 @@
  chainon (postfix_attrs, all_prefix_attrs));
  if (!d)
d = error_mark_node;
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
c_finish_omp_declare_simd (parser, d, NULL_TREE,
   omp_declare_simd_clauses);
}
@@ -1746,7 +1755,8 @@
  chainon (postfix_attrs, all_prefix_attrs));
  if (!d)
d = error_mark_node;
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
c_finish_omp_declare_simd (parser, d, NULL_TREE,
   omp_declare_simd_clauses);
  start_init (d, asm_name, global_bindings_p ());
@@ -1774,7 +1784,8 @@
  tree d = start_decl (declarator, specs, false,
   chainon (postfix_attrs,
all_prefix_attrs));
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
{
  tree parms = NULL_TREE;
  if (d && TREE_CODE (d) == FUNCTION_DECL)
@@ -1902,7 +1913,8 @@
c_parser_declaration_or_fndef (parser, false, false, false,
   true, false, NULL, vNULL);
   store_parm_decls ();
-  if (omp_de

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

+  /* Two CPP_EOF token is added as a safety-net since the normal C
+ front-end has two token look-ahead.  */


"Two CPP_EOF tokens are added"...  Also, "safety net" are two words, not 
one hyphenated one.


Otherwise, I'm fine with the present patch.  It's up to Jakub or another 
global reviewer to give the final approval.


Aldy

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Thursday, December 12, 2013 3:29 PM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On 12/12/13 07:56, Iyer, Balaji V wrote:
> 
> > Will it be Ok if I don’t mark them as cilk simd function but just keep
> > it as omp declare simd from the start? That should get around this
> > issue.
> 
> No, because then we won't be able to distinguish between OMP and Cilk
> Plus clones.  This is something we do here:
> 
>/* To distinguish from an OpenMP simd clone, Cilk Plus functions to
>   be cloned have a distinctive artificial label in addition to "omp
>   declare simd".  */
>bool cilk_clone
>  = (flag_enable_cilkplus
> && lookup_attribute ("cilk plus elemental",
>   DECL_ATTRIBUTES (node->decl)));
> 
>/* Allocate one more than needed just in case this is an in-branch
>   clone which will require a mask argument.  */
>struct cgraph_simd_clone *clone_info = simd_clone_struct_alloc (n + 1);
>clone_info->nargs = n;
>clone_info->cilk_elemental = cilk_clone;
> 


Ok. I found the bug and fixed it. It was looking for cilk plus elemental and 
now that we renamed it to cilk simd function it was cloning a whole bunch of 
them. It is fixed and now giving the correct behavior.

It passes all the tests in my x86_64 machine. Is it OK to install in 
GOMP4/Trunk?

Here are the Changelog entries:

Gcc/ChangeLog
2013-12-13  Balaji V. Iyer  

* omp-low.c (simd_clone_clauses_extract): Replaced the string
"cilk simd elemental" with "cilk simd function."

Gcc/c/ChangeLog
2013-12-13  Balaji V. Iyer  

* c-parser.c (struct c_parser::cilk_simd_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if cilk_simd_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_cilk_clause_vectorlength): Modified function to be shared
between SIMD-enabled functions and #pragma simd.  Changed return-type
to bool and added new parameter.
(c_parser_cilk_all_clauses): Modified the usage of the function
c_parser_cilk_clause_vectorlength as mentioned above.
(c_parser_cilk_simd_fn_vector_attrs): New function.
(c_finish_cilk_simd_fn_tokens): Likewise.
(c_parser_attributes): Added a cilk_simd_fn_tokens parameter.  Added a
check for vector attribute and if so call the function
c_parser_cilk_simd_fn_vector_attrs.  Also, when Cilk plus is enabled,
called the function c_finish_cilk_simd_fn_tokens.
(c_finish_omp_declare_simd): Added a check if cilk_simd_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.

Gcc/testsuite/ChangeLog
2013-12-13  Balaji V. Iyer  

* c-c++-common/cilk-plus/SE/ef_test.c: New test.
* c-c++-common/cilk-plus/SE/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/SE/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error2.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.


The C++ patch with this fix is coming up ...

Thanks,

Balaji V. Iyer.
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 205759)
+++ gcc/c-family/c-common.c (working copy)
@@ -771,6 +771,8 @@
  handle_returns_nonnull_attribute, false },
   { "omp declare simd",   0, -1, true,  false, false,
  handle_omp_declare_simd_attribute, false },
+  { "cilk simd function", 0, -1, true,  false, false,
+ handle_omp_declare_simd_attribute, false },
   { "omp declare target", 0, 0, true, false, false,
  handle_omp_declare_target_attribute, false },
   { "bnd_variable_size",  0, 0, true,  false, false,
Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c(revision 205759)
+++ gcc/c/c-parser.c(working copy)
@@ -208,6 +208,12 @@
   /* True if we are in a context where the Objective-C "Property attribute"
  keywords are valid.  */
   BOOL_BITFIELD objc_property_attr_context : 1;
+
+  /* Cilk Plus specific parser/lexer information.  */
+
+  /* Buffer to hold all the tokens from parsing the vector attribute for the
+ SIMD-enabled functions (formerly known as ele

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

On 12/12/13 07:56, Iyer, Balaji V wrote:


Will it be Ok if I don’t mark them as cilk simd function but just
keep it as omp declare simd from the start? That should get around
this issue.


No, because then we won't be able to distinguish between OMP and Cilk 
Plus clones.  This is something we do here:


  /* To distinguish from an OpenMP simd clone, Cilk Plus functions to
 be cloned have a distinctive artificial label in addition to "omp
 declare simd".  */
  bool cilk_clone
= (flag_enable_cilkplus
   && lookup_attribute ("cilk plus elemental",
DECL_ATTRIBUTES (node->decl)));

  /* Allocate one more than needed just in case this is an in-branch
 clone which will require a mask argument.  */
  struct cgraph_simd_clone *clone_info = simd_clone_struct_alloc (n + 1);
  clone_info->nargs = n;
  clone_info->cilk_elemental = cilk_clone;

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Thursday, December 12, 2013 10:26 AM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On 12/11/13 10:44, Iyer, Balaji V wrote:
> 
> > Just out of curiosity, why can't I keep it as-is? It is giving the
> > correct output/behavior and doesn't seem to interfere with anything
> > else. The only extra thing I am doing is to add an extra if-statement
> > while recursing through all the functions to check for cilk simd
> > function and then calling the function to handle it, which the OMP
> > will have to do anyway. The only extra thing I added was an extra
> > if-statement.
> 
> If Cilk clones are tagged differently then we need to special case Cilk clones
> every where we handle OMP clones.  If they share an attribute, no special
> handling is needed.

Will it be Ok if I don’t mark them as cilk simd function but just keep it as 
omp declare simd from the start? That should get around this issue.

Thanks,

Balaji V. Iyer.

> 
> Aldy

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

On 12/11/13 10:44, Iyer, Balaji V wrote:


Just out of curiosity, why can't I keep it as-is? It is giving the
correct output/behavior and doesn't seem to interfere with anything
else. The only extra thing I am doing is to add an extra if-statement
while recursing through all the functions to check for cilk simd
function and then calling the function to handle it, which the OMP
will have to do anyway. The only extra thing I added was an extra
if-statement.


If Cilk clones are tagged differently then we need to special case Cilk 
clones every where we handle OMP clones.  If they share an attribute, no 
special handling is needed.


Aldy

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Aldy,

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Wednesday, December 11, 2013 1:27 PM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; 'gcc-patches@gcc.gnu.org'
> Subject: RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> Can you investigate why it is creating multiple clones?
> 

I can and will let you know as soon as I find it. I am working on GOMP4 branch, 
could that cause anything? 

Just out of curiosity, why can't I keep it as-is? It is giving the correct 
output/behavior and doesn't seem to interfere with anything else. The only 
extra thing I am doing is to add an extra if-statement while recursing through 
all the functions to check for cilk simd function and then calling the function 
to handle it, which the OMP will have to do anyway. The only extra thing I 
added was an extra if-statement.

Please don't read my above paragraph as an argument. I am just asking for 
clarification and to understand more about the underlying routines.

Thanks,

Balaji V. Iyer.

> On Dec 11, 2013 10:06 AM, "Iyer, Balaji V"  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Aldy Hernandez [mailto:ald
> 
> > -Original Message-
> > From: Aldy Hernandez [mailto:al...@redhat.com]
> > Sent: Wednesday, December 11, 2013 12:38 PM
> > To: Iyer, Balaji V
> > Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> > Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> > Elemental functions) for C
> >
> > On 12/11/13 09:31, Iyer, Balaji V wrote:
> > >
> > >
> > >> -Original Message-
> > >> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> > >> ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
> > >> Sent: Tuesday, December 10, 2013 1:03 PM
> > >> To: Iyer, Balaji V
> > >> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> > >> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions
> > >> (formerly Elemental functions) for C
> > >>
> > >>
> > >>>> But aren't both OpenMP and Cilk Plus simd clones marked as "omp
> > >>>> declare simd"?  In which case you shouldn't have to do anything?
> > >>>> Are the Cilk Plus clones not being marked as "omp declare simd"
> > >>>> in the front-ends?
> > >>>>
> > >>>
> > >>> Didn't you mention in this thread
> > >>> (http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that
> > >>> Cilk Plus SIMD-enabled functions must be marked as "cilk plus
> elementals"
> > >>> (as it wsa called then)? Did I misunderstand you?
> > >>
> > >> Both OpenMP and Cilk Plus clones should be marked with "omp declare
> > >> simd".  But Cilk Plus should _also_ be marked with "cilk plus
> > >> elementals" (or "cilk simd function" now).  In which case the
> > >> aforementioned function doesn't require any changes because Cilk
> > >> Plus clones will be seen as they are marked with "omp declare simd".
> > >>
> > >> So...
> > >>
> > >>  OpenMP declare simd gets tagged as:
> > >>  "omp declare simd"
> > >>  Cilk Plus vector functions gets tagged as:
> > >>  "omp declare simd"
> > >>  "cilk simd function"
> > >>
> > >
> > > Ok, so you want the same clauses included in both omp declare simd
> > > and
> > cilk simd function tree lists?
> > >
> > > For example in the c-parser.c, I have something like this:
> > >
> > >if (is_cilkplus_cilk_simd_fn)
> > >  c = build_tree_list (get_identifier ("cilk simd function"), c);
> > >else
> > >  c = build_tree_list (get_identifier ("omp declare simd"), c);
> > >TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
> > >DECL_ATTRIBUTES (fndecl) = c;
> > >
> > >
> > > You want to change it something like this?
> > >
> > >
> > > If (is_cilkplus_cilk_simd_fn)
> > >{
> > >   tree c_cilk = build_tree_list (get_identifier ("cilk simd function"), 
> > > c);
> > >   TREE_CHAIN (c_cilk) = DECL_ATTRIBUTES (fndecl);
> > >   DECL_ATTRIBUTES (fndecl) = c_cilk;
> > > }
> > > c = build_tree_list (get_identififer ("omp declare simd"), c);
> > > TREE_CHAIN (c) =DECL_ATTRIBUTES (fndecl); DECL_ATTRIBUTES (fndecl)
> =
> > > c;
> > >
> > >
> >
> > Yes.
> 
> The issue with doing this is that it is creating duplicate clones. If I just 
> kept the
> patch as is, it does not.
> 
> For example:
> 
> __attribute__((vector (vectorlength(sizeof(int) int func3 (int x) {
>   return x;
> }
> 
> Should create two clones: mask and unmask (_ZGVbN4v_func3 and
> _ZGVbM4v_func3). It is doing that if I kept the patch AS-IS.
> 
> Now, if I make the modification that I mentioned above and remove the
> handling of cilk simd function in omp-low.c it is creating  6 clones:
> _ZGVdM4v_func3, _ZGVbN4v_func3, _ZGVcN4v_func3, _ZGVcM4v_func3,
> _ZGVdN4v_func3
> 
> Thanks,
> 
> Balaji V. Iyer.

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

Can you investigate why it is creating multiple clones?

On Dec 11, 2013 10:06 AM, "Iyer, Balaji V"  wrote:
>
>
>
> > -Original Message- 
> > From: Aldy Hernandez [mailto:ald

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Wednesday, December 11, 2013 12:38 PM
> To: Iyer, Balaji V
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On 12/11/13 09:31, Iyer, Balaji V wrote:
> >
> >
> >> -Original Message-
> >> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> >> ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
> >> Sent: Tuesday, December 10, 2013 1:03 PM
> >> To: Iyer, Balaji V
> >> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> >> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> >> Elemental functions) for C
> >>
> >>
> >>>> But aren't both OpenMP and Cilk Plus simd clones marked as "omp
> >>>> declare simd"?  In which case you shouldn't have to do anything?
> >>>> Are the Cilk Plus clones not being marked as "omp declare simd" in
> >>>> the front-ends?
> >>>>
> >>>
> >>> Didn't you mention in this thread
> >>> (http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that Cilk
> >>> Plus SIMD-enabled functions must be marked as "cilk plus elementals"
> >>> (as it wsa called then)? Did I misunderstand you?
> >>
> >> Both OpenMP and Cilk Plus clones should be marked with "omp declare
> >> simd".  But Cilk Plus should _also_ be marked with "cilk plus
> >> elementals" (or "cilk simd function" now).  In which case the
> >> aforementioned function doesn't require any changes because Cilk Plus
> >> clones will be seen as they are marked with "omp declare simd".
> >>
> >> So...
> >>
> >>OpenMP declare simd gets tagged as:
> >>"omp declare simd"
> >>Cilk Plus vector functions gets tagged as:
> >>"omp declare simd"
> >>"cilk simd function"
> >>
> >
> > Ok, so you want the same clauses included in both omp declare simd and
> cilk simd function tree lists?
> >
> > For example in the c-parser.c, I have something like this:
> >
> >if (is_cilkplus_cilk_simd_fn)
> >  c = build_tree_list (get_identifier ("cilk simd function"), c);
> >else
> >  c = build_tree_list (get_identifier ("omp declare simd"), c);
> >TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
> >DECL_ATTRIBUTES (fndecl) = c;
> >
> >
> > You want to change it something like this?
> >
> >
> > If (is_cilkplus_cilk_simd_fn)
> >{
> > tree c_cilk = build_tree_list (get_identifier ("cilk simd function"), 
> > c);
> > TREE_CHAIN (c_cilk) = DECL_ATTRIBUTES (fndecl);
> > DECL_ATTRIBUTES (fndecl) = c_cilk;
> > }
> > c = build_tree_list (get_identififer ("omp declare simd"), c);
> > TREE_CHAIN (c) =DECL_ATTRIBUTES (fndecl); DECL_ATTRIBUTES (fndecl) =
> > c;
> >
> >
> 
> Yes.

The issue with doing this is that it is creating duplicate clones. If I just 
kept the patch as is, it does not.

For example: 

__attribute__((vector (vectorlength(sizeof(int)
int func3 (int x)
{
  return x;
}

Should create two clones: mask and unmask (_ZGVbN4v_func3 and _ZGVbM4v_func3). 
It is doing that if I kept the patch AS-IS.

Now, if I make the modification that I mentioned above and remove the handling 
of cilk simd function in omp-low.c it is creating  6 clones: _ZGVdM4v_func3, 
_ZGVbN4v_func3, _ZGVcN4v_func3, _ZGVcM4v_func3, _ZGVdN4v_func3

Thanks,

Balaji V. Iyer.

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Wednesday, December 11, 2013 12:38 PM
> To: Iyer, Balaji V
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On 12/11/13 09:31, Iyer, Balaji V wrote:
> >
> >
> >> -Original Message-
> >> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> >> ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
> >> Sent: Tuesday, December 10, 2013 1:03 PM
> >> To: Iyer, Balaji V
> >> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> >> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> >> Elemental functions) for C
> >>
> >>
> >>>> But aren't both OpenMP and Cilk Plus simd clones marked as "omp
> >>>> declare simd"?  In which case you shouldn't have to do anything?
> >>>> Are the Cilk Plus clones not being marked as "omp declare simd" in
> >>>> the front-ends?
> >>>>
> >>>
> >>> Didn't you mention in this thread
> >>> (http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that Cilk
> >>> Plus SIMD-enabled functions must be marked as "cilk plus elementals"
> >>> (as it wsa called then)? Did I misunderstand you?
> >>
> >> Both OpenMP and Cilk Plus clones should be marked with "omp declare
> >> simd".  But Cilk Plus should _also_ be marked with "cilk plus
> >> elementals" (or "cilk simd function" now).  In which case the
> >> aforementioned function doesn't require any changes because Cilk Plus
> >> clones will be seen as they are marked with "omp declare simd".
> >>
> >> So...
> >>
> >>OpenMP declare simd gets tagged as:
> >>"omp declare simd"
> >>Cilk Plus vector functions gets tagged as:
> >>"omp declare simd"
> >>"cilk simd function"
> >>
> >
> > Ok, so you want the same clauses included in both omp declare simd and
> cilk simd function tree lists?
> >
> > For example in the c-parser.c, I have something like this:
> >
> >if (is_cilkplus_cilk_simd_fn)
> >  c = build_tree_list (get_identifier ("cilk simd function"), c);
> >else
> >  c = build_tree_list (get_identifier ("omp declare simd"), c);
> >TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
> >DECL_ATTRIBUTES (fndecl) = c;
> >
> >
> > You want to change it something like this?
> >
> >
> > If (is_cilkplus_cilk_simd_fn)
> >{
> > tree c_cilk = build_tree_list (get_identifier ("cilk simd function"), 
> > c);
> > TREE_CHAIN (c_cilk) = DECL_ATTRIBUTES (fndecl);
> > DECL_ATTRIBUTES (fndecl) = c_cilk;
> > }
> > c = build_tree_list (get_identififer ("omp declare simd"), c);
> > TREE_CHAIN (c) =DECL_ATTRIBUTES (fndecl); DECL_ATTRIBUTES (fndecl) =
> > c;
> >
> >
> 
> Yes.

The issue with doing this is that it is creating duplicate clones. If I just 
kept the patch as is, it does not.

For example: 

__attribute__((vector (vectorlength(sizeof(int)
int func3 (int x)
{
  return x;
}

Should create two clones: mask and unmask (_ZGVbN4v_func3 and _ZGVbM4v_func3). 
It is doing that if I kept the patch AS-IS.

Now, if I make the modification that I mentioned above and remove the handling 
of cilk simd function in omp-low.c it is creating  6 clones: _ZGVdM4v_func3, 
_ZGVbN4v_func3, _ZGVcN4v_func3, _ZGVcM4v_func3, _ZGVdN4v_func3

Thanks,

Balaji V. Iyer.

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

On 12/11/13 09:31, Iyer, Balaji V wrote:




-Original Message-
From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
Sent: Tuesday, December 10, 2013 1:03 PM
To: Iyer, Balaji V
Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
Elemental functions) for C



But aren't both OpenMP and Cilk Plus simd clones marked as "omp
declare simd"?  In which case you shouldn't have to do anything?
Are the Cilk Plus clones not being marked as "omp declare simd" in
the front-ends?



Didn't you mention in this thread
(http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that Cilk
Plus SIMD-enabled functions must be marked as "cilk plus elementals"
(as it wsa called then)? Did I misunderstand you?


Both OpenMP and Cilk Plus clones should be marked with "omp declare
simd".  But Cilk Plus should _also_ be marked with "cilk plus elementals" (or
"cilk simd function" now).  In which case the aforementioned function
doesn't require any changes because Cilk Plus clones will be seen as they are
marked with "omp declare simd".

So...

OpenMP declare simd gets tagged as:
"omp declare simd"
Cilk Plus vector functions gets tagged as:
"omp declare simd"
"cilk simd function"



Ok, so you want the same clauses included in both omp declare simd and cilk 
simd function tree lists?

For example in the c-parser.c, I have something like this:

   if (is_cilkplus_cilk_simd_fn)
 c = build_tree_list (get_identifier ("cilk simd function"), c);
   else
 c = build_tree_list (get_identifier ("omp declare simd"), c);
   TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
   DECL_ATTRIBUTES (fndecl) = c;


You want to change it something like this?


If (is_cilkplus_cilk_simd_fn)
   {
tree c_cilk = build_tree_list (get_identifier ("cilk simd function"), 
c);
TREE_CHAIN (c_cilk) = DECL_ATTRIBUTES (fndecl);
DECL_ATTRIBUTES (fndecl) = c_cilk;
}
c = build_tree_list (get_identififer ("omp declare simd"), c);
TREE_CHAIN (c) =DECL_ATTRIBUTES (fndecl);
DECL_ATTRIBUTES (fndecl) = c;




Yes.

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Aldy Hernandez
> Sent: Tuesday, December 10, 2013 1:03 PM
> To: Iyer, Balaji V
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> 
> >> But aren't both OpenMP and Cilk Plus simd clones marked as "omp
> >> declare simd"?  In which case you shouldn't have to do anything?
> >> Are the Cilk Plus clones not being marked as "omp declare simd" in
> >> the front-ends?
> >>
> >
> > Didn't you mention in this thread
> > (http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that Cilk
> > Plus SIMD-enabled functions must be marked as "cilk plus elementals"
> > (as it wsa called then)? Did I misunderstand you?
> 
> Both OpenMP and Cilk Plus clones should be marked with "omp declare
> simd".  But Cilk Plus should _also_ be marked with "cilk plus elementals" (or
> "cilk simd function" now).  In which case the aforementioned function
> doesn't require any changes because Cilk Plus clones will be seen as they are
> marked with "omp declare simd".
> 
> So...
> 
>   OpenMP declare simd gets tagged as:
>   "omp declare simd"
>   Cilk Plus vector functions gets tagged as:
>   "omp declare simd"
>   "cilk simd function"
> 

Ok, so you want the same clauses included in both omp declare simd and cilk 
simd function tree lists?
 
For example in the c-parser.c, I have something like this:

  if (is_cilkplus_cilk_simd_fn)
c = build_tree_list (get_identifier ("cilk simd function"), c);
  else
c = build_tree_list (get_identifier ("omp declare simd"), c);
  TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
  DECL_ATTRIBUTES (fndecl) = c;


You want to change it something like this?


If (is_cilkplus_cilk_simd_fn)
  {
tree c_cilk = build_tree_list (get_identifier ("cilk simd function"), 
c);
TREE_CHAIN (c_cilk) = DECL_ATTRIBUTES (fndecl);
DECL_ATTRIBUTES (fndecl) = c_cilk;
}
c = build_tree_list (get_identififer ("omp declare simd"), c);
TREE_CHAIN (c) =DECL_ATTRIBUTES (fndecl);
DECL_ATTRIBUTES (fndecl) = c;

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

But aren't both OpenMP and Cilk Plus simd clones marked as "omp
declare simd"?  In which case you shouldn't have to do anything?
Are the Cilk Plus clones not being marked as "omp declare simd" in
the front-ends?



Didn't you mention in this thread
(http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03506.html) that Cilk
Plus SIMD-enabled functions must be marked as "cilk plus elementals"
(as it wsa called then)? Did I misunderstand you?


Both OpenMP and Cilk Plus clones should be marked with "omp declare
simd".  But Cilk Plus should _also_ be marked with "cilk plus
elementals" (or "cilk simd function" now).  In which case the
aforementioned function doesn't require any changes because Cilk Plus
clones will be seen as they are marked with "omp declare simd".

So...

OpenMP declare simd gets tagged as:
"omp declare simd"
Cilk Plus vector functions gets tagged as:
"omp declare simd"
"cilk simd function"


if (is_simd_fn) { c_token new_token; /* If we are here then it
has be a number.  */ new_token.type = CPP_NUMBER;
new_token.keyword = RID_MAX;


I thought we agreed integral constants were allowed.  Why would we
be expecting only a number?  Also, it should have been "it has TO
be a number".



Whatever that is reading this token only checks if this token is not
a keyword or a variable.  So, I put it as a number since it is the
most common case. I clarified it in the comment.


Ah, I see, perfect.  Thanks for the comment explaining so.

Aldy

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Aldy,
Answers to your questions are given below. Here are the fixed ChangeLog 
entries. Is it OK now?

Thanks,

Balaji V. Iyer.


Gcc/ChangeLog
2013-12-09  Balaji V. Iyer  

* omp-low.c (expand_simd_clones): Added a new parameter called "type."
(ipa_omp_simd_clone): Added a call to expand_simd_clones when Cilk Plus
is enabled.
(simd_clone_clauses_extract): Replaced the string "cilk simd elemental"
with "cilk simd function."

Gcc/c-family/ChangeLog
2013-12-09  Balaji V. Iyer  

* c-common.c (c_common_attribute_table): Added "cilk simd function" 
attribute.

Gcc/c/ChangeLog
2013-12-09  Balaji V. Iyer  

* c-parser.c (struct c_parser::cilk_simd_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if cilk_simd_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_cilk_clause_vectorlength): Modified function to be shared
between SIMD-enabled functions and #pragma simd.  Changed return-type
to bool and added new parameter.
(c_parser_cilk_all_clauses): Modified the usage of the function
c_parser_cilk_clause_vectorlength as mentioned above.
(c_parser_cilk_simd_fn_vector_attrs): New function.
(c_finish_cilk_simd_fn_tokens): Likewise.
(c_parser_attributes): Added a cilk_simd_fn_tokens parameter.  Added a
check for vector attribute and if so call the function
c_parser_cilk_simd_fn_vector_attrs.  Also, when Cilk plus is 
enabled,
called the function c_finish_cilk_simd_fn_tokens.
(c_finish_omp_declare_simd): Added a check if cilk_simd_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.

Gcc/testsuite/ChangeLog
2013-12-09  Balaji V. Iyer  

* c-c++-common/cilk-plus/SE/ef_test.c: New test.
* c-c++-common/cilk-plus/SE/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/SE/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error2.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.



> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Friday, December 6, 2013 6:16 PM
> To: Iyer, Balaji V
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> [Jakub, see below]
> 
> >>> +  if (!c_parser_elem_fn_vectorlength (parser)) +
> >>> { +  c_parser_skip_until_found (parser,
> >>> CPP_CLOSE_PAREN, NULL); +  /* NO reason to keep
> >>> any of these tokens if the + vectorlength is
> >>> messed up.  */ +vec_free (parser->elem_fn_tokens); +
> >>> return;
> >>
> >> It may be cleaner to make the caller free the vector.
> >
> > Well, the caller doesn't know if an error has occurred. I suppose I
> > could do something like check for seen_error (), but this sounds a bit
> > more clearer (to me altleast)
> 
> Sorry, what I meant to say is that it may be cleaner if
> c_parser_elem_fn_vectorlength (or whatever it's called now) frees the
> vector upon error before returning.
> 

Answer to this question is given below

> >> First of all, it's a really bad idea to scan all the functions twice.
> >> You should have adapted expand_simd_clones() to do the work for both.
> >>
> >
> > OK.  I included this in the first loop itself so we won't have to
> > scan the functions twice.
> >
> >> But even so, I don't think you need to do this at all.  Aren't Cilk
> >> Plus elementals supposed to be tagged as "omp declare simd" as
> >> well?  In which case expand_simd_clones() will DTRT.  It
> >> should...and then simd_clone_clauses_extract() already has the
> >> smarts to differentiate between the both variants.
> >>
> >
> > Yes, the thing is, there is a big do-while loop in the function and
> > that needs to be replaced if we have to check for SIMD-enabled
> > function and #pragma omp declare simd. If we pass it as a type, then
> > it just needs to check for the type string.
> 
> But aren't both OpenMP and Cilk Plus simd clones marked as "omp declare
> simd"?  In which case you shouldn't have to do anything?  Are the Cilk
> Plus clones not being marked as "omp declare simd" in the front-ends?
> 

Didn't you mention in t

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

[Jakub, see below]


+  if (!c_parser_elem_fn_vectorlength (parser)) +
{ +  c_parser_skip_until_found (parser,
CPP_CLOSE_PAREN, NULL); +  /* NO reason to keep
any of these tokens if the + vectorlength is
messed up.  */ +  vec_free (parser->elem_fn_tokens); +
return;


It may be cleaner to make the caller free the vector.


Well, the caller doesn't know if an error has occurred. I suppose I
could do something like check for seen_error (), but this sounds a
bit more clearer (to me altleast)


Sorry, what I meant to say is that it may be cleaner if 
c_parser_elem_fn_vectorlength (or whatever it's called now) frees the 
vector upon error before returning.



First of all, it's a really bad idea to scan all the functions
twice. You should have adapted expand_simd_clones() to do the work
for both.



OK.  I included this in the first loop itself so we won't have to
scan the functions twice.


But even so, I don't think you need to do this at all.  Aren't Cilk
Plus elementals supposed to be tagged as "omp declare simd" as
well?  In which case expand_simd_clones() will DTRT.  It
should...and then simd_clone_clauses_extract() already has the
smarts to differentiate between the both variants.



Yes, the thing is, there is a big do-while loop in the function and
that needs to be replaced if we have to check for SIMD-enabled
function and #pragma omp declare simd. If we pass it as a type, then
it just needs to check for the type string.


But aren't both OpenMP and Cilk Plus simd clones marked as "omp declare 
simd"?  In which case you shouldn't have to do anything?  Are the Cilk 
Plus clones not being marked as "omp declare simd" in the front-ends?



I narrowed this to 5 with the help of Jakub a while back. But now, I
have replaced it with 3, "-O3 -g"," -O3 -g -std=c99"


I would prefer to get rid of -O3, since inlining may do interesting 
things to tests, and you'll have to use __attribute__((noinline)) to 
test some things.


Jakub, would you be ok with "-O0 -g" and "-O0 -std=c99"?  For that 
matter, I'd say pass no arguments at all (""), and let the test itself 
test something special if required.



For that matter, we should probably get rid of all the variant
testing in the rest of Cilk Plus.


I will send this out as a different patch later for all the others.


Thank you.  Do so after Jakub responds as to what he prefers.


Renamed "EF" to "SE" (Simd Enabled function)


If you must, but I would still prefer something more meaningful (i.e., 
not an abbreviation).  I know there is precedence with the 
array-notation feature, but I dislike that too :).  Feel free to ignore 
me on this one.



+/* Parses the vector attribute of SIMD enabled functions in Cilk Plus.



+   The VEC_TOKEN is the "vector" token that is replaced with "simd" and
+   pushed into the token list.



+   Syntax:
+   vector
+   vector ().  */


Also, s/The VEC_TOKEN/VEC_TOKEN/


+static void
+c_parser_cilk_simd_fn_expr_list (c_parser *parser, c_token vec_token)


This is a parsing routine for the vector attribute, let's call this 
"c_parser_cilk_simd_fn_vector" or "c_parser_cilk_simd_fn_vector_attrs". 
 The expr_list is confusing.



+  /* NO reason to keep any of these tokens if the
+ vectorlength is messed up.  */


Lower case "NO".


+ vec_free (parser->cilk_simd_fn_tokens);
+  // parser->cilk_simd_fn_tokens->release ();
+  return;


What's this commented out code?  If unnecessary, remove it.


+  return;
+}


Empty return at end of function.  Remove it.


+  /* c_parser_attributes is called in several places, and so if these EOF


s/and so/so/


+  /* Two EOF_token is added as a safety-net since the normal C front-end has
+ two token look-ahead.  */


Shouldn't that be, "Two CPP_EOF tokens" ??


+  error ("%<#pragma omp declare simd%> cannot be used in the same "
+"function marked as a SIMD-enabled function");


Perhaps we should say "...as a Cilk Plus SIMD-enabled...", to make it 
absolutely clear that it is OpenMP and Cilk Plus that can't coexist.



 /* Cilk Plus:
-   vectorlength ( constant-expression ) */
+   This function is shared by SIMD-enabled functions and #pragma simd.
+   If IS_SIMD_FN is true then it is parsing a SIMD-enabled function and
+   CLAUSES is unused.
+   Syntax:
+   vectorlength ( constant-expression )  */

-static tree
-c_parser_cilk_clause_vectorlength (c_parser *parser, tree clauses)
+static bool
+c_parser_cilk_clause_vectorlength (c_parser *parser, tree *clauses,
+  bool is_simd_fn)


Can you document what this function does?  Also, document the fact that 
when IS_SIMD_FN is true, we are merely caching the tokens, otherwise we 
are building the OMP_CLAUSE_SAFELEN.



  /* if expr is error_mark_node, then the returning function would have
 flagged the error.  No need to flag them twice.  */

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Aldy  & Jakub,
I have made the fixes you have mentioned and have answered your 
questions below. Attached is the fixed patch and here are the ChangeLog entries.

Gcc/ChangeLog
2013-12-06  Balaji V. Iyer  

* omp-low.c (expand_simd_clones): Added a new parameter called "type."
(ipa_omp_simd_clone): Added a call to expand_simd_clones when Cilk Plus
is enabled.
(simd_clone_clauses_extract): Replaced the string "cilk simd elemental"
with "cilk simd function."

Gcc/c-family/Changelog
2013-12-06  Balaji V. Iyer  

* c-common.c (c_common_attribute_table): Added "cilk simd function"
attribute.

Gcc/c/ChangeLog
2013-12-06  Balaji V. Iyer  

* c-parser.c (struct c_parser::cilk_simd_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if cilk_simd_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_cilk_clause_vectorlength): Modified function to be shared
between SIMD-enabled functions and #pragma simd.  Changed return-type
to bool and added new parameter.
(c_parser_cilk_all_clauses): Modified the usage of the function
c_parser_cilk_clause_vectorlength as mentioned above.
(c_parser_cilk_simd_fn_expr_list): Likewise.
(c_finish_cilk_simd_fn_tokens): Likewise.
(c_parser_attributes): Added a cilk_simd_fn_tokens parameter.  Added a
check for vector attribute and if so call the function
c_parser_cilk_simd_fn_expr_list.  Also, when Cilk plus is enabled,
called the function c_finish_cilk_simd_fn_tokens.
(c_finish_omp_declare_simd): Added a check if cilk_simd_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.

Gcc/testsuite/ChangeLog
2013-12-06  Balaji V. Iyer  

* c-c++-common/cilk-plus/SE/ef_test.c: New test.
* c-c++-common/cilk-plus/SE/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/SE/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error.c: Likewise.
* c-c++-common/cilk-plus/SE/ef_error2.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.

Jakub, Is it Ok for branch?

Thanks,

Balaji V. Iyer.



> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Thursday, December 5, 2013 3:20 PM
> To: Iyer, Balaji V
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> On 11/30/13 20:38, Iyer, Balaji V wrote:
> > Hello Aldy,
> > Some of the middle end changes I made in the previous patch was
> not flying for the C++. Here is a fixed patch where the middle-end changes
> will work for both C and C++.
> > With this email, I am attaching the patch for C along with the middle
> end changes. Is this Ok for the branch?
> 
> Jakub and company ultimately have to approve your patch, but here are a
> few things.
> 
> > +
> > +  /* Cilk Plus specific parser/lexer information.  */
> > +
> > +  /* Buffer to hold all the tokens from parsing the vector attribute for 
> > the
> > + SIMD Enabled functions (formerly known as elemental functions).
> > + */  vec  *elem_fn_tokens;
> 
> If the name "elementals" is being phased out, then perhaps you need to
> come up with another name here.  Perhaps "cilk_simd_clone_tokens" or
> something that looks similar to the OpenMP variant
> "cilk_declare_simd_tokens" (akin to omp_declare_simd_clauses) in the rest
> of the patch.
> 

Fixed. I called it "cilk_simd_fn" instead of "elem_fn"

> Also, "Enabled" should not be capitalized.
> 

Fixed.


> > +/* Parses the vectorlength vector attribute for the SIMD Enabled
> functions
> > +   in Cilk Plus.
> > +   Syntax:
> > +   vectorlength ()  */
> > +
> > +static bool
> > +c_parser_elem_fn_vectorlength (c_parser *parser)
> 
> Similarly here.  Let's get rid of *elem* nomenclature throughout.
> Perhaps c_parser_cilk_declare_simd_vectorlength and similarly throughout
> the other parsing routines in the patch.  This will make it clearer that
> *cilk_declare_simd* is related to OpenMP's declare simd.
> 

Fixed.

> > +  if (TREE_CODE (value) != INTEGER_CST)
> > +{
> > +  error_at (token->location, "vectorlength must be a constant 
> > integer");
> > +  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
> > +  return false;
> > +}
> 
> I thought integer constant expressions were allowed here.

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

On 11/30/13 20:38, Iyer, Balaji V wrote:

Hello Aldy,
Some of the middle end changes I made in the previous patch was not 
flying for the C++. Here is a fixed patch where the middle-end changes will 
work for both C and C++.
With this email, I am attaching the patch for C along with the middle 
end changes. Is this Ok for the branch?


Jakub and company ultimately have to approve your patch, but here are a 
few things.



+
+  /* Cilk Plus specific parser/lexer information.  */
+
+  /* Buffer to hold all the tokens from parsing the vector attribute for the
+ SIMD Enabled functions (formerly known as elemental functions).  */
+  vec  *elem_fn_tokens;


If the name "elementals" is being phased out, then perhaps you need to 
come up with another name here.  Perhaps "cilk_simd_clone_tokens" or 
something that looks similar to the OpenMP variant 
"cilk_declare_simd_tokens" (akin to omp_declare_simd_clauses) in the 
rest of the patch.


Also, "Enabled" should not be capitalized.


+/* Parses the vectorlength vector attribute for the SIMD Enabled functions
+   in Cilk Plus.
+   Syntax:
+   vectorlength ()  */
+
+static bool
+c_parser_elem_fn_vectorlength (c_parser *parser)


Similarly here.  Let's get rid of *elem* nomenclature throughout. 
Perhaps c_parser_cilk_declare_simd_vectorlength and similarly throughout 
the other parsing routines in the patch.  This will make it clearer that 
*cilk_declare_simd* is related to OpenMP's declare simd.



+  if (TREE_CODE (value) != INTEGER_CST)
+{
+  error_at (token->location, "vectorlength must be a constant integer");
+  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
+  return false;
+}


I thought integer constant expressions were allowed here.  Shouldn't 
things like "sizeof (int)" be allowed?  See what is done in 
c_parser_cilk_clause_vectorlength() which handles constant expressions. 
 Also, you will need a corresponding test.


For that matter... can't we combine the above function with 
c_parser_cilk_clause_vectorlength().  It doesn't make much sense having 
two functions parsing the exact same clause.  Perhaps add a flag to it 
and have it work in two modes: one mode to create OMP_CLAUSE_SAFELEN and 
one mode to fill the token vector.



+  if (!c_parser_elem_fn_vectorlength (parser))
+{
+  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
+  /* NO reason to keep any of these tokens if the
+ vectorlength is messed up.  */
+ vec_free (parser->elem_fn_tokens);
+  return;


It may be cleaner to make the caller free the vector.


+  /* linear and uniform are the same between SIMD
+ enabled functions and #pragma omp declare simd.  */


Capitalize first word.


+  /* Do not push the last ')'  */
+  if (!(token->type == CPP_CLOSE_PAREN && paren_scope == 0))
+   vec_safe_push (parser->elem_fn_tokens, *token);


You're documenting the obvious.  Perhaps a better comment would be to 
explain why you do not push the last ')'.



+/* Parses the vector attribute of SIMD enabled functions in Cilk Plus.
+   Syntax:
+   vector
+   vector ().  */
+
+static void
+c_parser_elem_fn_expr_list (c_parser *parser, c_token vec_token)


Please document the second argument.



+  /* 2 EOF_token is added as a safety-net since the normal C front-end has
+ two token look-ahead.  */


"Two EOF tokens are added..."


 static void
-expand_simd_clones (struct cgraph_node *node)
+expand_simd_clones (struct cgraph_node *node, const char *type)


Document second argument, but perhaps we don't even need the second 
argument.  See below.



@@ -12337,7 +12336,10 @@
 {
   struct cgraph_node *node;
   FOR_EACH_FUNCTION (node)
-expand_simd_clones (node);
+expand_simd_clones (node, "omp declare simd");
+  if (flag_enable_cilkplus)
+FOR_EACH_FUNCTION (node)
+  expand_simd_clones (node, "cilk plus elemental");


First of all, it's a really bad idea to scan all the functions twice. 
You should have adapted expand_simd_clones() to do the work for both.


But even so, I don't think you need to do this at all.  Aren't Cilk Plus 
elementals supposed to be tagged as "omp declare simd" as well?  In 
which case expand_simd_clones() will DTRT.  It should...and then 
simd_clone_clauses_extract() already has the smarts to differentiate 
between the both variants.


Both OpenMP and Cilk Plus simd clones should have the "omp declare simd" 
attribute, and then Cilk Plus clones should *also* have the "cilk plus 
elemental" attribute.


Speak of which, we should probably rename the "cilk plus elemental" 
attribute throughout to "cilk plus declare simd" as I described earlier, 
but this is not your fault.  Bonus points if you want to do it as part 
of this patch :).



+dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -g" " 
"
+dg-runtest [lsort [glob -nocomplain $srcdir/c-c+

FW: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

PING!

-Balaji V. Iyer.

> -Original Message-
> From: Iyer, Balaji V
> Sent: Saturday, November 30, 2013 11:38 PM
> To: 'al...@redhat.com'
> Cc: 'Jakub Jelinek'; 'gcc-patches@gcc.gnu.org'
> Subject: RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> Hello Aldy,
>   Some of the middle end changes I made in the previous patch was
> not flying for the C++. Here is a fixed patch where the middle-end changes
> will work for both C and C++.
>   With this email, I am attaching the patch for C along with the middle
> end changes. Is this Ok for the branch?
> 
> Here are the ChangeLog entries:
> gcc/ChangeLog
> 2013-11-30  Balaji V. Iyer  
> 
> * omp-low.c (expand_simd_clones): Added a new parameter called
> "type."
> (ipa_omp_simd_clone): Added a call to expand_simd_clones when Cilk
> Plus
> is enabled.
> 
> gcc/c-family/ChangeLog
> 2013-11-30  Balaji V. Iyer  
> 
> * c-common.c (c_common_attribute_table): Added "cilk plus elemental"
> attribute.
> 
> gcc/c/ChangeLog
> 2013-11-30  Balaji V. Iyer  
> 
> * c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
> (c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
> field in parser is not empty.  If not-empty, call the function
> c_parser_finish_omp_declare_simd.
> (c_parser_elem_fn_vectorlength): New function.
> (c_parser_elem_fn_expr_list): Likewise.
> (c_finish_elem_fn_tokens): Likewise.
> (c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
> check for vector attribute and if so call c_parser_elem_fn_expr_list.
> Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
> (c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
> parser field is non-empty.  If so, parse them as you would parse
> the omp declare simd pragma.
> 
> gcc/testsuite/ChangeLog
> 2013-11-30  Balaji V. Iyer  
> 
> * c-c++-common/cilk-plus/EF/ef_test.c: New test.
> * c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
> * c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
> * c-c++-common/cilk-plus/EF/ef_error.c: Likewise.
> * c-c++-common/cilk-plus/EF/ef_error2.c: Likewise.
> * gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> 
> > -Original Message-----
> > From: Iyer, Balaji V
> > Sent: Wednesday, November 27, 2013 1:15 PM
> > To: al...@redhat.com
> > Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org
> > Subject: RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> > Elemental functions) for C
> >
> > HI Aldy and Jakub,
> > Attached, please find a fixed patch. I have fixed all the changes you
> > have mentioned below. Is this OK to install?
> >
> > Here are the ChangeLog entries:
> > gcc/ChangeLog
> > 2013-11-27  Balaji V. Iyer  
> >
> > * config/i386/i386.c
> (ix86_simd_clone_compute_vecsize_and_simdlen):
> > Removed a carriage return from the warning string.
> > * omp-low.c (simd_clone_clauses_extract): Added a check for cilk 
> > plus
> > SIMD-enabled function attributes.
> >
> > gcc/c/ChangeLog
> > 2013-11-27  Balaji V. Iyer  
> >
> > * c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
> > (c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
> > field in parser is not empty.  If not-empty, call the function
> > c_parser_finish_omp_declare_simd.
> > (c_parser_elem_fn_vectorlength): New function.
> > (c_parser_elem_fn_expr_list): Likewise.
> > (c_finish_elem_fn_tokens): Likewise.
> > (c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
> > check for vector attribute and if so call 
> > c_parser_elem_fn_expr_list.
> > Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
> > (c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
> > parser field is non-empty.  If so, parse them as you would parse
> > the omp declare simd pragma.
> >
> > gcc/testsuite/ChangeLog
> > 2013-11-27  Balaji V. Iyer  
> >
> > * c-c++-common/cilk-plus/EF/ef_test.c: New test.
> > * c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
> > * c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
> > * c-c++-common/cilk-plus/EF

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hello Aldy,
Some of the middle end changes I made in the previous patch was not 
flying for the C++. Here is a fixed patch where the middle-end changes will 
work for both C and C++.
With this email, I am attaching the patch for C along with the middle 
end changes. Is this Ok for the branch?

Here are the ChangeLog entries:
gcc/ChangeLog
2013-11-30  Balaji V. Iyer  

* omp-low.c (expand_simd_clones): Added a new parameter called "type."
(ipa_omp_simd_clone): Added a call to expand_simd_clones when Cilk Plus
is enabled.

gcc/c-family/ChangeLog
2013-11-30  Balaji V. Iyer  

* c-common.c (c_common_attribute_table): Added "cilk plus elemental"
attribute.

gcc/c/ChangeLog
2013-11-30  Balaji V. Iyer  

* c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_elem_fn_vectorlength): New function.
(c_parser_elem_fn_expr_list): Likewise.
(c_finish_elem_fn_tokens): Likewise.
(c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
check for vector attribute and if so call c_parser_elem_fn_expr_list.
Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
(c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.

gcc/testsuite/ChangeLog
2013-11-30  Balaji V. Iyer  

* c-c++-common/cilk-plus/EF/ef_test.c: New test.
* c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/EF/ef_error.c: Likewise.
* c-c++-common/cilk-plus/EF/ef_error2.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.

Thanks,

Balaji V. Iyer.


> -Original Message-
> From: Iyer, Balaji V
> Sent: Wednesday, November 27, 2013 1:15 PM
> To: al...@redhat.com
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org
> Subject: RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> HI Aldy and Jakub,
>   Attached, please find a fixed patch. I have fixed all the changes you
> have mentioned below. Is this OK to install?
> 
> Here are the ChangeLog entries:
> gcc/ChangeLog
> 2013-11-27  Balaji V. Iyer  
> 
> * config/i386/i386.c (ix86_simd_clone_compute_vecsize_and_simdlen):
> Removed a carriage return from the warning string.
> * omp-low.c (simd_clone_clauses_extract): Added a check for cilk plus
> SIMD-enabled function attributes.
> 
> gcc/c/ChangeLog
> 2013-11-27  Balaji V. Iyer  
> 
> * c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
> (c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
> field in parser is not empty.  If not-empty, call the function
> c_parser_finish_omp_declare_simd.
> (c_parser_elem_fn_vectorlength): New function.
> (c_parser_elem_fn_expr_list): Likewise.
> (c_finish_elem_fn_tokens): Likewise.
> (c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
> check for vector attribute and if so call c_parser_elem_fn_expr_list.
> Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
> (c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
> parser field is non-empty.  If so, parse them as you would parse
> the omp declare simd pragma.
> 
> gcc/testsuite/ChangeLog
> 2013-11-27  Balaji V. Iyer  
> 
> * c-c++-common/cilk-plus/EF/ef_test.c: New test.
> * c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
> * c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
> * c-c++-common/cilk-plus/EF/ef_error.c: Likewise.
> * c-c++-common/cilk-plus/EF/ef_error2.c: Likewise.
> * gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.
> 
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> > -Original Message-
> > From: Aldy Hernandez [mailto:al...@redhat.com]
> > Sent: Wednesday, November 27, 2013 10:52 AM
> > To: Iyer, Balaji V
> > Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org
> > Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> > Elemental functions) for C
> >
> > "Iyer, Balaji V"  writes:
> >
> > >  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
> > >  vec clauses)
> > >  {
> > > +
> > > +  if (flag_enable_cilkplus
> > >

RE: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

HI Aldy and Jakub,
Attached, please find a fixed patch. I have fixed all the changes you 
have mentioned below. Is this OK to install?

Here are the ChangeLog entries:
gcc/ChangeLog
2013-11-27  Balaji V. Iyer  

* config/i386/i386.c (ix86_simd_clone_compute_vecsize_and_simdlen):
Removed a carriage return from the warning string.
* omp-low.c (simd_clone_clauses_extract): Added a check for cilk plus
SIMD-enabled function attributes.

gcc/c/ChangeLog
2013-11-27  Balaji V. Iyer  

* c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
(c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
field in parser is not empty.  If not-empty, call the function
c_parser_finish_omp_declare_simd.
(c_parser_elem_fn_vectorlength): New function.
(c_parser_elem_fn_expr_list): Likewise.
(c_finish_elem_fn_tokens): Likewise.
(c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
check for vector attribute and if so call c_parser_elem_fn_expr_list.
Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
(c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
parser field is non-empty.  If so, parse them as you would parse
the omp declare simd pragma.

gcc/testsuite/ChangeLog
2013-11-27  Balaji V. Iyer  

* c-c++-common/cilk-plus/EF/ef_test.c: New test.
* c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
* c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
* c-c++-common/cilk-plus/EF/ef_error.c: Likewise.
* c-c++-common/cilk-plus/EF/ef_error2.c: Likewise.
* gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.


Thanks,

Balaji V. Iyer.

> -Original Message-
> From: Aldy Hernandez [mailto:al...@redhat.com]
> Sent: Wednesday, November 27, 2013 10:52 AM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org
> Subject: Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> Elemental functions) for C
> 
> "Iyer, Balaji V"  writes:
> 
> >  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
> >vec clauses)
> >  {
> > +
> > +  if (flag_enable_cilkplus
> > +  && clauses.exists () && !vec_safe_is_empty (parser-
> >elem_fn_tokens))
> > +{
> > +  error ("%<#pragma omp declare simd%> cannot be used in the same"
> > +"function marked as a SIMD-enabled function");
> > +  vec_free (parser->elem_fn_tokens);
> > +  return;
> > +}
> 
> I see Cilk Plus elementals are handled as omp declare simd in the above
> function.  This function sets an "omp declare simd" attribute here:
> 
>  if (c != NULL_TREE)
> c = tree_cons (NULL_TREE, c, NULL_TREE);
>   c = build_tree_list (get_identifier ("omp declare simd"), c);
>   TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
>   DECL_ATTRIBUTES (fndecl) = c;
> 
> but you also need a "cilk plus elemental" attribute so the rest of the 
> compiler
> can differentiate Cilk Plus elementals from omp declare simds.
> 

Fixed.

> See simd_clone_clauses_extract():
> 
> +  /* To distinguish from an OpenMP simd clone, Cilk Plus functions to
> + be cloned have a distinctive artificial label in addition to "omp
> + declare simd".  */
> +  bool cilk_clone
> += (flag_enable_cilkplus
> +   && lookup_attribute ("cilk plus elemental",
> +   DECL_ATTRIBUTES (node->decl)));
> 
> Also you probably want some kind of test for this, so test for whatever
> cilk_elemental is doing.  In trunk, the only use of cilk_elemental is in
> ix86_simd_clone_compute_vecsize_and_simdlen(), so come up with some
> x86 specific test for cilk_elemental==true.
> 

Fixed. 

> Aldy
Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c(revision 205457)
+++ gcc/c/c-parser.c(working copy)
@@ -208,6 +208,12 @@
   /* True if we are in a context where the Objective-C "Property attribute"
  keywords are valid.  */
   BOOL_BITFIELD objc_property_attr_context : 1;
+
+  /* Cilk Plus specific parser/lexer information.  */
+
+  /* Buffer to hold all the tokens from parsing the vector attribute for the
+ SIMD Enabled functions (formerly known as elemental functions).  */
+  vec  *elem_fn_tokens;
 } c_parser;
 
 
@@ -1647,7 +1653,8 @@
C_DTR_NORMAL, &dummy);
   if (declarator == NULL)
{
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_i

Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez]

"Iyer, Balaji V"  writes:

>  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
>  vec clauses)
>  {
> +
> +  if (flag_enable_cilkplus
> +  && clauses.exists () && !vec_safe_is_empty (parser->elem_fn_tokens))
> +{
> +  error ("%<#pragma omp declare simd%> cannot be used in the same"
> +  "function marked as a SIMD-enabled function");
> +  vec_free (parser->elem_fn_tokens);
> +  return;
> +}

I see Cilk Plus elementals are handled as omp declare simd in the above
function.  This function sets an "omp declare simd" attribute here:

 if (c != NULL_TREE)
c = tree_cons (NULL_TREE, c, NULL_TREE);
  c = build_tree_list (get_identifier ("omp declare simd"), c);
  TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
  DECL_ATTRIBUTES (fndecl) = c;

but you also need a "cilk plus elemental" attribute so the rest of the
compiler can differentiate Cilk Plus elementals from omp declare simds.

See simd_clone_clauses_extract():

+  /* To distinguish from an OpenMP simd clone, Cilk Plus functions to
+ be cloned have a distinctive artificial label in addition to "omp
+ declare simd".  */
+  bool cilk_clone
+= (flag_enable_cilkplus
+   && lookup_attribute ("cilk plus elemental",
+   DECL_ATTRIBUTES (node->decl)));

Also you probably want some kind of test for this, so test for whatever
cilk_elemental is doing.  In trunk, the only use of cilk_elemental is in
ix86_simd_clone_compute_vecsize_and_simdlen(), so come up with some
x86 specific test for cilk_elemental==true.

Aldy

[PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Iyer, Balaji V]

Hi Jakub,
Did you get a chance to look at this patch? Is it Ok to install?

Thanks,

Balaji V. Iyer.

> -Original Message-
> From: Iyer, Balaji V
> Sent: Tuesday, November 19, 2013 5:09 PM
> To: Jakub Jelinek
> Cc: Joseph S. Myers; gcc-patches@gcc.gnu.org; Aldy Hernandez
> (al...@redhat.com); Jeff Law
> Subject: RE: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental
> functions) for C
> 
> 
> 
> > -Original Message-
> > From: Jakub Jelinek [mailto:ja...@redhat.com]
> > Sent: Tuesday, November 19, 2013 12:00 PM
> > To: Iyer, Balaji V
> > Cc: Joseph S. Myers; gcc-patches@gcc.gnu.org; Aldy Hernandez
> > (al...@redhat.com); Jeff Law
> > Subject: Re: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly
> > Elemental
> > functions) for C
> >
> > On Tue, Nov 19, 2013 at 04:54:29PM +, Iyer, Balaji V wrote:
> > > I just need a clarification, so I am sorry if I am making you repeat:
> > >
> > > Right now, the array of tokens (vec elem_fn_tokens) is
> > > passed in as a parameter and then passed back to the
> > > c_parser_declaration_or_fndef function.
> > >
> > > Instead of that, you would like this information to be stored in
> > > parser->specs (using the c_parser_declspecs function) and then have
> > > parser->my own
> >
> > No, somewhere in *parser, it can be a new field with vector of the
> > vec vectors or similar and store something that can be
> > used to find that out into vector attribute argument.  Then at the
> > spot where the #pragma omp declare simd tokens are parsed right now
> > (when arguments are finally available) you could look up the vector
> > attribute and parse the corresponding tokens.
> 
> Hi Jakub,
>  Thanks for all your help and advice on this. I have made the changes as 
> you
> suggested. I added a new field to parser to hold the tokens for elemental
> functions. Also removed the call for c_parser_attributes that I added in
> c_parser_declaration_or_fndef and then let c_parser_declspec handle the
> token collection.  Also, I added a check for usage of #pragma omp declare
> simd and __attribute__((vector...)) and flag them as error, also added a test
> to catch this.
> 
> Also, fixed the spelling mistake in a comment and the mis-comment I made
> about OpenMP requiring two CPP_EOF. Is this Patch OK to install?
> 
> Here is the Fixed ChangeLog entry:
> gcc/c/ChangeLog
> 2013-11-19  Balaji V. Iyer  
> 
> * c-parser.c (struct c_parser::elem_fn_tokens): Added new field.
> (c_parser_declaration_or_fndef): Added a check if elem_fn_tokens
> field in parser is not empty.  If not-empty, call the function
> c_parser_finish_omp_declare_simd.
> (c_parser_elem_fn_vectorlength): New function.
> (c_parser_elem_fn_expr_list): Likewise.
> (c_finish_elem_fn_tokens): Likewise.
> (c_parser_attributes): Added a elem_fn_tokens parameter.  Added a
> check for vector attribute and if so call c_parser_elem_fn_expr_list.
> Also, called c_finish_elem_fn_tokens when Cilk Plus is enabled.
> (c_finish_omp_declare_simd): Added a check if elem_fn_tokens in
> parser field is non-empty.  If so, parse them as you would parse
> the omp declare simd pragma.
> 
> gcc/testsuite/ChangeLog
> 2013-11-19  Balaji V. Iyer  
> 
> * c-c++-common/cilk-plus/EF/ef_test.c: New test.
> * c-c++-common/cilk-plus/EF/ef_test2.c: Likewise.
> * c-c++-common/cilk-plus/EF/vlength_errors.c: Likewise.
> * c-c++-common/cilk-plus/EF/ef_error.c: Likewise.
> * gcc.dg/cilk-plus/cilk-plus.exp: Added calls for the above tests.
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> >
> > Jakub
Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c(revision 205055)
+++ gcc/c/c-parser.c(working copy)
@@ -203,6 +203,12 @@
   /* True if we are in a context where the Objective-C "Property attribute"
  keywords are valid.  */
   BOOL_BITFIELD objc_property_attr_context : 1;
+
+  /* Cilk Plus specific parser/lexer information.  */
+
+  /* Buffer to hold all the tokens from parsing the vector attribute for the
+ SIMD-enabled functions (formerly known as elemental functions).  */
+  vec  *elem_fn_tokens;
 } c_parser;
 
 
@@ -1642,7 +1648,8 @@
C_DTR_NORMAL, &dummy);
   if (declarator == NULL)
{
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses.exists ()
+ || !vec_safe_is_empty (parser->elem_fn_tokens))
c_finish_omp_declare_simd (parser, NULL_TREE, NULL_TREE,
   omp_declare_simd_clauses);
  c_parser_skip_to_end_of_block_or_statement (parser);
@@ -1729,7 +1736,8 @@
  chainon (postfix_attrs, all_prefix_attrs));
  if (!d)
d = error_mark_node;
- if (omp_declare_simd_clauses.exists ())
+