Re: [PATCHv2] openmp: Add support for 'present' modifier

[Tobias Burnus <tobias@codesourcery.com>]

Hi Kwok,

On 17.02.23 12:45, Kwok Cheung Yeung wrote:
> This is a revised version of the patch for the 'present' modifier for
> OpenMP. Compared to the first version, three improvements have been made:
>
> - A bug which caused bootstrapping with a '-m32' multilib on x86-64 to
> fail due to pointer size issues has been fixed.
> - The Fortran parse tree dump now shows clauses with 'present' applied.
> - The reordering of OpenMP clauses has been moved to
> gimplify_scan_omp_clauses, where the other clause reordering rules are
> applied.

thanks for the patch; I have a bunch of smaller review comments, requiring small
code changes or more tedious but still simple changes.

Namely:

In the ChangeLog:
>          (c_parser_omp_target_data): Allow map clauses with 'present'
>          modifiers.
>          (c_parser_omp_target_enter_data): Likewise.
>          (c_parser_omp_target_exit_data): Likewise.
>          (c_parser_omp_target): Likewise.

Those be combined; a separate entry is only required per file not per
function name.

> +             if (kind == OMP_CLAUSE_FROM || kind == OMP_CLAUSE_TO)
> +               OMP_CLAUSE_SET_MOTION_MODIFIER (u, OMP_CLAUSE_MOTION_NONE);

This should not be needed as 'build_omp_clause' memset '\0' the data and
OMP_CLAUSE_MOTION_NONE == 0 (as it should).

However, as you really only have two values, denoting that the modifier has
been specified or not, you should really use an available existing flag. For instance,
other code uses base.deprecated_flag – which could also be used here.

Macro wise, this would then permit to use:
   OMP_CLAUSE_MOTION_PRESENT (node) = 1;
or
   OMP_CLAUSE_TO_PRESENT (node) = 1;
   OMP_CLAUSE_FROM_PRESENT (node) = 1;
and 'if (OMP_... (node))' which is shorter and is IMHO to be also more readable.

* * *

I think c_parser_omp_var_list_parens / cp_parser_omp_var_list / gfc_match_omp_variable_list
should not be modified.

For C/C++, you just could do the '(' + {'present', ':' } parsing before the call to
   c_parser_omp_variable_list / cp_parser_omp_var_list_no_open
and then loop over 'list' after the call - and similarly for Fortran.

And besides not cluttering a generic function, we will also soon add support for
'mapper' (→ Julian's patch set adds generic mapper support) and 'iterator' is also
missing. And we really do not want those in the generic function!

> +       kind = always_present_modifier ? GOMP_MAP_ALWAYS_PRESENT_FROM
> +              : present_modifier ? GOMP_MAP_PRESENT_FROM
> +              : always_modifier ? GOMP_MAP_ALWAYS_FROM
> +              : GOMP_MAP_FROM;

Can you wrap the RHS in parenthesis, i.e. 'kind = (' ... ');' to aid some
editors in terms of indenting. (I assume 'emacs' is that editor, which I don't use.)

> +               tkind
> +                 = OMP_CLAUSE_MOTION_MODIFIER (c) == OMP_CLAUSE_MOTION_PRESENT
> +                   ? GOMP_MAP_PRESENT_TO : GOMP_MAP_TO;

Likewise.


* * *

> @@ -1358,6 +1371,7 @@ typedef struct gfc_omp_namelist
>            ENUM_BITFIELD (gfc_omp_linear_op) op:4;
>            bool old_modifier;
>          } linear;
> +      gfc_omp_motion_modifier motion_modifier;
>         struct gfc_common_head *common;
>         bool lastprivate_conditional;
>       } u;


I think a 'bool present;' would do here. Can you additionally move the
pointers first and then the bitfields/enums later? That way,
less space is wasted by padding and we might even save space despite
adding another variable.

> @@ -2893,20 +2912,38 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
>                        if (close_modifier++ == 1)
>                          second_close_locus = current_locus;
>                      }
> +                 else if (gfc_match ("present ") == MATCH_YES)
> +                   {
> +                     if (present_modifier++ == 1)
> +                       second_close_locus = current_locus;
> +                   }
> ...

This code is broken in terms of error diagnostic. You need to handle this
differently to get diagostic for:
   'map(present, present : var)'

Can you fix the code + add a testcase (e.g. by augmenting the 'always' testcase
files testsuite/gfortran.dg/gomp/map-{7,8}.f90).


> +                   gomp_fatal ("present clause: !omp_target_is_present "

I personally find the error a bit unclear. How about something more explicit
like: 'present clause: not present on the device' - or something like that?

> +/* { dg-do run { target offload_target_any } } */


This needs to be '{ target offload_device }' - i.e. the default device needs to be
an offload device (!= the initial device).

(Side note: We may need to consider whether offload_device_nonshared_as might make sense, but
the current omp_target_is_present and omp_target_(dis)associate_ptr implies that
we will still go though this route with USM for explicitly mapped variables
(but do not do any actual mapping in that case).
But that we can handle, once the USM code gets merged and we get FAILs.)

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955