This patch is based on Kwok's patch, posted as (4/5) at
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00964.html – which is targeting OpenACC's use_device* – but it also applies to OpenMP use_device_{ptr,addr}. I added an OpenMP test case. It showed that for arguments with value attribute and for assumed-shape array, one needs to do more — as the decl cannot be directly used for the is-argument-present check. (For 'value', a hidden boolean '_' + arg-name is passed in addition; for assumed-shape arrays, the array descriptor "x" is replaced by the local variable "x.0" (with "x.0 = x->data") and the original decl "x" is in GFC_DECL_SAVED_DESCRIPTOR. Especially for assumed-shape arrays, the new decl cannot be used unconditionally as it is uninitialized when the argument is absent.) Bootstrapped and regtested on x86_64-gnu-linux without offloading + with nvptx. OK? Cheers, Tobias *The OpenACC test cases are in 5/5 and depend on some other changes. Submission of {1,missing one line of 2,3,5}/5 is planned next. PPS: For fully absent-optional support, mapping needs to be handled for OpenACC (see Kwok's …/5 patches) and OpenMP (which is quite different on FE level) – and OpenMP also needs changes for the share clauses.] |
Thinking about the patch over night, I have now updated it a bit:
Namely, I only add the "if(present-check)" condition, if the original variable is dereferenced. There is no need for code like omp_data_arr.c = c == NULL ? NULL : c; and then, after the libgomp call, code like "c_2 = c == NULL ? NULL : omp_data_arr.c;"; due to the libgomp call, the latter cannot even be optimized away. Hence, I added 'do_optional_check'; additionally, I had a libgomp.fortran/use_device_ptr-optional-1.f90 change floating around, which I included. Otherwise unchanged. Retested. OK for the trunk? Cheers, Tobias On 11/6/19 4:04 PM, Tobias Burnus wrote: > This patch is based on Kwok's patch, posted as (4/5) at > https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00964.html – which is > targeting OpenACC's use_device* – but it also applies to OpenMP > use_device_{ptr,addr}. > > I added an OpenMP test case. It showed that for arguments with value > attribute and for assumed-shape array, one needs to do more — as the > decl cannot be directly used for the is-argument-present check. > > (For 'value', a hidden boolean '_' + arg-name is passed in addition; > for assumed-shape arrays, the array descriptor "x" is replaced by the > local variable "x.0" (with "x.0 = x->data") and the original decl "x" > is in GFC_DECL_SAVED_DESCRIPTOR. Especially for assumed-shape arrays, > the new decl cannot be used unconditionally as it is uninitialized > when the argument is absent.) > > Bootstrapped and regtested on x86_64-gnu-linux without offloading + > with nvptx. > OK? > > Cheers, > > Tobias > > *The OpenACC test cases are in 5/5 and depend on some other changes. > Submission of {1,missing one line of 2,3,5}/5 is planned next. > PPS: For fully absent-optional support, mapping needs to be handled > for OpenACC (see Kwok's …/5 patches) and OpenMP (which is quite > different on FE level) – and OpenMP also needs changes for the share > clauses.] > |
Hi Jakub,
thanks for the review. On 11/8/19 3:39 PM, Jakub Jelinek wrote: >> + /* Walk function argument list to find the hidden arg. */ >> + decl = DECL_ARGUMENTS (DECL_CONTEXT (decl)); >> + for ( ; decl != NULL_TREE; decl = TREE_CHAIN (decl)) >> + if (DECL_NAME (decl) == tree_name) >> + break; > Is this reliable? I mean, consider -fallow-leading-underscore with: > subroutine foo (a, _a) I also assume that this will break; unlikely in real-world code but still. > Not really sure if additional DECL_ARTIFICIAL (decl) test would be enough. At least, I cannot quickly come up with a case where it will break. – I have now added it; also to the existing trans-expr.c function – which uses the used and, hence, same algorithm. >> +omp_check_optional_argument (tree decl, bool also_value) > Why is the argument called for_present_check in the langhook and > also_value here? Looks inconsistent. Because I initially was thinking only of the VALUE attribute until I realized that assumed-shape arrays have the same issue; they use a local variable for the data – and make the actual array descriptor available via lang-specific field. – As the use is either an extra deref is needed or a check whether the variable is present, I changed the meaning – seemingly, three places survived with the old name. With DECL_ARTIFICIAL added and also_value replaced: Build on x86-64-gnu-linux. OK once regtested? Tobias |
Free forum by Nabble | Edit this page |