Re: [PATCH v5 5/11] openmp, fortran: Add support for iterators in OpenMP 'target update' constructs (Fortran)

[Tobias Burnus]

Kwok Cheung Yeung wrote on July 9, 2025:

Subject: [PATCH 05/11] openmp, fortran: Add support for iterators in OpenMP
  'target update' constructs (Fortran)

This adds Fortran support for iterators in 'to' and 'from' clauses in the
'target update' OpenMP directive.

...

@@ -1418,16 +1419,67 @@ gfc_match_motion_var_list (const char *str, 
gfc_omp_namelist **list,
    if (m != MATCH_YES)
      return m;

- match m_present = gfc_match (" present : ");

+  gfc_namespace *ns_iter = NULL, *ns_curr = gfc_current_ns;
+  locus old_loc = gfc_current_locus;
+  int present_modifier = 0;
+  int iterator_modifier = 0;
+  locus second_present_locus = old_loc;
+  locus second_iterator_locus = old_loc;
+
+  for (;;)
+    {
+      locus current_locus = gfc_current_locus;
+      if (gfc_match ("present ") == MATCH_YES)
+       {
+         if (present_modifier++ == 1)
+           second_present_locus = current_locus;
+       }
+      else if (gfc_match_iterator (&ns_iter, true) == MATCH_YES)
+       {
+         if (iterator_modifier++ == 1)
+           second_iterator_locus = current_locus;
+       }
+      else
+       break;
+      gfc_match (", ");
+    }

The code above accepts

  !$omp target update to(present iterator(i=0:4) : x)

which has no comma between the 'present' and the iterator modifier.

However, while between the directive and is first clause
and between clauses, the comma is optional, between modifiers
in a clause, a comma is in principle required.

That's quite tedious noted in the <= 5.1 syntax and since 5.2
it is codified as:

"Unless otherwise specified, modified arguments are pre-modified,
 for which the format is:
 [modifier-specification [[, modifier-specification] ,... ] 
:]clause-argument-list"

Thus, the code above should be rejected. However, I have to admit
that the 5.1, only, syntax for (only) 'target update' gets it wrong:

   to([motion-modifier[,] [motion-modifier[,] ...]: ] locator-list)
   from([motion-modifier[,] [motion-modifier[,] ...]: ] locator-list)

by making the 'comma' optional and also permitting a tailing comma!


As your parser matches 5.1: Can you add a comment to that parser
code like the following:

/* For the modifier list, OpenMP 5.1, only, permits leaving out
   the separating comma and having tailing comma.  */

That way we will not wonder about it later - and can consider to
diagnose it in the future (like for -Wdeprecated).

* * *

On the other hand, I also wouldn't mind if we plug this clear
syntax bug and only accept the 'normal' syntax, required also
for the 'to'/'from' clauses with 5.2/6.0.

* * *

Actually, the current code also permits the following with
free-form Fortran - which is surprising:

!$omp target update to(presentiterator(i=0:4), : x)

For fixed-form Fortran, that's actually fine (assuming the
',' is optional) as spaces have no meaning, recall the fun
with
  d o i = ...
which can be a 'do i = ...' loop or a 'doi = ...' assignment,
depending whether there is a ',' on the RHS. But in free-form
Fortran spaces actually matter.

I am a bit unsure what to do about it - keeping it (and adding
a note to the comment above), handling it by requiring the comma,
or do something specific about it.

* * *

+++ b/libgomp/testsuite/libgomp.fortran/target-update-iterators-2.f90
@@ -0,0 +1,63 @@
+! { dg-do run }
+! { dg-require-effective-target offload_device_nonshared_as }

I was afraid that we completely skip those tests, unless
'offload_device_nonshared_as' - which would be bad. However, I
see that we still have some of those:

libgomp.c-c++-common/target-update-iterators-{1,4}.c
libgomp.fortran/target-map-iterators-[1-5].f90
libgomp.fortran/target-update-iterators-{1,4}.f90

Thus, that seems to be fine.

* * *

LGTM with the parsing part being addressed.

Thanks,

Tobias