Hi Sebastian! Congratulations for submitting this first patch! :-)
You've produced a very good set of changes here, especially given that you don't really have much prior experience in the GCC/Fortran front end and libgomp, and without much guidance yet. With some proper guidance (as per this email -- I hope), we shall quickly get this into shape so that it can be applied to GCC trunk! Please don't worry if this is taking a bit longer than you estimated. Let's add a few new OpenACC features properly, instead of rushing through many, but only get buggy/half-done implementations. This first review turned out a bit lengthy. That's not unexpected, no worries. I'm also providing some background information that I assume may be helpful, for reference. I've ocassionally also left included some "work in progress" thinking on my side, in case that may be hepful for understanding my trains of thoughts. I've put "[high priority]" tags to the items that we should address right away. Those are the ones that address test suite FAILs, avoid code churn, or relate to ABI definitions/stability. All other (untagged) items can be done incrementally, after the big patch has gone in. (It's of course fine to also address non-"[high priority]" items right away, if you'd like.) I expect to be able to reply much faster to incremental updates on top of what you've sent here and what I've now reviewed. Also, your work has turned up a few pre-existing quirky items in the current implementation, where I've made a note to review them later. (Not all reflected in this email here.) Emails related to GCC/Fortran parts should also CC <[email protected]>. (Added.) I locally 'git am'ed your email, and it applied fine, but there's some issues with the ChangeLog (as part of the Git commit log): $ contrib/gcc-changelog/git_check_commit.py --print-changelog HEAD Checking [...]: FAILED ERR: line should start with a tab: "list" ERR: line should start with a tab: "device_type list" [...] ERR: line should start with a tab: "translation" ERR: extra space after tab: " directive" [...] [high priority] Please check whether it's fine for you locally -- that is, whether it's just a email/patch submission issue. In that case, maybe submit the changes as an attachment, instead of inline email. But I assume that's not the problem, as your email has: X-Mailer: git-send-email 2.54.0 See <https://gcc.gnu.org/codingconventions.html#ChangeLogs> for how to fix up the ChangeLog; but let me know before wasting too much time on this. Your editor (I suppose) has removed some stray whitepace at end of line etc., in regions unrelated to those you're actually modifying. I personally wouldn't do that (don't have my editor set up for such whitespace etc. clean-up), but I also won't object to it in patch review, as long as such changes aren't excessive. > Subject: [PATCH] Add Fortran front-end support for OpenACC init, set and > shutdown directives. [high priority] I understand, of coursem what you mean with "Fortran front-end support for [...]", but you're actually not only implementing the Fortran front end parts, but (as you also write in the following text) you're actually implementing the whole flow, including new libgomp entry points. So you may boost that up to: "Fortran support for [...]". ;-) It's totally fine that you've done Fortran first (or any one language) and have been holding off the corresponding C, C++ changes, in particular until we've clarified the GCC/libgomp interface ('GOACC_[...]' functions). Regarding the latter, you've proposed: GOACC_init (int d, int n_device) GOACC_shutdown (int d, int n_device) Those two interfaces seem fine; I don't currently see any possibility of having to support other clauses than 'device_type', 'device_num' for 'init', 'shutdown'. (The type of 'd' should actually be 'acc_device_t', but we currently can't easily express that, so 'int' is fine -- just like is used for 'BUILT_IN_ACC_ON_DEVICE' ('acc_on_device'), for example.) We need to support the case of 'device_type' and/or 'device_num' clause not specified, but we can use suitable negative values for 'd' and/or 'n_device' (as you've implemented). Regarding: GOACC_set (int d, int n_device) ..., I'm less clear about this one. Here, we'll eventually need to support 'default_async' (and potentially further ICVs, later on). So that might argue in favor of having a versatile/extensible "varargs" API (like 'GOACC_parallel_keyed', for example), or a set of individual 'GOACC_set_[clause]' entry points. However, certainly 'device_type', 'device_num' are closely related. That's also the case in the relevant text in the OpenACC specification, where the behavior of 'device_type', 'device_num' clauses is specified with interdependencies. Also, those clauses often appear together on one '!$acc init' directive (unless only of them appears, of course), and in such cases it would be "wasteful" to call into libgomp twice: first for 'GOACC_set_device_type', directly followed by 'GOACC_set_device_num'. [high priority] I suggest, to make the intent clear, that we rename 'GOACC_set' to 'GOACC_set_device_type_device_num' (with the API as you've proposed). Later, we can then add orthogonal interfaces such as 'GOACC_set_default_async', and whatever else we might need. [high priority] Ah, one point actually has to be made, in context of the corresponding 'acc_set_device_num', 'acc_init_device', 'acc_shutdown_device' functions: void acc_set_device_num(int dev_num, acc_device_t dev_type); void acc_init_device(int dev_num, acc_device_t dev_type); void acc_shutdown_device(int dev_num, acc_device_t dev_type); These specify 'dev_num' first, and then 'dev_type'. For consistency, let's please do the same here: GOACC_init (int dev_num, int dev_type) GOACC_shutdown (int dev_num, int dev_type) GOACC_set_dev[ice]_num_dev[ice]_type (int dev_num, int dev_type) (I've corresponsingly changed my proposed 'GOACC_set_device_type_device_num' into 'GOACC_set_dev[ice]_num_dev[ice]_type'. You get thos choose whether it should be the shorter 'dev' variant, or the full 'device' variant.) On 2026-06-16T16:19:19-0400, Sebastian Galindo <[email protected]> wrote: > This patch adds parsing and libgomp runtime calls for these directives, > sharing the existing implementation with runtime API calls. It also > includes support for the device_type and device_num clauses in the > Fortran front-end. > > For the device_type clause, the current implementation limits usage to > accepting only a single clause per directive. While the OpenACC > specification states that device_type should modify the device for > subsequent clauses inside other directives or constructs, supporting > this behavior requires a larger refactoring and may be addressed in > the future. Right. (..., and isn't applicable to the 'init', 'shutdown', 'set' directives anyway.) I see in your GCC/Fortran front end changes that you're already paving the way for a 'device-type-list' in 'device_type' clauses. Again: not necessary here, but also not wrong, of course. > > The set directive does not yet support the default_async clause, as > further discussion is needed to agree on its semantics and libgomp > implementation. ACK. > > Signed-off-by: Sebastian Galindo <[email protected]> If I remember correctly, the "Signed-off-by" line should be at the end of the Git commit long (so that standard Git tooling understands it as a "trailer"). > [ChangeLog updates] ... to be revised as per above. In the following code review, I've snipped out the parts that I have no comments about. (That is, those are fime.) Please be aware that I'm also not familiar with all the details and conventions of the GCC/Fortran front end, so if you see me propose something that doesn't make sense to you (or anyone else reading this, of course), please question my remarks. > --- a/gcc/fortran/gfortran.h > +++ b/gcc/fortran/gfortran.h > @@ -1360,6 +1361,16 @@ gfc_expr_list; > > #define gfc_get_expr_list() XCNEW (gfc_expr_list) > > +/* Likewise to gfc_expr_list, but contains OpenACC device types */ > +typedef struct gfc_device_type_list > +{ > + int device; > + struct gfc_device_type_list* next; > +} > +gfc_device_type_list; > + > +#define gfc_get_device_type_list() XCNEW (gfc_device_type_list) > + > enum gfc_omp_reduction_op > { > OMP_REDUCTION_NONE = -1, > @@ -1760,8 +1771,10 @@ typedef struct gfc_omp_clauses > struct gfc_expr *num_gangs_expr; > struct gfc_expr *num_workers_expr; > struct gfc_expr *vector_length_expr; > + struct gfc_expr *device_num_expr; > gfc_expr_list *wait_list; > gfc_expr_list *tile_list; > + gfc_device_type_list *device_type_list; > unsigned async:1, gang:1, worker:1, vector:1, seq:1, independent:1; > unsigned par_auto:1, gang_static:1; > unsigned if_present:1, finalize:1; Curious, do we really need 'device_type_list' to be of (new) 'gfc_device_type_list' type, instead of using the generic 'gfc_expr_list'? Need to handle 'device_num_expr', 'device_type_list' in 'gcc/fortran/dump-parse-tree.cc:show_omp_clauses'. > @@ -3275,6 +3288,7 @@ enum gfc_exec_op > EXEC_OACC_DATA, EXEC_OACC_HOST_DATA, EXEC_OACC_LOOP, EXEC_OACC_UPDATE, > EXEC_OACC_WAIT, EXEC_OACC_CACHE, EXEC_OACC_ENTER_DATA, EXEC_OACC_EXIT_DATA, > EXEC_OACC_ATOMIC, EXEC_OACC_DECLARE, > + EXEC_OACC_INIT, EXEC_OACC_SHUTDOWN, EXEC_OACC_SET, > EXEC_OMP_CRITICAL, EXEC_OMP_FIRST_OPENMP_EXEC = EXEC_OMP_CRITICAL, > EXEC_OMP_DO, EXEC_OMP_FLUSH, EXEC_OMP_MASTER, > EXEC_OMP_ORDERED, EXEC_OMP_PARALLEL, EXEC_OMP_PARALLEL_DO, Need to handle 'EXEC_OACC_INIT', 'EXEC_OACC_SHUTDOWN', 'EXEC_OACC_SET' in 'gcc/fortran/dump-parse-tree.cc': 'show_omp_node' (twice), 'show_code_node'. (See 'EXEC_OACC_WAIT', for example.) > --- a/gcc/fortran/openmp.cc > +++ b/gcc/fortran/openmp.cc > @@ -213,12 +213,14 @@ gfc_free_omp_clauses (gfc_omp_clauses *c) > gfc_free_expr (c->num_gangs_expr); > gfc_free_expr (c->num_workers_expr); > gfc_free_expr (c->vector_length_expr); > + gfc_free_expr (c->device_num_expr); > for (enum gfc_omp_list_type t = OMP_LIST_FIRST; t < OMP_LIST_NUM; > t = gfc_omp_list_type (t + 1)) > gfc_free_omp_namelist (c->lists[t], t); > gfc_free_expr_list (c->wait_list); > gfc_free_expr_list (c->tile_list); > gfc_free_expr_list (c->sizes_list); > + gfc_free_device_type_list (c->device_type_list); > free (const_cast<char *> (c->critical_name)); > if (c->assume) > { Please move 'device_type_list' handling in the correct spot as per its position in 'gfc_omp_clauses'. > +static int > +match_oacc_device_type_kind (void) > +{ > + if (gfc_match (" nvidia") == MATCH_YES) > + return GOMP_DEVICE_NVIDIA_PTX; I would've thought we'd do something like: char name[GFC_MAX_SYMBOL_LEN + 1]; match m = gfc_match (" %n ", name); ..., and then do a case-insensitive string comparison (as per OpenACC specification "Recommendations for Implementers", "Target Devices"). (Reflect that aspect in test cases, too.) > + if (gfc_match (" radeon") == MATCH_YES) > + return GOMP_DEVICE_GCN; > + if (gfc_match (" host") == MATCH_YES) > + return GOMP_DEVICE_HOST; Suggest to handle these in the same order that 'GOMP_DEVICE_[...]' are defined. > + if (gfc_match (" *") == MATCH_YES) > + return GOMP_DEVICE_GCN + 1; // means all I think we can actually just use 'GOMP_DEVICE_NONE' ('acc_device_none'; '0') instead of 'GOMP_DEVICE_GCN + 1' (..., which looks fragile/hacky?), which corresponds to 'acc_device_radeon + 1': '_ACC_device_hwm' in the libgomp/OpenACC code, where your proposed changes of course need to be adjusted correspondingly. (There would never be any proper case where we'd need to handle user-specified 'acc_device_none', "no device type"?) > + return -1; > +} > + > +static match > +match_oacc_device_type_list (const char *str, gfc_device_type_list **list) > +{ > + gfc_device_type_list *head = NULL, *tail = NULL, *p = NULL; > + locus old_loc = gfc_current_locus; > + > + if (gfc_match (str) != MATCH_YES) > + return MATCH_NO; In my understanding, this handling of 'str' here (passed in as ' (') is only necessary if a 'MATCH_NO' result needs to be handled differently from 'MATCH_ERROR'? This isn't necessary for the 'device_type' clause, where an opening parens always must follow. So, you should be able to just call 'gfc_match_dupl_check' with 'open_parens' argument set to 'true' instead of 'false', and remove the 'str' handling here. > + > + int device_n = 0, device_exist = 0, asterisk_bit = GOMP_DEVICE_GCN + 1; > + > + for (;;) > + { > + int device = match_oacc_device_type_kind (); > + if (device < 0) > + { > + gfc_error ("Unknown device at %C, expecting one of the following: " > + "nvidia, radeon, host, *"); > + goto cleanup; > + } > + > + if (device_exist & (1 << (device))) > + { > + gfc_error ("Duplicate device_type parameter at %C"); > + goto cleanup; > + } > + > + device_exist |= (1 << device); > + device_n++; > + if ((device_exist & (1 << asterisk_bit)) && device_n > 1) > + { > + gfc_error ( > + "Asterisk must be the unique parameter in device_type at %C"); > + goto cleanup; > + } > + p = gfc_get_device_type_list (); > + p->device = device; > + > + if (head == NULL) > + { > + head = tail = p; > + } > + else > + { > + tail->next = p; > + tail = tail->next; > + } > + > + if (gfc_match_char (')') == MATCH_YES) > + break; > + if (gfc_match_char (',') != MATCH_YES) > + goto sintax; > + } > + > + while (*list) > + list = &(*list)->next; > + > + *list = head; > + > + return MATCH_YES; > + > +sintax: > + gfc_error ("Syntax error in OpenACC device_type list at %C"); > +cleanup: > + gfc_free_device_type_list (head); > + gfc_current_locus = old_loc; > + return MATCH_ERROR; > +} Again, I concur that we'll eventually need something like this proposed 'device-type-list' parsing -- but don't need it yet for purposes of the 'init', 'shutdown', 'set' directives, which support only a single argument in the 'device_type' clause. From that follows that we currently don't have any test coverage for this non-trivial code, and I therefore suggest to back out these changes for now, and re-introduce them only once we actually need and can properly test this code (for "the other" 'device_type' clause usage). Works for you? (I've therefore not yet reviewed this function in more detail.) [high priority] In other words: for the time being, parse only a single argument in the 'device_type' clause, and error out if more than one. > @@ -1168,22 +1265,23 @@ enum omp_mask2 > OMP_CLAUSE_ATTACH, > OMP_CLAUSE_NOHOST, > OMP_CLAUSE_HAS_DEVICE_ADDR, /* OpenMP 5.1 */ > - OMP_CLAUSE_ENTER, /* OpenMP 5.2 */ > - OMP_CLAUSE_DOACROSS, /* OpenMP 5.2 */ > - OMP_CLAUSE_ASSUMPTIONS, /* OpenMP 5.1. */ > - OMP_CLAUSE_USES_ALLOCATORS, /* OpenMP 5.0 */ > - OMP_CLAUSE_INDIRECT, /* OpenMP 5.1 */ > - OMP_CLAUSE_FULL, /* OpenMP 5.1. */ > - OMP_CLAUSE_PARTIAL, /* OpenMP 5.1. */ > - OMP_CLAUSE_SIZES, /* OpenMP 5.1. */ > - OMP_CLAUSE_INIT, /* OpenMP 5.1. */ > - OMP_CLAUSE_DESTROY, /* OpenMP 5.1. */ > - OMP_CLAUSE_USE, /* OpenMP 5.1. */ > - OMP_CLAUSE_NOVARIANTS, /* OpenMP 5.1 */ > - OMP_CLAUSE_NOCONTEXT, /* OpenMP 5.1 */ > - OMP_CLAUSE_INTEROP, /* OpenMP 5.1 */ > - OMP_CLAUSE_LOCAL, /* OpenMP 6.0 */ > + OMP_CLAUSE_ENTER, /* OpenMP 5.2 */ > + OMP_CLAUSE_DOACROSS, /* OpenMP 5.2 */ > + OMP_CLAUSE_ASSUMPTIONS, /* OpenMP 5.1. */ > + OMP_CLAUSE_USES_ALLOCATORS, /* OpenMP 5.0 */ > + OMP_CLAUSE_INDIRECT, /* OpenMP 5.1 */ > + OMP_CLAUSE_FULL, /* OpenMP 5.1. */ > + OMP_CLAUSE_PARTIAL, /* OpenMP 5.1. */ > + OMP_CLAUSE_SIZES, /* OpenMP 5.1. */ > + OMP_CLAUSE_INIT, /* OpenMP 5.1. */ > + OMP_CLAUSE_DESTROY, /* OpenMP 5.1. */ > + OMP_CLAUSE_USE, /* OpenMP 5.1. */ > + OMP_CLAUSE_NOVARIANTS, /* OpenMP 5.1 */ > + OMP_CLAUSE_NOCONTEXT, /* OpenMP 5.1 */ > + OMP_CLAUSE_INTEROP, /* OpenMP 5.1 */ > + OMP_CLAUSE_LOCAL, /* OpenMP 6.0 */ > OMP_CLAUSE_DYN_GROUPPRIVATE, /* OpenMP 6.1 */ [high priority] Please undo these indentation changes: it's difficult to see what actually changed. (Nothing?) > + OMP_CLAUSE_DEVICE_NUM, /* OpenACC 3.4 */ > /* This must come last. */ > OMP_MASK2_LAST > }; [high priority] The OpenACC 'device_num' clause was introduced in OpenACC 2.5 not 3.4, together with the 'init', 'shutdown', 'set' directives. But I don't see the need for mentioning that here; just add 'OMP_CLAUSE_DEVICE_NUM' without comment. > @@ -3158,9 +3256,21 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const > omp_mask mask, > OMP_MAP_FORCE_DEVICEPTR, false, > allow_derived)) > continue; > - if ((mask & OMP_CLAUSE_DEVICE_TYPE) > + > + if ((mask & OMP_CLAUSE_DEVICE_TYPE) && openacc > + && gfc_match_dupl_check (!c->device_type_list, "device_type", > + false) > + == MATCH_YES > + && match_oacc_device_type_list (" (", &c->device_type_list) > + == MATCH_YES) > + { > + continue; > + } Yes, something like this, but see the 'match_oacc_device_type_list' comments above. > + > + if ((mask & OMP_CLAUSE_DEVICE_TYPE) && !openacc ACK. > && gfc_match_dupl_check (c->device_type == OMP_DEVICE_TYPE_UNSET, > - "device_type", true) == MATCH_YES) > + "device_type", true) > + == MATCH_YES) [high priority] Please undo that spurious change. > @@ -3281,6 +3391,17 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const > omp_mask mask, > goto error; > continue; > } > + > + if ((mask & OMP_CLAUSE_DEVICE_NUM) && openacc > + && (m = gfc_match_dupl_check (!c->device_num_expr, "device_num")) > + != MATCH_NO) > + { > + if (m == MATCH_ERROR) > + goto error; > + if (gfc_match ("( %e )", &c->device_num_expr) != MATCH_YES) > + goto error; > + continue; > + } > break; > case 'e': Move this 'device_num' clause parsing before the 'device' clause parsing (actually move it in front of the 'device_type' parsing, for proper lexicographical ordering), so that we'll make sure to not parse a 'device_num' clause as a 'device' clause without parens/argument, followed by a (non-sensical) '_num' clause. (Research "Fortran whitespace insignificant" for the background.) Also, don't need the '&& openacc' conditional here, as 'OMP_CLAUSE_DEVICE_NUM' only appears in 'OACC_[...]_CLAUSES', so there's no ambiguity here to handle OpenACC vs. OpenMP syntax. > +match > +gfc_match_oacc_init (void) > +{ > + gfc_omp_clauses *c; > + if (gfc_match_omp_clauses (&c, OACC_INIT_CLAUSES, false, false, true) > + != MATCH_YES) > + return MATCH_ERROR; > + > + int device_n = 0; > + gfc_device_type_list *n, *list = c->device_type_list; > + > + for (; list; list = n) > + { > + n = list->next; > + if (list->device == GOMP_DEVICE_GCN + 1) > + { > + gfc_error ( > + "OpenACC init directive does not accept * argument in device_type > at %C"); > + return MATCH_ERROR; > + } > + device_n++; > + } > + > + if (device_n > 1) > + { > + gfc_error ( > + "OpenACC init directive only accepts one argument in device_type at > %C"); > + return MATCH_ERROR; > + } > + > + new_st.op = EXEC_OACC_INIT; > + new_st.ext.omp_clauses = c; > + return MATCH_YES; > +} This can be simplified, assuming that we simplify 'match_oacc_device_type_list', as per above. Then, we by construction have none or a single item in 'device_type_list', and may just: gcc_checking_assert (!list || !list->next); Or, maybe this code can become a simple call to 'match_acc', if we move the 'device_type_list' checking (that you have; or 'gcc_checking_assert' per my suggestion above) to the front end's "resolve" stage? See 'resolve_omp_clauses', where you alread have (type) checking of 'device_num_expr', and could similary verify 'device_type_list', potentially with differences per the respective OpenACC directive ('code->op'). > +match > +gfc_match_oacc_shutdown (void) May revise similar to 'gfc_match_oacc_init'. > +match > +gfc_match_oacc_set (void) May revise similar to 'gfc_match_oacc_init'. > +{ > + gfc_omp_clauses *c; > + if (gfc_match_omp_clauses (&c, OACC_SET_CLAUSES, false, false, true) > + != MATCH_YES) > + return MATCH_ERROR; > + > + int device_n = 0; > + gfc_device_type_list *n, *list = c->device_type_list; > + > + for (; list; list = n) > + { > + n = list->next; > + if (list->device == GOMP_DEVICE_GCN + 1) > + { > + gfc_error ( > + "OpenACC set directive does not accept * argument in device_type at > %C"); > + return MATCH_ERROR; > + } > + device_n++; > + } > + > + if (device_n > 1) > + { > + gfc_error ( > + "OpenACC set directive only accepts one argument in device_type at %C"); > + return MATCH_ERROR; > + } > + > + if (!c->device_type_list && !c->device_num_expr) > + { > + gfc_error ( > + "OpenACC set diretive needs at least one clause besides the if clause > at %C"); > + return MATCH_ERROR; > + } The "at least one" checking also belongs into the front end's "resolve" stage; see instances of "at least one" in 'resolve_omp_clauses', and also match the diagnostic text style that gets used in other such cases. (..., and no need to mention the 'if' clause here.) For reference, the OpenACC specification "Set Directive", "Restrictions": | * At least one 'default_async' [not yet applicable], 'device_num', or 'device_type' clause must appear. > + > + new_st.op = EXEC_OACC_SET; > + new_st.ext.omp_clauses = c; > + return MATCH_YES; > +} > @@ -11113,6 +11353,8 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses > *omp_clauses, > if (omp_clauses->async) > if (omp_clauses->async_expr) > resolve_scalar_int_expr (omp_clauses->async_expr, "ASYNC"); > + if (omp_clauses->device_num_expr) > + resolve_scalar_int_expr (omp_clauses->device_num_expr, "DEVICE_NUM"); > if (omp_clauses->num_gangs_expr) > resolve_positive_int_expr (omp_clauses->num_gangs_expr, "NUM_GANGS"); > if (omp_clauses->num_workers_expr) I suppose we should also handle 'device_type_list' here, in particular, in case that we get rid of the special-case 'gfc_device_type_list', as discussed above. In case that we're constructing the values in the front end (..., and as part of that process already validate the user input), they have to be "compliant" already, and we may verify/document that with some 'gcc_checking_assert' here. Don't we have to handle 'EXEC_OACC_INIT', 'EXEC_OACC_SHUTDOWN', 'EXEC_OACC_SET' in 'oacc_code_to_statement', to avoid running into 'default:', 'gcc_unreachable ();'? Are we missing test suite coverage where that would be relevant? > --- a/gcc/fortran/parse.cc > +++ b/gcc/fortran/parse.cc > @@ -790,6 +790,9 @@ decode_oacc_directive (void) > case 'h': > matcha ("host_data", gfc_match_oacc_host_data, ST_OACC_HOST_DATA); > break; > + case 'i': > + matcha ("init", gfc_match_oacc_init, ST_OACC_INIT); > + break; > case 'p': > matcha ("parallel loop", gfc_match_oacc_parallel_loop, > ST_OACC_PARALLEL_LOOP); > @@ -804,8 +807,10 @@ decode_oacc_directive (void) > matcha ("loop", gfc_match_oacc_loop, ST_OACC_LOOP); > break; > case 's': > + matcha ("set", gfc_match_oacc_set, ST_OACC_SET); > matcha ("serial loop", gfc_match_oacc_serial_loop, > ST_OACC_SERIAL_LOOP); > matcha ("serial", gfc_match_oacc_serial, ST_OACC_SERIAL); > + matcha ("shutdown", gfc_match_oacc_shutdown, ST_OACC_SHUTDOWN); > break; [high priority] Properly sort "set" lexicographically, please. > @@ -1975,7 +1980,8 @@ next_statement (void) > case ST_FORM_TEAM: case ST_SYNC_TEAM: \ > case ST_EVENT_POST: case ST_EVENT_WAIT: case ST_FAIL_IMAGE: \ > case ST_OACC_UPDATE: case ST_OACC_WAIT: case ST_OACC_CACHE: \ > - case ST_OACC_ENTER_DATA: case ST_OACC_EXIT_DATA > + case ST_OACC_INIT: case ST_OACC_ENTER_DATA: case ST_OACC_EXIT_DATA: \ > + case ST_OACC_SHUTDOWN: case ST_OACC_SET [high priority] Suggest to keep 'ST_OACC_INIT', 'ST_OACC_SHUTDOWN', 'ST_OACC_SET' grouped together, like you're doing elsewhere. > --- a/gcc/fortran/resolve.cc > +++ b/gcc/fortran/resolve.cc Don't we also have to handle 'EXEC_OACC_INIT', 'EXEC_OACC_SHUTDOWN', 'EXEC_OACC_SET' in 'gfc_resolve_blocks', to avoid running into 'default:', 'gfc_internal_error ("[...]");'? Are we missing test suite coverage where that would be relevant? > --- a/gcc/fortran/st.cc > +++ b/gcc/fortran/st.cc > @@ -291,6 +291,9 @@ gfc_free_statement (gfc_code *p) > case EXEC_OMP_TILE: > case EXEC_OMP_UNROLL: > case EXEC_OMP_WORKSHARE: > + case EXEC_OACC_INIT: > + case EXEC_OACC_SHUTDOWN: > + case EXEC_OACC_SET: > gfc_free_omp_clauses (p->ext.omp_clauses); > break; [high priority] Please sort those with the other 'EXEC_OACC_[...]' ones. > --- a/gcc/fortran/trans-openmp.cc > +++ b/gcc/fortran/trans-openmp.cc In 'gfc_trans_omp_clauses', similar to the existing: /* OpenACC 'nohost' clauses cannot appear here. */ gcc_checking_assert (!clauses->nohost); ..., add 'gcc_checking_assert's for '!clauses->device_num_expr', '!clauses->device_type_list', given that we (currently) don't expect these to appear in code paths where this function is called. (That's at least what I expect, given that those clauses (currently) only appear with OpenACC 'init', 'shutdown', 'set' directives, which we directly translate into 'BUILT_IN_GOACC_[...]' in the following functions.) > +tree > +gfc_trans_oacc_set_directive (gfc_code *code) > +{ > + stmtblock_t block; > + tree stmt; > + location_t loc = input_location; > + gfc_omp_clauses *clauses = code->ext.omp_clauses; > + > + gfc_start_block (&block); > + > + tree n_device = build_int_cst (integer_type_node, -1); Usually, we try to avoid magic constants (here: '-1'), but in this case, it should be clear what it's about: no 'device_num' clause specified by the user, and in that case, don't modify the value of ICV 'acc-current-device-num-var'. > + if (clauses->device_num_expr) > + n_device = gfc_convert_expr_to_tree (&block, clauses->device_num_expr); Please move the default initialization of 'n_device' into an 'else' branch. (That is, avoid the 'build_int_cst' function call if not necessary.) > + > + int device_type = GOMP_DEVICE_DEFAULT; > + if (clauses->device_type_list) > + device_type = clauses->device_type_list->device; Please add a 'gcc_checking_assert (!clauses->device_type_list->next)': we should, by construction, never get to translate an OpenACC 'set' directive that has a 'device_type' clause with more than one argument. > + > + tree d_type = build_int_cst (integer_type_node, device_type); I suggest to swap the construction of 'n_device' and 'd_type', to do it in the same order as they appear in the 'GOACC_set' function call. > + > + stmt = builtin_decl_explicit (BUILT_IN_GOACC_SET); > + > + stmt = build_call_expr_loc (loc, stmt, 2, d_type, n_device); > + > + if (clauses->if_expr) > + stmt = build3_loc (input_location, COND_EXPR, void_type_node, > + gfc_convert_expr_to_tree (&block, clauses->if_expr), > + stmt, NULL_TREE); > + > + gfc_add_expr_to_block (&block, stmt); > + > + return gfc_finish_block (&block); > +} > + > +tree > +gfc_trans_oacc_shutdown_directive (gfc_code *code) Please revise similary to 'gfc_trans_oacc_set_directive' comments. > +{ > + stmtblock_t block; > + tree stmt; > + location_t loc = input_location; > + gfc_omp_clauses *clauses = code->ext.omp_clauses; > + > + gfc_start_block (&block); > + > + tree n_device = build_int_cst (integer_type_node, -1); > + if (clauses->device_num_expr) > + n_device = gfc_convert_expr_to_tree (&block, clauses->device_num_expr); > + > + // All by default according the OpenACC spec > + int device_type = GOMP_DEVICE_GCN + 1; This latter thing, I assume, is for OpenACC specification "Shutdown Directive": | If no clauses appear then all available devices will be disconnected. [high priority] As per my 'match_oacc_device_type_kind' comment above, I suggest to use 'GOMP_DEVICE_NONE' ('0') for this case. (..., unless my suggestion is bogus, of course.) > + if (clauses->device_type_list) > + device_type = clauses->device_type_list->device; > + > + tree d_type = build_int_cst (integer_type_node, device_type); > + > + stmt = builtin_decl_explicit (BUILT_IN_GOACC_SHUTDOWN); > + > + stmt = build_call_expr_loc (loc, stmt, 2, d_type, n_device); > + > + if (clauses->if_expr) > + stmt = build3_loc (input_location, COND_EXPR, void_type_node, > + gfc_convert_expr_to_tree (&block, clauses->if_expr), > + stmt, NULL_TREE); > + > + gfc_add_expr_to_block (&block, stmt); > + > + return gfc_finish_block (&block); > +} > + > +tree > +gfc_trans_oacc_init_directive (gfc_code *code) Please revise similary to 'gfc_trans_oacc_set_directive' comments. > +{ > + stmtblock_t block; > + tree stmt; > + location_t loc = input_location; > + gfc_omp_clauses *clauses = code->ext.omp_clauses; > + > + gfc_start_block (&block); > + > + tree n_device = build_int_cst (integer_type_node, -1); > + if (clauses->device_num_expr) > + n_device = gfc_convert_expr_to_tree (&block, clauses->device_num_expr); > + > + // All by default according the OpenACC spec > + int device_type = GOMP_DEVICE_GCN + 1; That's, per my understanding, aimed at OpenACC specification "Init Directive", "device_num clause": | [...] If | no 'device_type' clause appears, then the specified device id will be initialized for all available | device types. [high priority] As per my 'match_oacc_device_type_kind' comment above, I suggest to use 'GOMP_DEVICE_NONE' ('0') for this case. (..., unless my suggestion is bogus, of course.) > + if (clauses->device_type_list) > + device_type = clauses->device_type_list->device; > + > + tree d_type = build_int_cst (integer_type_node, device_type); > + > + stmt = builtin_decl_explicit (BUILT_IN_GOACC_INIT); > + > + stmt = build_call_expr_loc (loc, stmt, 2, d_type, n_device); > + > + if (clauses->if_expr) > + stmt = build3_loc (input_location, COND_EXPR, void_type_node, > + gfc_convert_expr_to_tree (&block, clauses->if_expr), > + stmt, NULL_TREE); > + > + gfc_add_expr_to_block (&block, stmt); > + > + return gfc_finish_block (&block); > +} > @@ -9775,6 +9879,12 @@ gfc_trans_oacc_directive (gfc_code *code) > return gfc_trans_omp_atomic (code); > case EXEC_OACC_DECLARE: > return gfc_trans_oacc_declare (code); > + case EXEC_OACC_INIT: > + return gfc_trans_oacc_init_directive (code); > + case EXEC_OACC_SHUTDOWN: > + return gfc_trans_oacc_shutdown_directive (code); > + case EXEC_OACC_SET: > + return gfc_trans_oacc_set_directive (code); > default: > gcc_unreachable (); > } Let's leave it as-is for now, but later might also research whether we could also implement these via 'gfc_trans_oacc_executable_directive' instead of these three new functions? (I wonder; not actually sure.) > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/goacc/acc-init-1.f90 > @@ -0,0 +1,47 @@ > +! Test parsing and lowering of the OpenACC init directive. > + > +! { dg-do compile } > +! { dg-additional-options "-fdump-tree-original" } > + > +subroutine init0 > + implicit none > + !$acc init > +end subroutine init0 > + > +subroutine init1 > + implicit none > + !$acc init device_type(host) > +end subroutine init1 > + > +subroutine init2 > + implicit none > + !$acc init device_type(nvidia) > +end subroutine init2 > + > +subroutine init3 > + implicit none > + !$acc init device_num(0) > +end subroutine init3 > + > +subroutine init4 > + implicit none > + !$acc init device_type(host) device_num(0) > +end subroutine init4 > + > +subroutine init5 > + implicit none > + !$acc init if(.false.) > +end subroutine init5 > + > +subroutine init6(l) > + implicit none > + logical, value :: l > + !$acc init if(l) device_type(radeon) device_num(1) > +end subroutine init6 > + > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(9, -1\\)" 2 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(2, -1\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(5, -1\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(9, 0\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(2, 0\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_init \\(8, 1\\)" 1 > "original" } } Per these 'scan-tree-dump-times' checks, we're only verifying the expectd number of matches, but don't verify which source code function they belong to. If we wanted to do better, we'd either need some more elaborate scanning functionality (à la 'check-function-bodies', but for '-fdump-[...]'; too big of a project to include in this GSoC), or split this test case up into separate files per each '$acc init' directive (ugly) -- or just leave it like this. Let's do that latter. > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/goacc/acc-set-1.f90 Similar remark as for 'gfortran.dg/goacc/acc-init-1.f90', but again: leave it like this. (..., just addressing the remark below.) > @@ -0,0 +1,36 @@ > +! Test parsing and lowering of the OpenACC set directive. > + > +! { dg-do compile } > +! { dg-additional-options "-fdump-tree-original" } > + > +subroutine set0 > + implicit none > + !$acc set device_type(host) > +end subroutine set0 > + > +subroutine set1 > + implicit none > + !$acc set device_type(nvidia) > +end subroutine set1 > + > +subroutine set2 > + implicit none > + !$acc set device_num(0) > +end subroutine set2 > + > +subroutine set3 > + implicit none > + !$acc set device_type(host) device_num(0) > +end subroutine set3 > + > +subroutine set4(l) > + implicit none > + logical, value :: l > + !$acc set if(l) device_type(radeon) device_num(1) > +end subroutine set4 > + > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(2, -1\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(5, -1\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(1, -1\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(2, 0\\)" 1 > "original" } } > +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(8, 1\\)" 1 > "original" } } [high priority] I'm seeing one FAIL here: PASS: gfortran.dg/goacc/acc-set-1.f90 -O (test for excess errors) PASS: gfortran.dg/goacc/acc-set-1.f90 -O scan-tree-dump-times original "__builtin_GOACC_set \\(2, -1\\)" 1 PASS: gfortran.dg/goacc/acc-set-1.f90 -O scan-tree-dump-times original "__builtin_GOACC_set \\(5, -1\\)" 1 FAIL: gfortran.dg/goacc/acc-set-1.f90 -O scan-tree-dump-times original "__builtin_GOACC_set \\(1, -1\\)" 1 PASS: gfortran.dg/goacc/acc-set-1.f90 -O scan-tree-dump-times original "__builtin_GOACC_set \\(2, 0\\)" 1 PASS: gfortran.dg/goacc/acc-set-1.f90 -O scan-tree-dump-times original "__builtin_GOACC_set \\(8, 1\\)" 1 Don't you get that one, too? Maybe it's just some wrong magic numbers that are being verified here, but if not, for the time being just XFAIL that specific sub-test: -! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(1, -1\\)" 1 "original" } } +! { dg-final { scan-tree-dump-times "__builtin_GOACC_set \\(1, -1\\)" 1 "original" { xfail *-*-* } } } ..., and then sort that out, later. > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/goacc/acc-shutdown-1.f90 Similar remark as for 'gfortran.dg/goacc/acc-init-1.f90', but again: leave it like this. > --- a/libgomp/libgomp.map > +++ b/libgomp/libgomp.map > @@ -650,6 +650,9 @@ GOACC_2.0.2 { > global: > GOACC_enter_data; > GOACC_exit_data; > + GOACC_init; > + GOACC_shutdown; > + GOACC_set; > } GOACC_2.0.1; [high priority] We shall not have new symbols use a symbol version that has already appeared in a proper GCC release. The 'init', 'shutdown', 'set' directives were added in OpenACC 2.5, so please add a new 'GOACC_2.5' symbol version that links to 'GOACC_2.0.2'. > --- a/libgomp/oacc-init.c > +++ b/libgomp/oacc-init.c > +void > +GOACC_set (int d, int n_device) > +{ > + if ((acc_device_t) d != acc_device_default) > + acc_set_device_type ((acc_device_t) d); > + > + acc_set_device_num (n_device, (acc_device_t) d); > +} I don't understand the 'd != acc_device_default' conditional, doesn't 'acc_set_device_type' already handle this case ("no change")? Or is this about something else? > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/init-1.f90 > @@ -0,0 +1,30 @@ > +! { dg-do run } > +! > +! Test the OpenACC init directive at run time. > + > +use openacc > + > +implicit none > + > +logical :: l > + > +if (acc_get_num_devices (acc_device_host) .ne. 1) stop 1 > + > +!$acc init device_type(host) > + > +if (acc_get_device_type () .ne. acc_device_host) stop 2 > +if (acc_get_device_num (acc_device_host) .ne. 0) stop 3 > + > +call acc_shutdown (acc_device_host) > + > +l = .true. > +!$acc init if (l) device_type(host) > + > +if (acc_get_device_type () .ne. acc_device_host) stop 4 > + > +call acc_shutdown (acc_device_host) > + > +l = .false. > +!$acc init if (l) device_type(host) > + > +end We also need test coverage for '$acc init' without 'device_type' clause, 'device_type' clauses with argument different from 'host', and for 'device_num' clauses. You could, for example, port 'libgomp.oacc-fortran/lib-5.f90': one variant where it stays as-is and just uses '$acc init' instead of 'call acc_init', and similarly for the other directives' test cases, and one variant where you replace all applicable 'call's with directive variants. That last test that you have in 'libgomp.oacc-fortran/init-1.f90' ('if (l)' with 'l = .false.') isn't very meaningful; it would also pass if it erroneous were interpreted as 'if (.true.)'. You could do something like: if (acc_get_num_devices (acc_device_nvidia) .ne. 0) then l = .true. !$acc init if (l) device_type(nvidia) l = .false. !$acc init if (l) device_type(host) ! Would raise error if actually attempted. if (acc_get_device_type () .ne. acc_device_nvidia) error stop ..., and vice versa for 'device_type(radeon)'. And/or, similar to what you have in 'libgomp.oacc-fortran/set-1.f90', use an out-of-bounds 'device_num(N)' with 'if(.false.)', where 'N = acc_get_num_devices(acc_device_default)' or similar. > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/set-1.f90 OK. Might just similarly to what I suggested above for 'libgomp.oacc-fortran/init-1.f90' add some more variation for other 'device_type' clause variants. > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/shutdown-1.f90 > @@ -0,0 +1,32 @@ > +! { dg-do run } > +! > +! Test the OpenACC shutdown directive at run time. > + > +use openacc > + > +implicit none > + > +logical :: l > + > +if (acc_get_num_devices (acc_device_host) .ne. 1) stop 1 > + > +!$acc init device_type(host) > + > +if (acc_get_device_type () .ne. acc_device_host) stop 2 > +if (acc_get_device_num (acc_device_host) .ne. 0) stop 3 > + > +!$acc shutdown device_type(host) > + > +l = .true. > +!$acc init if (l) device_type(host) > + > +if (acc_get_device_type () .ne. acc_device_host) stop 4 > + > +!$acc shutdown if (l) device_type(host) > + > +l = .false. > +!$acc shutdown if (l) device_type(host) > + > +!$acc shutdown > + > +end [high priority] I'm seeing this one FAIL in all my testing, due to the very last '!$acc shutdown': libgomp: no device initialized That's probably not surprising: we've already '!$acc shutdown'ed a few lines above. Don't you get that one, too? It PASSes for me, if I just remove it: !$acc shutdown if (l) device_type(host) -!$acc shutdown - end Actually, the corresponding OpenACC specification section "Runtime Library Routines", "acc_shutdown" states that: | * If the program attempts to shut down the 'acc_device_host' device type, the behavior is undefined. ..., so '!$acc shutdown' of 'device_type(host)' looks questionable generally? (..., but it's still OK, under the assumption that the GCC/libgomp implementation correctly handles this case; we're then testing implementation-specific behavior.) With testing enhanced as per my 'libgomp.oacc-fortran/init-1.f90' comments, I assume we'll similarly get increased coverage of '!$acc shutdown'. I was thinking we need to update 'libgomp/libgomp.texi', but actually we don't have to: we document the "OpenACC Runtime Library Routines" in quite some detail, but nowhere document the directive variants. (Same for OpenMP, for that matter.) So, please look into the "[high priority]" items, so that we can get your patch into GCC trunk, and then work on the other items that I raised. Looking forward to your "v2"! :-) Grüße Thomas
