OpenACC fortran front end

On 11/13/2014 08:43 AM, Jakub Jelinek wrote:

> On Thu, Nov 06, 2014 at 02:25:52PM -0800, Cesar Philippidis wrote:
>> 	* cpp.c (cpp_define_builtins): Conditionally define _OPENACC.
>> 	* dump-parse-tree.c
>> 	(show_omp_node): Dump also OpenACC executable statements.
> 
> Put (show_omp_node): ... and what fits on the same line as *
> dump-parse-tree.c, don't wrap prematurely.
> 
>> +#undef DEF_GOACC_BUILTIN_COMPILER
>> +      /* TODO: this is not doing the right thing.  */
> 
> Can you please avoid the TODOs in the source?  If it is not the right
> thing, either do something better, or file a PR to schedule such work for
> the future.
> 
>> +  struct gfc_expr *gang_expr;
>> +  struct gfc_expr *worker_expr;
>> +  struct gfc_expr *vector_expr;
>> +  struct gfc_expr *num_gangs_expr;
>> +  struct gfc_expr *num_workers_expr;
>> +  struct gfc_expr *vector_length_expr;
>> +  gfc_expr_list *wait_list;
>> +  gfc_expr_list *tile_list;
>> +  unsigned async:1, gang:1, worker:1, vector:1, seq:1, independent:1;
>> +  unsigned wait:1, par_auto:1, gang_static:1;
>> +
>> +  /* Directive specific data.  */
>> +  union
>> +  {
>> +    /* !$ACC DECLARE locus.  */
>> +    locus loc;
>> +  }
>> +  ext;
> 
> Perhaps turn it into a union only when you have more than one field?
> And if we start to use unions, I bet we should do something with having
> such huge struct with most of the empty pointers anyway, add some hierarchy
> to those.
> 
>> --- a/gcc/fortran/match.c
>> +++ b/gcc/fortran/match.c
>> @@ -2491,7 +2491,7 @@ match_exit_cycle (gfc_statement st, gfc_exec_op op)
>>  
>>    if (o != NULL)
>>      {
>> -      gfc_error ("%s statement at %C leaving OpenMP structured block",
>> +      gfc_error ("%s statement at %C leaving OpenMP or OpenACC structured block",
>>  		 gfc_ascii_statement (st));
>>        return MATCH_ERROR;
>>      }
> 
> I think it would be better to specify which (ie. compute
> is_oacc and have two different format strings, is_oacc ? " ... " : " ... ").
> 
>> -/* Match OpenMP directive clauses. MASK is a bitmask of
>> +/* OpenACC 2.0 clauses. */
>> +#define OMP_CLAUSE_ASYNC		(1ULL << 32)
> 
> As C++98 doesn't mandate long long type, I'm afraid we shouldn't use
> 1ULL; we require some 64-bit type though, so perhaps just use
> ((uint64_t) 1 << 32) etc.?
> 
>> +#define OMP_CLAUSE_NUM_GANGS		(1ULL << 33)
>> +#define OMP_CLAUSE_NUM_WORKERS		(1ULL << 34)
>> +#define OMP_CLAUSE_VECTOR_LENGTH	(1ULL << 35)
>> +#define OMP_CLAUSE_COPY			(1ULL << 36)
>> +#define OMP_CLAUSE_COPYOUT		(1ULL << 37)
>> +#define OMP_CLAUSE_CREATE		(1ULL << 38)
>> +#define OMP_CLAUSE_PRESENT		(1ULL << 39)
>> +#define OMP_CLAUSE_PRESENT_OR_COPY	(1ULL << 40)
>> +#define OMP_CLAUSE_PRESENT_OR_COPYIN	(1ULL << 41)
>> +#define OMP_CLAUSE_PRESENT_OR_COPYOUT	(1ULL << 42)
>> +#define OMP_CLAUSE_PRESENT_OR_CREATE	(1ULL << 43)
>> +#define OMP_CLAUSE_DEVICEPTR		(1ULL << 44)
>> +#define OMP_CLAUSE_GANG			(1ULL << 45)
>> +#define OMP_CLAUSE_WORKER		(1ULL << 46)
>> +#define OMP_CLAUSE_VECTOR		(1ULL << 47)
>> +#define OMP_CLAUSE_SEQ			(1ULL << 48)
>> +#define OMP_CLAUSE_INDEPENDENT		(1ULL << 49)
>> +#define OMP_CLAUSE_USE_DEVICE		(1ULL << 50)
>> +#define OMP_CLAUSE_DEVICE_RESIDENT	(1ULL << 51)
>> +#define OMP_CLAUSE_HOST_SELF		(1ULL << 52)
>> +#define OMP_CLAUSE_OACC_DEVICE		(1ULL << 53)
>> +#define OMP_CLAUSE_WAIT			(1ULL << 54)
>> +#define OMP_CLAUSE_DELETE		(1ULL << 55)
>> +#define OMP_CLAUSE_AUTO			(1ULL << 56)
>> +#define OMP_CLAUSE_TILE			(1ULL << 57)
> 
>>  static match
>> -gfc_match_omp_clauses (gfc_omp_clauses **cp, unsigned int mask,
>> -		       bool first = true, bool needs_space = true)
>> +gfc_match_omp_clauses (gfc_omp_clauses **cp, unsigned long long mask,
>> +		       bool first = true, bool needs_space = true,
> 
> See above, use uint64_t.
> 
>> +		c->async_expr = gfc_get_constant_expr (BT_INTEGER,
>> +						       gfc_default_integer_kind,
>> +						      &gfc_current_locus);
>> +		/* TODO XXX: FIX -1 (acc_async_noval).  */
> 
> Please use gomp-constants.h for this, or some other enum, and avoid
> TODO XXX comments in the source.

Thomas told me that he fix this when he updates gomp-constants.h.

>> +static gfc_statement
>> +omp_code_to_statement (gfc_code *code)
>> +{
>> +switch (code->op)
>> +  {
>> +  case EXEC_OMP_PARALLEL:
>> +    return ST_OMP_PARALLEL;
> 
> Wrong formatting.  switch should be indented by 2 spaces, { after it by 4,
> case by 4 too and return by 6 spaces.
> 
>> +  default:
>> +    gcc_unreachable ();
>> +  }
> ...
> 
>> +oacc_code_to_statement (gfc_code *code)
>> +{
>> +switch (code->op)
>> +  {
> 
> Likewise here.
> 
>> +static void
>> +resolve_oacc_loop(gfc_code *code)
> 
> Formatting - missing space before (, please check it everywhere.
> 
>> +static void
>> +resolve_oacc_cache (gfc_code *code)
>> +{
>> +  gfc_error ("Sorry, !$ACC cache unimplemented yet at %L", &code->loc);
> 
> Shouldn't this be sorry ("...") instead?

That introduced an ICE. I've created a PR for it here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63861

>> +}
>> +
>> +
>> +void
>> +gfc_resolve_oacc_declare (gfc_namespace *ns)
>> +{
>> +  int list;
>> +  gfc_omp_namelist *n;
>> +  locus loc;
>> +  static const char *clause_names[] = {"COPY", "COPYIN", "COPYOUT", "CREATE",
> 
> Missing space after { ?
> 
>> +	"DELETE", "PRESENT", "PRESENT_OR_COPY", "PRESENT_OR_COPYIN",
>> +	"PRESENT_OR_COPYOUT", "PRESENT_OR_CREATE", "DEVICEPTR",
>> +	"DEVICE_RESIDENT"};
> 
> and before }.
> 
>> +  /* FIXME: handle omp_list_map.  */
> 
> Please avoid fixmes, either fix it, or file a PR.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63859

>> @@ -3568,9 +3813,13 @@ static void
>>  parse_critical_block (void)
>>  {
>>    gfc_code *top, *d;
>> -  gfc_state_data s;
>> +  gfc_state_data s, *sd;
>>    gfc_statement st;
>>  
>> +  for (sd = gfc_state_stack; sd; sd = sd->previous) 
>> +    if (sd->state == COMP_OMP_STRUCTURED_BLOCK)
>> +      gfc_error_now ("CRITICAL block inside of OpenMP or OpenACC region at %C");
> 
> Couldn't you determine which one and make the diagnostics explicit?
> 
>> --- a/gcc/fortran/scanner.c
>> +++ b/gcc/fortran/scanner.c
>> @@ -55,9 +55,11 @@ gfc_directorylist *include_dirs, *intrinsic_modules_dirs;
>>  
>>  static gfc_file *file_head, *current_file;
>>  
>> -static int continue_flag, end_flag, openmp_flag, gcc_attribute_flag;
>> +static int continue_flag, end_flag, gcc_attribute_flag;
>> +static int openmp_flag, openacc_flag; /* If !$omp/!$acc occurred in current comment line */
> 
> Too long line.  Best put it above the two flags.
> End the comment with ".  */" (dot and one space missing).
> 
>> +	      gcc_assert(gfc_wide_tolower (c) == (unsigned char) "!$acc"[i]);
> 
> Formatting, missing space before (.
> 
>>        /* See if this line is a continuation line.  */
>> -      if (openmp_flag != prev_openmp_flag)
>> -	{
>> -	  openmp_flag = prev_openmp_flag;
>> -	  goto not_continuation;
>> -	}
>> +      if (gfc_option.gfc_flag_openmp)
>> +	if (openmp_flag != prev_openmp_flag)
>> +	  {
>> +	    openmp_flag = prev_openmp_flag;
>> +	    goto not_continuation;
>> +	  }
>> +      if (gfc_option.gfc_flag_openacc)
>> +	if (openacc_flag != prev_openacc_flag)
>> +	  {
>> +	    openacc_flag = prev_openacc_flag;
>> +	    goto not_continuation;
>> +	  }
> 
> Why the guards by gfc_flag_open* ?  What happens
> if both -fopenmp -fopenacc is enabled?
> And, if it is right this way, the nested ifs are weird,
> use if (gfc_option.gfc_flag_openmp && openmp_flag != prev_openmp_flag)
> instead.

Using -fopenmp with -fopenacc is broken. See

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63858

>> +      if (clauses->async_expr)
>> +	OMP_CLAUSE_ASYNC_EXPR (c) =
>> +	    gfc_convert_expr_to_tree (block, clauses->async_expr);
> 
> = should be on the next line.
> 
>> +      tree num_gangs_var = 
>> +	  gfc_convert_expr_to_tree (block, clauses->num_gangs_expr);
> 
> Likewise and several times more.
> 
> LGTM otherwise.

Thanks. I couldn't figure out how to assign the bugs in the PR. Maybe my
account doesn't have permission to do so. Regardless, I'll work on them.

I've applied the attached patch to gomp-4_0-branch. It should address
all of your requests. I'll regenerate a complete patch later. Thomas
told me that he would merge the fortran FE along with the middle end
when the time comes.

This patch also includes a change to a gomp test. That change is
specific to gomp-4_0-branch, so if it looks out of place just ignore it.

Cesar

OpenACC fortran front end

Commit Message

Comments

Patch