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,
	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  <balaji.v.iyer@intel.com>

	* 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  <balaji.v.iyer@intel.com>

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

Gcc/c/ChangeLog
2013-12-16  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* 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  <balaji.v.iyer@intel.com>

	* 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-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@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 +0000, 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 (<vector attributes>).  */
> > +
> > +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.
> I'd prefer if you just keep the tokens as is, and simply:
> 

OK. I have removed the token tweaking of both places.

> >    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.
> 

Yes, I allowed all the vector variants you mentioned. Fixed in the patch.

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

Yes, I am planning to do it after I finish submitting all the Cilk Plus parts.

> 
> >        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.
> 

Yep, used a different mask. Fixed.

> 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.
> 

In this release, we are not supporting having parameter as a step-size for linear. I am planning to add it in the next release of the compiler. I have added an error case to catch such instances and also added a testsuite case to test this.

> 	Jakub

Attachment: diff6.txt
Description: diff6.txt