| Message ID | BF230D13CA30DD48930C31D4099330003A4B23A0@FMSMSX101.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
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 > +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 %<linear%> 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
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
On Tue, Dec 17, 2013 at 03:51:14AM +0000, 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
Hi! For reference, here's my rationale for OpenACC on this topic: On Tue, 17 Dec 2013 07:17:31 +0100, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Dec 17, 2013 at 03:51:14AM +0000, 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
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
Hi! On Tue, 17 Dec 2013 11:27:51 +0100, Jakub Jelinek <jakub@redhat.com> 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
> -----Original Message----- > From: Jakub Jelinek [mailto:jakub@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 +0000, 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
On Mon, 16 Dec 2013, Jakub Jelinek wrote: > 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? Fine with me.
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 <c_token, va_gc> *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_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, current_function_decl, NULL_TREE, omp_declare_simd_clauses); DECL_STRUCT_FUNCTION (current_function_decl)->function_start_locus @@ -3765,6 +3777,134 @@ return attr_name; } +/* 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_OMP_CLAUSE_SIMDLEN; + else if (!strcmp (p, "uniform")) + result = PRAGMA_OMP_CLAUSE_UNIFORM; + else if (!strcmp (p, "linear")) + result = PRAGMA_OMP_CLAUSE_LINEAR; + else if (!strcmp (p, "mask")) + result = PRAGMA_OMP_CLAUSE_INBRANCH; + else if (!strcmp (p, "nomask")) + result = PRAGMA_OMP_CLAUSE_NOTINBRANCH; + + if (result != PRAGMA_OMP_CLAUSE_NONE) + c_parser_consume_token (parser); + return result; +} + +/* Returns true of NAME is an IDENTIFIER_NODE with identiifer "vector," + "__vector" or "__vector__." */ + +static 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; +} + +#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)) + +/* 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 (is_cilkplus_vector_p (vec_token.value)); + + int paren_scope = 0; + vec_safe_push (parser->cilk_simd_fn_tokens, vec_token); + /* Consume the "vector" token. */ + c_parser_consume_token (parser); + + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)) + { + c_parser_consume_token (parser); + paren_scope++; + } + while (paren_scope > 0) + { + c_token *token = c_parser_peek_token (parser); + if (token->type == CPP_OPEN_PAREN) + paren_scope++; + else if (token->type == CPP_CLOSE_PAREN) + paren_scope--; + if (token->type == CPP_NAME + && TREE_CODE (token->value) == IDENTIFIER_NODE) + if (simple_cst_equal (token->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; + } + /* Do not push the last ')' since we are not pushing the '('. */ + if (!(token->type == CPP_CLOSE_PAREN && paren_scope == 0)) + vec_safe_push (parser->cilk_simd_fn_tokens, *token); + c_parser_consume_token (parser); + } + + /* Since we are converting an attribute to a pragma, we need to end the + attribute with PRAGMA_EOL. */ + c_token eol_token; + memset (&eol_token, 0, sizeof (eol_token)); + eol_token.type = CPP_PRAGMA_EOL; + vec_safe_push (parser->cilk_simd_fn_tokens, eol_token); +} + +/* Add 2 CPP_EOF at the end of PARSER->ELEM_FN_TOKENS vector. */ + +static void +c_finish_cilk_simd_fn_tokens (c_parser *parser) +{ + c_token last_token = parser->cilk_simd_fn_tokens->last (); + + /* c_parser_attributes is called in several places, so if these EOF + tokens are already inserted, then don't do them again. */ + if (last_token.type == CPP_EOF) + return; + + /* Two CPP_EOF token are added as a safety net since the normal C + front-end has two token look-ahead. */ + c_token eof_token; + eof_token.type = CPP_EOF; + vec_safe_push (parser->cilk_simd_fn_tokens, eof_token); + vec_safe_push (parser->cilk_simd_fn_tokens, eof_token); +} + /* Parse (possibly empty) attributes. This is a GNU extension. attributes: @@ -3829,6 +3969,12 @@ attr_name = c_parser_attribute_any_word (parser); if (attr_name == NULL) break; + if (is_cilkplus_vector_p (attr_name)) + { + c_token *v_token = c_parser_peek_token (parser); + c_parser_cilk_simd_fn_vector_attrs (parser, *v_token); + continue; + } c_parser_consume_token (parser); if (c_parser_next_token_is_not (parser, CPP_OPEN_PAREN)) { @@ -3909,6 +4055,9 @@ } parser->lex_untranslated_string = false; } + + if (flag_enable_cilkplus && !vec_safe_is_empty (parser->cilk_simd_fn_tokens)) + c_finish_cilk_simd_fn_tokens (parser); return attrs; } @@ -10401,7 +10550,7 @@ linear ( variable-list : expression ) */ static tree -c_parser_omp_clause_linear (c_parser *parser, tree list) +c_parser_omp_clause_linear (c_parser *parser, tree list, bool is_cilk_simd_fn) { location_t clause_loc = c_parser_peek_token (parser)->location; tree nl, c, step; @@ -10418,10 +10567,16 @@ 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) + { + error_at (clause_loc, "using parameters for %<linear%> step is " + "not supported in this release"); + step = integer_one_node; + } if (!INTEGRAL_TYPE_P (TREE_TYPE (step))) { error_at (clause_loc, "%<linear%> clause step expression must " - "be integral"); + "be integral"); step = integer_one_node; } @@ -10789,7 +10944,11 @@ c_parser_consume_token (parser); here = c_parser_peek_token (parser)->location; - 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); 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); c_name = "linear"; break; case PRAGMA_OMP_CLAUSE_DEPEND: @@ -12754,10 +12914,20 @@ c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms, vec<c_token> clauses) { + + if (flag_enable_cilkplus + && clauses.exists () && !vec_safe_is_empty (parser->cilk_simd_fn_tokens)) + { + error ("%<#pragma omp declare simd%> cannot be used in the same " + "function marked as a Cilk Plus SIMD-enabled function"); + vec_free (parser->cilk_simd_fn_tokens); + return; + } + /* Normally first token is CPP_NAME "simd". CPP_EOF there indicates error has been reported and CPP_PRAGMA that c_finish_omp_declare_simd has already processed the tokens. */ - if (clauses[0].type == CPP_EOF) + if (clauses.exists () && clauses[0].type == CPP_EOF) return; if (fndecl == NULL_TREE || TREE_CODE (fndecl) != FUNCTION_DECL) { @@ -12766,7 +12936,7 @@ clauses[0].type = CPP_EOF; return; } - if (clauses[0].type != CPP_NAME) + if (clauses.exists () && clauses[0].type != CPP_NAME) { error_at (DECL_SOURCE_LOCATION (fndecl), "%<#pragma omp declare simd%> not immediately followed by " @@ -12780,23 +12950,49 @@ unsigned int tokens_avail = parser->tokens_avail; gcc_assert (parser->tokens == &parser->tokens_buf[0]); - parser->tokens = clauses.address (); - parser->tokens_avail = clauses.length (); - + bool is_cilkplus_cilk_simd_fn = false; + + if (flag_enable_cilkplus && !vec_safe_is_empty (parser->cilk_simd_fn_tokens)) + { + parser->tokens = parser->cilk_simd_fn_tokens->address (); + parser->tokens_avail = vec_safe_length (parser->cilk_simd_fn_tokens); + is_cilkplus_cilk_simd_fn = true; + } + else + { + parser->tokens = clauses.address (); + parser->tokens_avail = clauses.length (); + } + /* c_parser_omp_declare_simd pushed 2 extra CPP_EOF tokens at the end. */ while (parser->tokens_avail > 3) { c_token *token = c_parser_peek_token (parser); - gcc_assert (token->type == CPP_NAME - && strcmp (IDENTIFIER_POINTER (token->value), "simd") == 0); + if (!is_cilkplus_cilk_simd_fn) + gcc_assert (token->type == CPP_NAME + && strcmp (IDENTIFIER_POINTER (token->value), "simd") == 0); + else + gcc_assert (token->type == CPP_NAME + && is_cilkplus_vector_p (token->value)); 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, CILK_SIMD_FN_CLAUSE_MASK, + "SIMD-enabled functions attribute"); + else + c = c_parser_omp_all_clauses (parser, OMP_DECLARE_SIMD_CLAUSE_MASK, + "#pragma omp declare simd"); c = c_omp_declare_simd_clauses_to_numbers (parms, c); if (c != NULL_TREE) c = tree_cons (NULL_TREE, c, NULL_TREE); + if (is_cilkplus_cilk_simd_fn) + { + tree k = build_tree_list (get_identifier ("cilk simd function"), c); + TREE_CHAIN (k) = DECL_ATTRIBUTES (fndecl); + DECL_ATTRIBUTES (fndecl) = k; + } c = build_tree_list (get_identifier ("omp declare simd"), c); TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl); DECL_ATTRIBUTES (fndecl) = c; @@ -12804,7 +13000,11 @@ parser->tokens = &parser->tokens_buf[0]; parser->tokens_avail = tokens_avail; - clauses[0].type = CPP_PRAGMA; + if (clauses.exists ()) + clauses[0].type = CPP_PRAGMA; + + if (!vec_safe_is_empty (parser->cilk_simd_fn_tokens)) + vec_free (parser->cilk_simd_fn_tokens); } @@ -13400,39 +13600,110 @@ } /* 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. The main purpose of this function is to parse a + vectorlength attribute or clause and check for parse errors. + When IS_SIMD_FN is true then the function is merely caching the tokens + in PARSER->CILK_SIMD_FN_TOKENS. If errors are found then the token + cache is cleared since there is no reason to continue. + 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) { - /* The vectorlength clause behaves exactly like OpenMP's safelen - clause. Represent it in OpenMP terms. */ - check_no_duplicate_clause (clauses, OMP_CLAUSE_SAFELEN, "vectorlength"); + c_token *token = c_parser_peek_token (parser); + if (is_simd_fn) + { + gcc_assert (simple_cst_equal (token->value, + get_identifier ("vectorlength")) == 1); + // token->value = get_identifier ("simdlen"); + vec_safe_push (parser->cilk_simd_fn_tokens, *token); + c_parser_consume_token (parser); + } + else + /* The vectorlength clause behaves exactly like OpenMP's safelen + clause. Represent it in OpenMP terms. */ + check_no_duplicate_clause (*clauses, OMP_CLAUSE_SAFELEN, "vectorlength"); + token = c_parser_peek_token (parser); if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) - return clauses; + { + /* No reason to keep any of these tokens if the + vectorlength is messed up. */ + vec_free (parser->cilk_simd_fn_tokens); + return false; + } + if (is_simd_fn) + vec_safe_push (parser->cilk_simd_fn_tokens, *token); + + *token = *c_parser_peek_token (parser); location_t loc = c_parser_peek_token (parser)->location; tree expr = c_parser_expr_no_commas (parser, NULL).value; expr = c_fully_fold (expr, false, NULL); + bool error_found = false; - if (!TREE_TYPE (expr) - || !TREE_CONSTANT (expr) - || !INTEGRAL_TYPE_P (TREE_TYPE (expr))) - error_at (loc, "vectorlength must be an integer constant"); + /* If expr is error_mark_node, then the returning function would have + flagged the error. No need to flag it twice. */ + if (expr == error_mark_node) + error_found = true; + else if (!TREE_TYPE (expr) + || !TREE_CONSTANT (expr) + || !INTEGRAL_TYPE_P (TREE_TYPE (expr))) + { + error_at (loc, "vectorlength must be an integer constant"); + error_found = true; + } else if (exact_log2 (TREE_INT_CST_LOW (expr)) == -1) - error_at (loc, "vectorlength must be a power of 2"); + { + error_at (loc, "vectorlength must be a power of 2"); + error_found = true; + } else { - tree u = build_omp_clause (loc, OMP_CLAUSE_SAFELEN); - OMP_CLAUSE_SAFELEN_EXPR (u) = expr; - OMP_CLAUSE_CHAIN (u) = clauses; - clauses = u; + if (is_simd_fn) + { + c_token new_token; + /* If we are here then it has be a number or an integral constant. + The function that checks this token only checks to see if it is + a keyword or name, so we set it to the most common case. */ + new_token.type = CPP_NUMBER; + new_token.keyword = RID_MAX; + new_token.value = expr; + vec_safe_push (parser->cilk_simd_fn_tokens, new_token); + } + else + { + tree u = build_omp_clause (loc, OMP_CLAUSE_SAFELEN); + OMP_CLAUSE_SAFELEN_EXPR (u) = expr; + OMP_CLAUSE_CHAIN (u) = *clauses; + *clauses = u; + } } + if (error_found && is_simd_fn) + { + 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->cilk_simd_fn_tokens); + return false; + } - c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>"); + token = c_parser_peek_token (parser); + if (!c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>")) + { + /* No reason to keep any of these tokens if the + vectorlength is messed up. */ + vec_free (parser->cilk_simd_fn_tokens); + return false; + } - return clauses; + if (is_simd_fn) + vec_safe_push (parser->cilk_simd_fn_tokens, *token); + return true; } /* Cilk Plus: @@ -13571,7 +13842,7 @@ switch (c_kind) { case PRAGMA_CILK_CLAUSE_VECTORLENGTH: - clauses = c_parser_cilk_clause_vectorlength (parser, clauses); + c_parser_cilk_clause_vectorlength (parser, &clauses, false); break; case PRAGMA_CILK_CLAUSE_LINEAR: clauses = c_parser_cilk_clause_linear (parser, clauses); Index: gcc/omp-low.c =================================================================== --- gcc/omp-low.c (revision 205759) +++ gcc/omp-low.c (working copy) @@ -11300,7 +11300,7 @@ declare simd". */ bool cilk_clone = (flag_enable_cilkplus - && lookup_attribute ("cilk plus elemental", + && lookup_attribute ("cilk simd function", DECL_ATTRIBUTES (node->decl))); /* Allocate one more than needed just in case this is an in-branch Index: gcc/testsuite/gcc.dg/cilk-plus/cilk-plus.exp =================================================================== --- gcc/testsuite/gcc.dg/cilk-plus/cilk-plus.exp (revision 205759) +++ gcc/testsuite/gcc.dg/cilk-plus/cilk-plus.exp (working copy) @@ -59,6 +59,10 @@ dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O3 -flto -g -fcilkplus" " " } +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/SE/*.c]] " -g" " " +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/SE/*.c]] " -O3 -std=c99" " " +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/SE/*.c]] " -O3 -g" " " + dg-finish unset TEST_EXTRA_LIBS Index: gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error.c (revision 0) @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus -fopenmp" } */ + +#pragma omp declare simd linear(y:1) simdlen(4) +__attribute__((vector (linear (y:1), vectorlength(4)))) +int func (int x, int y) { /* { dg-error "cannot be used in the same function marked as a Cilk Plus SIMD-enabled" } */ + return (x+y); +} +__attribute__((vector (linear (y:1), private (x)))) /* { dg-error "expected clause" } */ +int func2 (int x, int y) { + return (x+y); +} + +__attribute__((vector (linear (y:1), simdlen (4)))) /* { dg-error "expected clause" } */ +int func2_1 (int x, int y) { + return (x+y); +} + +__attribute__((vector (linear (y:1), inbranch))) /* { dg-error "expected clause" } */ +int func2_3 (int x, int y) { + return (x+y); +} + +__attribute__((vector (notinbranch, vectorlength (4)))) /* { dg-error "expected clause" } */ +int func2_2 (int x, int y) { + return (x+y); +} + +int main (void) +{ + return (func (5,6)); +} Index: gcc/testsuite/c-c++-common/cilk-plus/SE/vlength_errors.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/vlength_errors.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/vlength_errors.c (revision 0) @@ -0,0 +1,56 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus -Wunknown-pragmas" } */ + +#define Q 4 + +int z = Q; + +__attribute__ ((vector (uniform(x), vectorlength (), linear (y:1) ))) /* { dg-error "expected expression" } */ +int func2 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), linear (y:1), vectorlength (4.5) ))) /* { dg-error "vectorlength must be an integer" } */ +int func3 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), linear (y:1), vectorlength (z) ))) /* { dg-error "vectorlength must be an integer" } */ +int func4 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), linear (y:1), vectorlength (Q) ))) /* This is OK! */ +int func5 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), vectorlength (z), linear (y:1)))) /* { dg-error "vectorlength must be an integer" } */ +int func6 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), linear (y:1), vectorlength (sizeof (int)) ))) /* This is OK too! */ +int func7 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +int main (void) +{ + int ii = 0, q = 5; + for (ii = 0; ii < 10; ii++) + q += func2 (z, ii); + return q; +} Index: gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error2.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error2.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error2.c (revision 0) @@ -0,0 +1,14 @@ +/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-options "-fcilkplus -Wall" } */ + +__attribute__((vector (vectorlength(32)))) +//#pragma omp simd simdlen (32) +int func2 (int x, int y) /* { dg-warning "unsupported simdlen" } */ +{ + return (x+y); +} + +int main (void) +{ + return (func2 (5,6)); +} Index: gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error3.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error3.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/ef_error3.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-options "-fcilkplus -Wall" } */ + +__attribute__((vector (linear (x:y)))) /* { dg-error "is not supported in this release" } */ +int func2 (int x, int y) +{ + return (x+y); +} + +int main (void) +{ + return (func2 (5,6)); +} Index: gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test.c (revision 0) @@ -0,0 +1,78 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus -Wunknown-pragmas" } */ + +/* Tests the clauses in several combinations put in different locations. */ +/* This is mostly a parser test. */ +#define Q 4 + +int z = Q; + + __attribute__ ((vector (uniform(x), linear (y:1), vectorlength (4) ))) +int func (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + __attribute__ ((__vector__ (uniform(x), vectorlength (2), linear (y:1) ))) +int func2 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(y), linear (x), vectorlength (4) ))) +int func3 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((__vector (uniform(x), linear (y:1), mask))) +int func4 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), linear (y:1), nomask))) +int func5 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform(x), mask, linear (y:1)))) +int func6 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform (x), mask, linear (y:1)), vector)) +int func7 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector (uniform (x), mask, linear (y:1)), vector (uniform (y), mask))) +int func8 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +__attribute__ ((vector, vector (uniform (y), mask))) +int func9 (int x, int y) +{ + int zq = 5; + return x + (y*zq); +} + +int main (int argc, char *argv[]) +{ + int ii = 0, q = 5; + for (ii = 0; ii < 10; ii++) + q += func (argc, ii); + return q; +} Index: gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test2.c =================================================================== --- gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test2.c (revision 0) +++ gcc/testsuite/c-c++-common/cilk-plus/SE/ef_test2.c (revision 0) @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ +void func (int x, int y) __attribute__((vector(linear(x:1), uniform (y)), + vector)); + +int q; +int main (void) +{ + int ii = 0; + q = 5; + for (ii = 0; ii < 100; ii++) + func (ii, q); + + return 0; +} +
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