On Wed, Nov 20, 2019 at 02:06:18PM +0100, Tobias Burnus wrote:
> ** Included in the attached patch are the following previously posted
> patches: [1] the trivial libgomp/oacc-mem.c change, [2] only the remaining
> single-line change in omp-low.c, [3] the trans-openmp.c changes (which had
> to be modified+extended), and [5] the test cases. ([2] and [4] are already
> in GCC 10.) See:
> https://gcc.gnu.org/ml/gcc-patches/2019-07/threads.html#00960 for the
> original patches.
>
> PS: For full OpenMP support, (absent) optional arguments also needed to be
> handled for data-share clauses.
Sure.
> 2019-10-20 Tobias Burnus <tob...@codesourcery.com>
> Kwok Cheung Yeung <k...@codesourcery.com>
>
> gcc/fortran/
> * trans-openmp.c (gfc_build_conditional_assign,
> gfc_build_conditional_assign_expr): New static functions.
> (gfc_omp_finish_clause, gfc_trans_omp_clauses): Handle mapping of
> absent optional arguments and fix mapping of present optional args.
>
> gcc/
> * omp-low.c (lower_omp_target): For optional arguments, deref once
> more to obtain the type.
>
> libgomp/
> * oacc-mem.c (update_dev_host, gomp_acc_insert_pointer): Just return
> if input it a NULL pointer.
> * testsuite/libgomp.oacc-c-c++-common/lib-43.c: Remove; dependent on
> diagnostic of NULL pointer.
> * testsuite/libgomp.oacc-c-c++-common/lib-47.c: Ditto.
> * testsuite/libgomp.fortran/optional-map.f90: New.
> * testsuite/libgomp.fortran/use_device_addr-1.f90
> (test_dummy_opt_callee_1_absent): New.
> (test_dummy_opt_call_1): Call it.
> * testsuite/libgomp.fortran/use_device_addr-2.f90: Likewise.
> * testsuite/libgomp.fortran/use_device_addr-3.f90: Likewise.
> * testsuite/libgomp.fortran/use_device_addr-4.f90: Likewise.
> * testsuite/libgomp.oacc-fortran/optional-cache.f95: New.
> * testsuite/libgomp.oacc-fortran/optional-data-copyin-by-value.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-data-copyin.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-data-copyout.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-data-enter-exit.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-declare.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-firstprivate.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-host_data.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-nested-calls.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-private.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-reduction.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-update-device.f90: New.
> * testsuite/libgomp.oacc-fortran/optional-update-host.f90: New.
Ok, with some formatting nits fixed.
> @@ -1199,6 +1257,8 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p)
> }
>
> tree c2 = NULL_TREE, c3 = NULL_TREE, c4 = NULL_TREE;
> + tree present = gfc_omp_is_optional_argument (decl)
> + ? gfc_omp_check_optional_argument (decl, true) : NULL_TREE;
I think emacs users (I'm not one of them) want ()s around, otherwise emacs
misformats that. So
tree present = (gfc_omp_is_optional_argument (decl)
? gfc_omp_check_optional_argument (decl, true) : NULL_TREE);
> @@ -1232,17 +1314,43 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p)
> stmtblock_t block;
> gfc_start_block (&block);
> tree type = TREE_TYPE (decl);
> - tree ptr = gfc_conv_descriptor_data_get (decl);
> + tree ptr;
> +
> + if (present)
> + ptr = gfc_build_conditional_assign_expr (
> + &block, present,
> + gfc_conv_descriptor_data_get (decl),
> + null_pointer_node);
I must say I don't like very much formatting like that, I'd find it cleaner
to use temporary to have shorter argument and put all the arguments after
the ( column.
> + else
> + ptr = gfc_conv_descriptor_data_get (decl);
In this case, it could even be:
ptr = gfc_conv_descriptor_data_get (decl);
if (present)
ptr = gfc_build_conditional_assign_expr (&block, present, ptr,
null_pointer_node);
by just using the same call from the else.
> @@ -2252,6 +2385,9 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> gfc_omp_clauses *clauses,
> TREE_ADDRESSABLE (decl) = 1;
> if (n->expr == NULL || n->expr->ref->u.ar.type == AR_FULL)
> {
> + tree present = gfc_omp_is_optional_argument (decl)
> + ? gfc_omp_check_optional_argument (decl, true)
> + : NULL_TREE;
> if (POINTER_TYPE_P (TREE_TYPE (decl))
> && (gfc_omp_privatize_by_reference (decl)
> || GFC_DECL_GET_SCALAR_POINTER (decl)
See above.
> @@ -2284,6 +2420,10 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> gfc_omp_clauses *clauses,
> {
> tree type = TREE_TYPE (decl);
> tree ptr = gfc_conv_descriptor_data_get (decl);
> + if (present)
> + ptr = gfc_build_conditional_assign_expr (
> + block, present, ptr,
> + null_pointer_node);
And here the other comment. It is much more indented though, but you could
use a temporary, like:
tree nullarg = null_pointer_node;
if (present)
ptr
= gfc_build_conditional_assign_expr (block, present,
ptr, nullarg);
Though, if it is too much for you, ignore.
Another option would be shorten the name of the function, say
s/conditional/cond/.
There were some discussions about lifting the 80 column restriction and bump
it to something like +-130, but nothing happened yet.
> + ptr = gfc_build_conditional_assign_expr (
> + block, present, ptr,
> + null_pointer_node);
Again.
> + stmtblock_t cond_block;
> + gfc_init_block (&cond_block);
> + tree size = gfc_full_array_size (&cond_block, decl,
> + GFC_TYPE_ARRAY_RANK (type));
Here one could use a temporary for GFC_TYPE_ARRAY_RANK (type);
> + if (present)
> + {
> + tree var = gfc_create_var (gfc_array_index_type,
> + NULL);
> + tree cond = fold_build2_loc (input_location,
> + NE_EXPR,
> + boolean_type_node,
> + present,
> + null_pointer_node);
> + gfc_add_modify (&cond_block, var, size);
> + gfc_add_expr_to_block (block,
> + build3_loc (input_location, COND_EXPR,
> + void_type_node, cond,
> + gfc_finish_block (&cond_block),
> + NULL_TREE));
And here for the expr, perhaps just reuse the cond variable.
> @@ -2346,6 +2534,18 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> gfc_omp_clauses *clauses,
> OMP_CLAUSE_SIZE (node), elemsz);
> }
> }
> + else if (present
> + && TREE_CODE (decl) == INDIRECT_REF
> + && TREE_CODE (TREE_OPERAND (decl, 0))
> + == INDIRECT_REF)
Above I'd expect
&& (TREE_CODE (TREE_OPERAND (decl, 0))
== INDIRECT_REF))
but perhaps I'm just pushing my coding style too much, ignore in that case.
Jakub
