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