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

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

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-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@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 +0000, 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 <c_token, va_gc> *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 %<linear%> 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)
> >  	{
> > @@ -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.
> 

The reason why I do mask == CILK_SIMD_FN_CLAUSE_MASk is to set the bool parameter to true if we are compiling the Cilk Plus SIMD-enabled function. This bool is checked to give the sorry error that using parameter for step-size is not implemented.

> 	Jakub

Attachment: diff7.txt
Description: diff7.txt