Re: PING: Fwd: Re: [patch] implement Cilk Plus simd loops on trunk

[Aldy Hernandez <aldyh at redhat dot com>]

On 11/14/13 10:05, Jakub Jelinek wrote:

[Balaji, see below for question.]

> On Thu, Nov 14, 2013 at 09:49:41AM -0700, Aldy Hernandez wrote:
> > +    case OMP_PARALLEL:
> > +    case OMP_TASK:
> > +    case OMP_FOR:
> > +    case OMP_SIMD:
> > +    case OMP_SECTIONS:
> > +    case OMP_SINGLE:
> > +    case OMP_SECTION:
> > +    case OMP_MASTER:
> > +    case OMP_ORDERED:
> > +    case OMP_CRITICAL:
> > +    case OMP_ATOMIC:
> > +    case OMP_ATOMIC_READ:
> > +    case OMP_ATOMIC_CAPTURE_OLD:
> > +    case OMP_ATOMIC_CAPTURE_NEW:
>
> This is only a subset of OpenMP statements.
> You are missing OMP_DISTRIBUTE, OMP_TARGET, OMP_TARGET_DATA, OMP_TEAMS,
> OMP_TARGET_UPDATE, OMP_TASKGROUP.
> Also, CALL_EXPRs to
>            case BUILT_IN_GOMP_BARRIER:
>            case BUILT_IN_GOMP_CANCEL:
>            case BUILT_IN_GOMP_CANCELLATION_POINT:
>            case BUILT_IN_GOMP_TASKYIELD:
>            case BUILT_IN_GOMP_TASKWAIT:
> are OpenMP statements.  For OpenMP we diagnose this later on, in
> check_omp_nesting_restrictions in omp-low.c, wouldn't it be better to
> handle it there too?

Woah, indeed.  I removed all of this section in favor of the error in 
check_omp_nesting_restriction, and adjusted the testcase error accordingly.

> > +      error_at (EXPR_LOCATION (*tp), "OpenMP statements are not allowed "
> > +		"within loops annotated with #pragma simd");
> > +      *valid = false;
> > +      *walk_subtrees = 0;
> > +      break;
> > --- a/gcc/c-family/c-pragma.c
> > +++ b/gcc/c-family/c-pragma.c
> > @@ -1380,6 +1380,12 @@ init_pragma (void)
> >   				      omp_pragmas_simd[i].id, true, true);
> >       }
> >
> > +  if (flag_enable_cilkplus && !flag_preprocess_only)
> > +    {
> > +      cpp_register_deferred_pragma (parse_in, NULL, "simd",
> > +				    PRAGMA_CILK_SIMD, true, false);
> > +    }
>
> Unnecessary {}s (or do you expect further cilk+ pragmas?

I sincerely hope not :).  Fixed.

> > @@ -11543,6 +11553,9 @@ c_parser_omp_for_loop (location_t loc, c_parser *parser, enum tree_code code,
> >   	    case LT_EXPR:
> >   	    case LE_EXPR:
> >   	      break;
> > +	    case NE_EXPR:
> > +	      if (code == CILK_SIMD)
> > +		break;
>
> Add /* FALLTHRU */ here?

Done.

> >   	    default:
> >   	      /* Can't be cond = error_mark_node, because we want to preserve
> >   		 the location until c_finish_omp_for.  */
>
> > +      else if (c_kind == PRAGMA_CILK_CLAUSE_REDUCTION)
> > +	/* Use the OMP 4.0 equivalent function.  */
> > +	clauses = cp_parser_omp_clause_reduction (parser, clauses);
>
> I'm surprised here, does the Cilk+ reduction clause really want to grok
> OpenMP user defined reductions etc.?

Hmm, I doubt it.  Balaji, OpenMP user defined reductions are not allowed 
for Cilk Plus, right?

If so, Jakub what do you suggest, disallowing parsing of user defined 
reductions in cp_parser_omp_clause_reduction() when some Cilk Plus flag, 
or did you have something else in mind?

How does this look (fixed all the above with the exception of 
user-defined reductions, as well as Joseph's suggestion)?

Aldy

Attachment: curr
Description: Text document