Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]

[Andre Vehreschild]

Hi Tobias,

I am wondering why the testcase has no `!{ dg-do ... }` line. What will dejagnu
do then? Sorry for the may be stupid question, but I never encountered a
testcase without a dg-do line. It was the minimum for me.

Besides that the patch looks ok to me. 

- Andre

On Fri, 26 Jul 2024 20:34:18 +0200
Tobias Burnus  wrote:

> Updated patch - only change is to the testcase:
> 
> * With the just posted patch for PR116107, array sections with offset 
> work for 'link', hence, I updated the testcase.
> 
> * For 'arr2', I added ref to the associated PR.
> 
> I intent to commit it once PR116107 has been committed.
> 
> Tobias
> 
> Tobias Burnus wrote:
> > Hi all,
> >
> > it turned out that 'declare target' with 'link' clause was broken in
> > multiple ways.
> >
> > The main fix is the attached patch, i.e. namely pushing the variables
> > already to the offload-vars list already in the FE.
> >
> > When implementing it, I noticed:
> > * C has a similar issue when using nested functions, which is
> >a GNU extension →https://gcc.gnu.org/115574
> >
> > * When doing partial mapping of arrays (which is one of the reasons for
> > 'link'), offsets are mishandled in Fortran (not tested in C), see FIXME in
> > the patch) There: arr2(10) should print 10 but with map(arr2(10:)) it
> > prints 19. (I will file a PR about this).
> >
> > * It might happen that linked variables do not get linked. I have not
> > investigated why, but 'arr2' gives link errors – while 'arr' works.
> >See FIXME in the patch. (I will file a PR about this)
> >
> > * For COMMON blocks, map(/common/) is rejected,https://gcc.gnu.org/PR115577
> >
> > * When then mapping map(a,b,c) which is identical for 'common /mycom/
> > a,b,c', it fails to link the device side as the 'mycom_' symbol cannot be
> > found on the device side.  (I will file a PR about this)
> >
> > As COMMON as issues, an alternative would be to defer the trans-common.cc
> > changes to a later patch.
> >
> > Comments, questions, concerns?
> >
> > Tobias
> >
> > PS: Tested with nvptx offloading with a page-migration supporting system
> > with nvptx and GCN offloading configured and no new fails observed.  


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]

[Tobias Burnus]

Hi Andre,

Andre Vehreschild wrote:

I am wondering why the testcase has no `!{ dg-do ... }` line. What will dejagnu
do then? Sorry for the may be stupid question, but I never encountered a
testcase without a dg-do line. It was the minimum for me.


Well, then you need look harder ;-)

In gcc/testsuite/, the default is '{ dg-do compile }', i.e. you can
specify or leave out that line without any additional effect. Having it
might be a tad clearer, albeit makes the test a tad longer.

But if you want to 'run' or 'link', you need to specify the dg-do line.
There are several files which don't have the "dg-do compile" line, also
under gcc/testsuite/gfortran.dg

In case of libgomp, it is becomes interesting: the default is running
the code, i.e. you need a 'compile' or 'link' when it shouldn't be run.

However, at least for Fortran (libgomp.{oacc-}fortran), there is a
difference between specifying nothing and specifying 'dg-do run': In
case of the default, it is compiled and run. But if you specify 'dg-do
run', it is compiled multiple times with different optimization options
and then run.

(Actually, also under gcc/testsuite/gfortran.dg, you get multiple
compilations + runs with 'dg-do run'. If you use dg-additional-options,
you can also add options. I think with dg-options, you set it to a
single run [not confirmed].)

The downside of compiling + running it multiple times is a longer test
time without any real benefit. However, especially with Fortran,
compiling with different optimization levels did expose issues in the
past, both in the Fortran front end and in the middle end. — Thus, there
some benefit of using it.

In any case, there more complex the code is that front-end + middle-end
code have to process, the more useful is "dg-do run". The more work is
done by the run-time library, be it libgfortran or libgomp, the less
useful it becomes as the heavy lifting is done in the run-time library.
— As libgomp progressing already takes quite some time (albeit it can
now run in parallel), there are some who prefer few 'dg-do run' and
others who prefer if all Fortran testcases there use 'dg-do run' …

I hope it helps,

Tobias

Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]

[Andre Vehreschild]

Thanks a lot Tobias,

yes, I could have looked harder :-)

This isn't by any chance documented on the developer website of gcc somewhere?
It would be sad, if that knowledge is not publicy available for the future.

Thanks again for the explanation and keep up the good work.

Regards,
Andre

On Mon, 29 Jul 2024 09:29:28 +0200
Tobias Burnus  wrote:

> Hi Andre,
> 
> Andre Vehreschild wrote:
> > I am wondering why the testcase has no `!{ dg-do ... }` line. What will
> > dejagnu do then? Sorry for the may be stupid question, but I never
> > encountered a testcase without a dg-do line. It was the minimum for me.  
> 
> Well, then you need look harder ;-)
> 
> In gcc/testsuite/, the default is '{ dg-do compile }', i.e. you can
> specify or leave out that line without any additional effect. Having it
> might be a tad clearer, albeit makes the test a tad longer.
> 
> But if you want to 'run' or 'link', you need to specify the dg-do line.
> There are several files which don't have the "dg-do compile" line, also
> under gcc/testsuite/gfortran.dg
> 
> In case of libgomp, it is becomes interesting: the default is running
> the code, i.e. you need a 'compile' or 'link' when it shouldn't be run.
> 
> However, at least for Fortran (libgomp.{oacc-}fortran), there is a
> difference between specifying nothing and specifying 'dg-do run': In
> case of the default, it is compiled and run. But if you specify 'dg-do
> run', it is compiled multiple times with different optimization options
> and then run.
> 
> (Actually, also under gcc/testsuite/gfortran.dg, you get multiple
> compilations + runs with 'dg-do run'. If you use dg-additional-options,
> you can also add options. I think with dg-options, you set it to a
> single run [not confirmed].)
> 
> The downside of compiling + running it multiple times is a longer test
> time without any real benefit. However, especially with Fortran,
> compiling with different optimization levels did expose issues in the
> past, both in the Fortran front end and in the middle end. — Thus, there
> some benefit of using it.
> 
> In any case, there more complex the code is that front-end + middle-end
> code have to process, the more useful is "dg-do run". The more work is
> done by the run-time library, be it libgfortran or libgomp, the less
> useful it becomes as the heavy lifting is done in the run-time library.
> — As libgomp progressing already takes quite some time (albeit it can
> now run in parallel), there are some who prefer few 'dg-do run' and
> others who prefer if all Fortran testcases there use 'dg-do run' …
> 
> I hope it helps,
> 
> Tobias
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]

[Tobias Burnus]

Hi Andre, hi all,

Andre Vehreschild wrote:

yes, I could have looked harder 🙂


I wrote ;-) on purpose as this feature is somewhat hidden and writing 
'dg-do compile' doesn't harm.


In case of gcc/testsuite, the 'run' is also needed and were often missed 
(or rather caused by invalid variants such as 'dg-run' (should be: 
'dg-do run') or '{dg-do run }' (missing space after '{') prevented the 
running of the code). Sam did fix some of those (and some other dg-* 
issues) recently, e.g. in r15-2349-ga75c6295252d0d (→ 
https://gcc.gnu.org/r15-2349-ga75c6295252d0d ).



This isn't by any chance documented on the developer website of gcc somewhere?
It would be sad, if that knowledge is not publicy available for the future.


https://gcc.gnu.org/onlinedocs/gccint/Directives.html#Specify-how-to-build-the-test 
documents it.


And libgomp has: lib/libgomp.exp:set dg-do-what-default run

The all arguments vs. only -O2 is set in libgomp via:

libgomp.c++/c++.exp:    set DEFAULT_CFLAGS "-O2"

libgomp.c/c.exp:    set DEFAULT_CFLAGS "-O2"

and for libgomp.*fortran/fortran.exp, the difference between 'dg-do run' 
vs. default is *not* *documented,* but seems to be the result of the 
following:


# For Fortran we're doing torture testing, as Fortran has far more tests
# with arrays etc. that testing just -O0 or -O2 is insufficient, that is
# typically not the case for C/C++.
gfortran-dg-runtest $tests "" ""


Tobias

How to add an additional option to dg-compile and dg-run

[Thomas Koenig]

Hi,

for the fortran-unsigned branch, I would like to be able to run all
existing Fortran tests also with -funsigned, to make sure the option
does not break anything on existing code.

Question is: How?

I came as far as

$ make check-fortran RUNTESTFLAGS="--target_board=unix/-funsigned"

but that causes testsuite failures because C does not recognize
the option.

Any other possibilites?

Best regards

Thomas

Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]

[Jakub Jelinek]

On Mon, Jul 29, 2024 at 09:53:47AM +0200, Tobias Burnus wrote:
> Hi Andre, hi all,
> 
> Andre Vehreschild wrote:
> > yes, I could have looked harder 🙂
> 
> I wrote ;-) on purpose as this feature is somewhat hidden and writing 'dg-do
> compile' doesn't harm.

I think an explicit dg-do is better, otherwise one has to just guess
for some tests what has been actually intentional (see the recent
torture tests which were just compile time but written most likely to be
runtime; I've changed a few, Sam changed more).

Also, the subject line has too few digits in the PR number I think (9
missing?).

Otherwise LGTM.

Jakub

Re: How to add an additional option to dg-compile and dg-run

[Jonathan Wakely]

On Mon, 29 Jul 2024 at 09:20, Thomas Koenig via Gcc  wrote:
>
> Hi,
>
> for the fortran-unsigned branch, I would like to be able to run all
> existing Fortran tests also with -funsigned, to make sure the option
> does not break anything on existing code.
>
> Question is: How?
>
> I came as far as
>
> $ make check-fortran RUNTESTFLAGS="--target_board=unix/-funsigned"
>
> but that causes testsuite failures because C does not recognize
> the option.
>
> Any other possibilites?

I haven't tested it, but based on testsuite/gfortran.dg/dg.exp it
looks like you could add this in ~/.dejagnurc

set DEFAULT_FFLAGS "-funsigned -pedantic-errors"

(The -pedantic-errors is what dg.exp uses if the variable doesn't
already exist, so this preserves that.)

If you're using a site.exp you could add this there instead of ~/.dejagnurc

Re: How to add an additional option to dg-compile and dg-run

[Jonathan Wakely]

On Mon, 29 Jul 2024 at 10:20, Jonathan Wakely  wrote:
>
> On Mon, 29 Jul 2024 at 09:20, Thomas Koenig via Gcc  wrote:
> >
> > Hi,
> >
> > for the fortran-unsigned branch, I would like to be able to run all
> > existing Fortran tests also with -funsigned, to make sure the option
> > does not break anything on existing code.
> >
> > Question is: How?
> >
> > I came as far as
> >
> > $ make check-fortran RUNTESTFLAGS="--target_board=unix/-funsigned"
> >
> > but that causes testsuite failures because C does not recognize
> > the option.
> >
> > Any other possibilites?
>
> I haven't tested it, but based on testsuite/gfortran.dg/dg.exp it
> looks like you could add this in ~/.dejagnurc
>
> set DEFAULT_FFLAGS "-funsigned -pedantic-errors"
>
> (The -pedantic-errors is what dg.exp uses if the variable doesn't
> already exist, so this preserves that.)
>
> If you're using a site.exp you could add this there instead of ~/.dejagnurc

Oh but the comment suggests that will only be used for tests that
don't use dg-options:

# If a testcase doesn't have special options, use these.
global DEFAULT_FFLAGS
if ![info exists DEFAULT_FFLAGS] then {
set DEFAULT_FFLAGS " -pedantic-errors"
}

I don't know if that's correct, or if the dg-options flags get
appended to these default flags.

You might also need a leading space in the string, like dg.exp uses
(or maybe that's not needed in dg.exp anyway).

Re: How to add an additional option to dg-compile and dg-run

[Andrew Pinski]

On Mon, Jul 29, 2024 at 1:20 AM Thomas Koenig  wrote:
>
> Hi,
>
> for the fortran-unsigned branch, I would like to be able to run all
> existing Fortran tests also with -funsigned, to make sure the option
> does not break anything on existing code.
>
> Question is: How?
>
> I came as far as
>
> $ make check-fortran RUNTESTFLAGS="--target_board=unix/-funsigned"
>
> but that causes testsuite failures because C does not recognize
> the option.
>
> Any other possibilites?

Yes you could add it into the list of Torture options that
gfortran-dg-runtest uses.
Something like:
```
diff --git a/gcc/testsuite/lib/gfortran-dg.exp
b/gcc/testsuite/lib/gfortran-dg.exp
index fcba95dc396..0589d507382 100644
--- a/gcc/testsuite/lib/gfortran-dg.exp
+++ b/gcc/testsuite/lib/gfortran-dg.exp
@@ -153,6 +153,12 @@ proc gfortran-dg-runtest { testcases flags
default-extra-flags } {
} else {
set option_list [list { -O } ]
}
+   set old_option_list  $option_list
+   set option_list {}
+   foreach option $option_list {
+ lappend option_list $option
+ lappend option_list "$option -funsigned"
+   }

set nshort [file tail [file dirname $test]]/[file tail $test]
list-module-names $test
```
should work but I am not 100% I got the TCL syntax correct.

Thanks,
Andrew Pinski

>
> Best regards
>
> Thomas

Re: How to add an additional option to dg-compile and dg-run

[Thomas Koenig]

Hi Andrew,


Any other possibilites?

Yes you could add it into the list of Torture options that
gfortran-dg-runtest uses.
Something like:
```
diff --git a/gcc/testsuite/lib/gfortran-dg.exp
b/gcc/testsuite/lib/gfortran-dg.exp
index fcba95dc396..0589d507382 100644
--- a/gcc/testsuite/lib/gfortran-dg.exp
+++ b/gcc/testsuite/lib/gfortran-dg.exp
@@ -153,6 +153,12 @@ proc gfortran-dg-runtest { testcases flags
default-extra-flags } {
 } else {
 set option_list [list { -O } ]
 }
+   set old_option_list  $option_list
+   set option_list {}
+   foreach option $option_list {
+ lappend option_list $option
+ lappend option_list "$option -funsigned"
+   }

 set nshort [file tail [file dirname $test]]/[file tail $test]
 list-module-names $test
```
should work but I am not 100% I got the TCL syntax correct.


I tried this (well, a variant) but that also appended the -funsigned
option to the C compiler, where it again caused compilation errors.

Best regards

Thomas

Re: How to add an additional option to dg-compile and dg-run

[Thomas Schwinge]

Hi Thomas!

On 2024-07-29T10:18:49+0200, Thomas Koenig via Gcc  wrote:
> for the fortran-unsigned branch

By the way: I did see your recent announcement; wow -- Fortran finally
getting an UNSIGNED type!  :-)

> I would like to be able to run all
> existing Fortran tests also with -funsigned, to make sure the option
> does not break anything on existing code.
>
> Question is: How?
>
> I came as far as
>
> $ make check-fortran RUNTESTFLAGS="--target_board=unix/-funsigned"
>
> but that causes testsuite failures because C does not recognize
> the option.
>
> Any other possibilites?

Hard-code it to enabled in 'gcc/fortran/lang.opt'...  ;-)

Or: '-Wno-complain-wrong-lang' ought to help your case:

$ make check-fortran 
RUNTESTFLAGS="--target_board=unix/-funsigned/-Wno-complain-wrong-lang"

..., which I added for a very similar scenario, a while ago.  (See

"[PING, v2] Add '-Wno-complain-wrong-lang', and use it in 
'gcc/testsuite/lib/target-supports.exp:check_compile' and elsewhere".)


However, be prepared that the baseline:

$ make check-fortran 
RUNTESTFLAGS="--target_board=unix/-Wno-complain-wrong-lang"

... may have a number of "false FAILs".  For example (random),
'gfortran.dg/c-interop/allocate-errors.f90':

! { dg-additional-sources "allocate-errors-c.c dump-descriptors.c" }
! { dg-additional-options "-Wno-error -fcheck=all" }
! { dg-warning "command-line option '-fcheck=all' is valid for Fortran but 
not for C" "" { target *-*-* } 0 }

That 'dg-warning' will turn FAIL with '-Wno-complain-wrong-lang'.
Maybe there's a point to be made to clean all these up; replace such
'-Wno-error's and 'dg-warning's with '-Wno-complain-wrong-lang'?
Exception would be a few test cases that are meant to check
"command-line option '[...]' is valid for [...] but not for [...]"
diagnostic, which should thus get '-Wcomplain-wrong-lang' added via
'dg-additional-sources'.


Grüße
 Thomas