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

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

Hi Jakub,
	I have fixed all the issues you have mentioned below. I am also attaching a fixed patch along with ChangeLog entries.  I have also rebased the patch to the trunk revision r206786.

	Is this Ok to install?

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Friday, January 17, 2014 12:46 PM
> To: Iyer, Balaji V
> Cc: 'Aldy Hernandez (aldyh@redhat.com)'; 'gcc-patches@gcc.gnu.org'
> Subject: Re: [GOMP4][PATCH] SIMD-enabled functions (formerly Elemental
> functions) for C++
> 
> On Thu, Dec 19, 2013 at 06:12:29PM +0000, Iyer, Balaji V wrote:
> > 2013-12-19  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >
> >         * 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"
> 
> The comma should have been after " .
> 

Fixed.

> > +		  /* 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_next_tokens_can_be_attribute_p (parser))
> 
> As you are just calling cp_parser_gnu_attributes_opt here and not ..._std_...,
> I'd say the above should be cp_next_tokens_can_be_gnu_attribute_p
> rather than cp_next_tokens_can_be_attribute_p.  I think [[...]] attributes at
> this position are ignored, so no need to handle them, not sure about
> whether we allow e.g. combination of GNU and std attributes or vice versa.
> 

Fixed.

> > +		    {
> > +		      cp_parser_parse_tentatively (parser);
> > +		      tree attr = cp_parser_gnu_attributes_opt (parser);
> > +		      if (cp_lexer_next_token_is_not (parser->lexer,
> > +						      CPP_SEMICOLON)
> > +			  && cp_lexer_next_token_is_not (parser->lexer,
> > +							 CPP_OPEN_BRACE))
> > +			cp_parser_abort_tentative_parse (parser);
> > +		      else if (!cp_parser_parse_definitely (parser))
> > +			;
> > +		      else
> > +			attrs = chainon (attr, attrs);
> > +		    }
> >  		  late_return = (cp_parser_late_return_type_opt
> >  				 (parser, declarator,
> >  				  memfn ? cv_quals : -1));
> 
> > @@ -17842,6 +17868,10 @@ cp_parser_late_return_type_opt (cp_parser*
> parser, cp_declarator *declarator,
> >        type = cp_parser_trailing_type_id (parser);
> >      }
> >
> > +  if (cilk_simd_fn_vector_p)
> > +    declarator->std_attributes
> > +      = cp_parser_late_parsing_cilk_simd_fn_info (parser,
> > +					     declarator->std_attributes);
> 
> Please make sure declarator is aligned below parser.
> 

Fixed.

> > +  token->type = CPP_PRAGMA_EOL;
> > +  parser->lexer->next_token = token;
> > +  cp_lexer_consume_token (parser->lexer);
> > +
> > +  struct cp_token_cache *cp =
> > +    cp_token_cache_new (v_token, cp_lexer_peek_token
> > + (parser->lexer));
> 
> The = should already go on the next line.
> 

Fixed.

> > +/* Handles the delayed parsing of the Cilk Plus SIMD-enabled function.
> > +   This function is modelled similar to the late parsing of omp declare
> > +   simd.  */
> > +
> > +static tree
> > +cp_parser_late_parsing_cilk_simd_fn_info (cp_parser *parser, tree
> > +attrs) {
> > +  struct cp_token_cache *ce;
> > +  cp_omp_declare_simd_data *info = parser->cilk_simd_fn_info;
> > +  int ii = 0;
> > +
> > +  if (parser->omp_declare_simd != NULL)
> > +    {
> > +      error ("%<#pragma omp declare simd%> cannot be used in the same
> function"
> > +	     " marked as a Cilk Plus SIMD-enabled function");
> > +      parser->cilk_simd_fn_info = NULL;
> 
> This will leak parser->cilk_simd_fn_info memory.  Please XDELETE it first.
> 

Fixed

> > +      return attrs;
> > +    }
> > +  if (!info->error_seen && info->fndecl_seen)
> > +    {
> > +      error ("vector attribute not immediately followed by a single function"
> > +	     " declaration or definition");
> > +      info->error_seen = true;
> > +    }
> > +  if (info->error_seen)
> > +    return attrs;
> > +
> > +  /* Vector attributes are converted to #pragma omp declare simd values
> and
> > +     so we need them enabled.  */
> > +  flag_openmp = 1;
> 
> The C FE doesn't do this.  I thought all the omp-low.c spots are now guarded
> by flag_openmp || flag_enable_cilkplus etc. conditions.

Fixed. The above "orring" was omitted in decl.c and pt.c . I have put them in.

> 
> > +      c = build_tree_list (get_identifier ("cilk simd function"),
> > + cl);
> 
> Please use NULL_TREE instead of cl here and please fix up the C FE as well,
> the "cilk simd function" attribute should be just an attribute without value,
> serving only as a boolean, if present, decl with "omp declare simd" attribute
> is Cilk+, otherwise OpenMP.
> 
> > +      TREE_CHAIN (c) = attrs;
> > +      if (processing_template_decl)
> > +	ATTR_IS_DEPENDENT (c) = 1;
> 

Fixed.

> But, as a boolean attribute it doesn't and shouldn't be ATTR_IS_DEPENDENT,
> there is nothing to adjust in it.
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -8602,9 +8602,12 @@ apply_late_template_attributes (tree *decl_p,
> tree attributes, int attr_flags,
> >  	    {
> >  	      *p = TREE_CHAIN (t);
> >  	      TREE_CHAIN (t) = NULL_TREE;
> > -	      if (flag_openmp
> > -		  && is_attribute_p ("omp declare simd",
> > -				     get_attribute_name (t))
> > +	      if (((flag_openmp
> > +		    && is_attribute_p ("omp declare simd",
> > +				       get_attribute_name (t)))
> > +		   || (flag_enable_cilkplus
> > +		       && is_attribute_p ("cilk simd function",
> > +					  get_attribute_name (t))))
> >  		  && TREE_VALUE (t))
> >  		{
> >  		  tree clauses = TREE_VALUE (TREE_VALUE (t));
> 
> And this change is unnecessary after the above suggested change.
> 

Fixed.

> 	Jakub

Attachment: diff.txt
Description: diff.txt

Attachment: ChangeLog.elem_fn
Description: ChangeLog.elem_fn