Re: [PATCH 2/6] [GOMP4] OpenACC 1.0+ support in fortran front-end

[Tobias Burnus <burnus at net-b dot de>]

Ilmir Usmanov wrote:
>     OpenACC 1.0 fortran FE support -- matching and resolving.

> +  return MATCH_ERROR;
> +}
> +
> +static match
> +match_oacc_clause_gang (gfc_omp_clauses *cp)
> +{

For consistency, can you  add another empty line before the function?

> +#define OMP_CLAUSE_SEQ                  (1ll << 32)

I think you should use 1LL instead of 1ll - that should be more readable 
and matches gcc/{configure,fold-const.c,glimits.h,simplify-rtx.c}

> +static void
> +resolve_oacc_scalar_int_expr (gfc_expr *expr, const char *clause)
> +{
> +  if (!gfc_resolve_expr (expr)
> +      || expr->ts.type != BT_INTEGER || expr->rank != 0)
> +    gfc_error ("%s clause at %L requires a scalar INTEGER expression",
> +                     clause, &expr->where);
> +}

I'd use "integer" instead of "INTEGER" as it is not a 'reserved' word

> +    gfc_warning ("INTEGER expression of %s clause at %L must be positive",
> +                     clause, &expr->where);
> +}

Ditto.

> @@ -800,10 +1366,14 @@ resolve_omp_clauses (gfc_code *code)
>     static const char *clause_names[]
>       = { "PRIVATE", "FIRSTPRIVATE", "LASTPRIVATE", "COPYPRIVATE", "SHARED",
> -	"COPYIN", "REDUCTION" };
> +	"COPYIN", "COPY", "COPYIN", "COPYOUT", "CREATE", "DELETE",
> +        "PRESENT", "PRESENT_OR_COPY", "PRESENT_OR_COPYIN", "PRESENT_OR_COPYOUT",
> +        "PRESENT_OR_CREATE", "DEVICEPTR", "USE_DEVICE", "DEVICE_RESIDENT",
> +        "HOST", "DEVICE", "CACHE", "REDUCTION"};

Indention: Should be a tab not spaces.

> @@ -933,8 +1503,43 @@ resolve_omp_clauses (gfc_code *code)
>   	else
>   	  gcc_unreachable ();
>   
> +  if (list >= OMP_LIST_DATA_CLAUSE_FIRST
> +      && list <= OMP_LIST_DATA_CLAUSE_LAST)
> +    {
> +      if (n->sym->ts.type == BT_DERIVED
> +          && n->sym->attr.allocatable)
> +        gfc_error ("ALLOCATABLE object '%s' of DERIVED type in %s clause at %L",
> +                   n->sym->name, name, &code->loc);
> +      if (n->sym->ts.type == BT_DERIVED
> +          && n->sym->attr.pointer)
> +        gfc_error ("POINTER object '%s' of DERIVED type in %s clause at %L",
> +                   n->sym->name, name, &code->loc);

I'd use "derived type" for the same reason as above.

Shouldn't you also reject polymorphic types ("BT_CLASS" and 
"BT_ASSUMED")? [BT_CLASS = "class(derived_type_name)" or "class(*)"; 
BT_ASSUMED = "type(*)"]

> +      if (n->sym->attr.pointer)
> +        gfc_error ("POINTER object '%s' in %s clause at %L",
> +                   n->sym->name, name, &code->loc);

Actually, here and probably elsewhere: Do you need to take care of 
derived-type components? I mean something like
   clause(derived_type%comp)

If so, note that symtree->n.sym->attr.pointer only checks for 
"derived_type" - even if you are interested in "comp"'s attributes. You 
probably should use  gfc_expr_attr() in this and similar cases.


> +      if (n->sym->as && n->sym->as->type == AS_ASSUMED_SIZE)
> +        gfc_error ("Assumed size array '%s' in %s clause at %L",
> +                   n->sym->name, name, &code->loc);
> +    }

Do you also need to reject AS_ASSUMED_RANK (new since Fortran Technical 
Specification ISO/IEC TS 29113:2012)? I mean code like:

   subroutine foo(x)
     integer, DIMENSION(..) :: x
     ... openacc_clause(x)

Side note: One should also check how OpenMP handles those.

> +	  case OMP_LIST_DEVICEPTR:
> +	    if (n->sym->attr.pointer)
> +	      gfc_error ("POINTER object '%s' in %s clause at %L",
> +	                 n->sym->name, name, &code->loc);

Talking about pointers, you probably also want to reject Cray pointers 
(attr.cray_pointee - and cray_pointee). [One should also check what 
OpenMP does; I think it does handle it correctly.]

And another point: I think you have to check whether the argument is a 
named constant (PARAMETER, attr.flavor == FL_PARAMETER), I think those 
you cannot put them there either.

What happens if you try to use a literal such as "deviceptr(5)"?


> +}
> +
> +static void
> +resolve_oacc_params_in_parallel (gfc_code *code, const char *clause)
> +{

Another empty line for consistency.

> +  if (oacc_is_parallel (code))
> +    gfc_error ("LOOP %s in PARALLEL section allows no argument or static at %L",
> +               clause, &code->loc);

I am not sure whether I understand the error - in particular what 
"static" means. (It might be obvious after looking at the spec - if not, 
the error message should be improved.) In any case, I think you want to 
use "nor" instead of "or".

> +  if (code->ext.omp_clauses->seq)
> +    {
> +      if (code->ext.omp_clauses->independent)
> +        gfc_error ("Both SEQ and INDEPENDENT are not allowed in %L", &code->loc);

Somehow, I don't like the wording; it sounds to me a bit as if SEQ is 
not permitted and INDEPENDENT is not permitted while you mean that only 
using them simultaneously is wrong. I don't have a very good suggestion, 
but here are some: "SEQ clause conflicts with INDEPENDENT at %L", "SEQ 
shall not be used together with INDEPENDENT at %L" or ...

> +          resolve_oacc_positive_int_expr (el->expr, "TILE");
> +          if (el->expr->expr_type != EXPR_CONSTANT)
> +            gfc_error ("TILE requires constant expression at %L", &code->loc);

Is the following permitted: "tile(4+5)"? If so, do you need to call 
gfc_simplify_expr() as some point? (Or is this already done at some point?)


> +static void
> +resolve_oacc_cache (gfc_code *)
> +{
> +	//TODO: resolve subarrays
> +}

;-)


Tobias