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.
> +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?
> +}
> +
> +
> +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.
> @@ -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.
> + 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.
Jakub
