Re: Support for NOINLINE attribute

Re: Support for NOINLINE attribute

[Harald Anlauf]

Hi Rimvydas,

> Gesendet: Sonntag, 12. Februar 2023 um 07:59 Uhr
> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
> An: "Harald Anlauf" <anlauf@gmx.de>
> Cc: "fortran" <fortran@gcc.gnu.org>
> Betreff: Re: Support for NOINLINE attribute
>
> On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
> > I am also not a native speaker, like many others contributing, but let
> > me quote the relevant orignal paragraph:
> >
> > "The @code{noreturn} keyword tells the compiler to assume that
> > @code{fatal} cannot return.  It can then optimize without regard to what
> > would happen if @code{fatal} ever did return.  This makes slightly
> > better code.  More importantly, it helps avoid spurious warnings of
> > uninitialized variables."
> >
> > My reading of this original paragraph differs very much from the
> > intention I get from the shortened version.  Would you please reread?
> >
> > > Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
> > > It is about marking dead conditional branches, so that the compiler
> > > can prove proper initialization (no -Wmaybe-uninitialized given).  It
> > > should behave the same as in C frontend.
> >
> > True.  And that's the whole point (IMHO), not silencing the compiler.
> Hmm both look the same to me, the silencing of false positive
> diagnostics is already implied by spurious.  To simplify I have
> changed it in v2 to just:
> "add a hint that a given function cannot return" documentation could
> be expanded later.
>
> > But shouldn't we rather follow what the C family of compilers in the
> > first place does for a particular target?  Most relevant libraries
> > for Fortran code are either C/C++ or Fortran anyway, including any
> > of the common MPI implementations, so should we care about Ada?
> I agree with you.  I have removed SUPPORTS_WEAK check and fixed
> indentation in v2.
>
> Regtested cleany on x86_64-pc-linux-gnu.
>
> Regards,
> Rimvydas

this version of the patch looks good to me, so it is basically OK
to commit.

There is one thing I cannot test, which is the handling of weak symbols
on other platforms.  A quick glance at the C testcases suggests that
someone with access to either an NVPTX or MingGW target might tell
whether that particular target should be excluded.  So I'd like to wait
for 24 hours for others to comment on this.

I see that you've signed-off your patch.  Do you have commit rights?
Otherwise I'll commit for you.  (I've CC'ed to gcc-patches@ for this
purpose.)

Thanks for the patch!

Harald

Re: Support for NOINLINE attribute

[Harald Anlauf]

Pushed as:

commit 086a1df4374962787db37c1f0d1bd9beb828f9e3

Thanks,
Harald

On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
> Hi Rimvydas,
>
>> Gesendet: Sonntag, 12. Februar 2023 um 07:59 Uhr
>> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
>> An: "Harald Anlauf" <anlauf@gmx.de>
>> Cc: "fortran" <fortran@gcc.gnu.org>
>> Betreff: Re: Support for NOINLINE attribute
>>
>> On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
>>> I am also not a native speaker, like many others contributing, but let
>>> me quote the relevant orignal paragraph:
>>>
>>> "The @code{noreturn} keyword tells the compiler to assume that
>>> @code{fatal} cannot return.  It can then optimize without regard to what
>>> would happen if @code{fatal} ever did return.  This makes slightly
>>> better code.  More importantly, it helps avoid spurious warnings of
>>> uninitialized variables."
>>>
>>> My reading of this original paragraph differs very much from the
>>> intention I get from the shortened version.  Would you please reread?
>>>
>>>> Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
>>>> It is about marking dead conditional branches, so that the compiler
>>>> can prove proper initialization (no -Wmaybe-uninitialized given).  It
>>>> should behave the same as in C frontend.
>>>
>>> True.  And that's the whole point (IMHO), not silencing the compiler.
>> Hmm both look the same to me, the silencing of false positive
>> diagnostics is already implied by spurious.  To simplify I have
>> changed it in v2 to just:
>> "add a hint that a given function cannot return" documentation could
>> be expanded later.
>>
>>> But shouldn't we rather follow what the C family of compilers in the
>>> first place does for a particular target?  Most relevant libraries
>>> for Fortran code are either C/C++ or Fortran anyway, including any
>>> of the common MPI implementations, so should we care about Ada?
>> I agree with you.  I have removed SUPPORTS_WEAK check and fixed
>> indentation in v2.
>>
>> Regtested cleany on x86_64-pc-linux-gnu.
>>
>> Regards,
>> Rimvydas
>
> this version of the patch looks good to me, so it is basically OK
> to commit.
>
> There is one thing I cannot test, which is the handling of weak symbols
> on other platforms.  A quick glance at the C testcases suggests that
> someone with access to either an NVPTX or MingGW target might tell
> whether that particular target should be excluded.  So I'd like to wait
> for 24 hours for others to comment on this.
>
> I see that you've signed-off your patch.  Do you have commit rights?
> Otherwise I'll commit for you.  (I've CC'ed to gcc-patches@ for this
> purpose.)
>
> Thanks for the patch!
>
> Harald
>
>
>

Re: Support for NOINLINE attribute

[Harald Anlauf]

Pushed as:

commit 086a1df4374962787db37c1f0d1bd9beb828f9e3

Thanks,
Harald

On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
> Hi Rimvydas,
> 
>> Gesendet: Sonntag, 12. Februar 2023 um 07:59 Uhr
>> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
>> An: "Harald Anlauf" <anlauf@gmx.de>
>> Cc: "fortran" <fortran@gcc.gnu.org>
>> Betreff: Re: Support for NOINLINE attribute
>>
>> On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
>>> I am also not a native speaker, like many others contributing, but let
>>> me quote the relevant orignal paragraph:
>>>
>>> "The @code{noreturn} keyword tells the compiler to assume that
>>> @code{fatal} cannot return.  It can then optimize without regard to what
>>> would happen if @code{fatal} ever did return.  This makes slightly
>>> better code.  More importantly, it helps avoid spurious warnings of
>>> uninitialized variables."
>>>
>>> My reading of this original paragraph differs very much from the
>>> intention I get from the shortened version.  Would you please reread?
>>>
>>>> Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
>>>> It is about marking dead conditional branches, so that the compiler
>>>> can prove proper initialization (no -Wmaybe-uninitialized given).  It
>>>> should behave the same as in C frontend.
>>>
>>> True.  And that's the whole point (IMHO), not silencing the compiler.
>> Hmm both look the same to me, the silencing of false positive
>> diagnostics is already implied by spurious.  To simplify I have
>> changed it in v2 to just:
>> "add a hint that a given function cannot return" documentation could
>> be expanded later.
>>
>>> But shouldn't we rather follow what the C family of compilers in the
>>> first place does for a particular target?  Most relevant libraries
>>> for Fortran code are either C/C++ or Fortran anyway, including any
>>> of the common MPI implementations, so should we care about Ada?
>> I agree with you.  I have removed SUPPORTS_WEAK check and fixed
>> indentation in v2.
>>
>> Regtested cleany on x86_64-pc-linux-gnu.
>>
>> Regards,
>> Rimvydas
> 
> this version of the patch looks good to me, so it is basically OK
> to commit.
> 
> There is one thing I cannot test, which is the handling of weak symbols
> on other platforms.  A quick glance at the C testcases suggests that
> someone with access to either an NVPTX or MingGW target might tell
> whether that particular target should be excluded.  So I'd like to wait
> for 24 hours for others to comment on this.
> 
> I see that you've signed-off your patch.  Do you have commit rights?
> Otherwise I'll commit for you.  (I've CC'ed to gcc-patches@ for this
> purpose.)
> 
> Thanks for the patch!
> 
> Harald
> 
> 
> 

nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

Hi!

On 2023-02-13T18:50:23+0100, Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> Pushed as:
>
> commit 086a1df4374962787db37c1f0d1bd9beb828f9e3

> On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
>> There is one thing I cannot test, which is the handling of weak symbols
>> on other platforms.  A quick glance at the C testcases suggests that
>> someone with access to either an NVPTX or MingGW target might tell
>> whether that particular target should be excluded.

Indeed nvptx does use a different assembler syntax; I've pushed to
master branch commit 8d8175869ca94c600e64e27b7676787b2a398f6e
"nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'", see
attached.


And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
declarations (which, for example, in the C world then evaluate to
zero-address unless actually defined)?  When I did a quick experiment,
that didn't seem to work?  (But may be my fault, of course.)

And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
suppose; test case?), or also for weak "data" in some way (which, for
example, in the C world then evaluates to a zero-address unless actually
defined)?

Could help to at least add a few more test cases, and clarify the
documentation?


Grüße
 Thomas


-----------------
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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-nvptx-Adjust-scan-assembler-in-gfortran.dg-weak-1.f9.patch --]
[-- Type: text/x-diff, Size: 1130 bytes --]

From 8d8175869ca94c600e64e27b7676787b2a398f6e Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 14 Feb 2023 10:11:19 +0100
Subject: [PATCH] nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'

Fix-up for recent commit 086a1df4374962787db37c1f0d1bd9beb828f9e3
"Fortran: Add !GCC$ attributes NOINLINE,NORETURN,WEAK".

	gcc/testsuite/
	* gfortran.dg/weak-1.f90: Adjust 'scan-assembler' for nvptx.
---
 gcc/testsuite/gfortran.dg/weak-1.f90 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gfortran.dg/weak-1.f90 b/gcc/testsuite/gfortran.dg/weak-1.f90
index d9aca686775a..9ec1fe74053e 100644
--- a/gcc/testsuite/gfortran.dg/weak-1.f90
+++ b/gcc/testsuite/gfortran.dg/weak-1.f90
@@ -1,6 +1,7 @@
 ! { dg-do compile }
 ! { dg-require-weak "" }
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func impl" 2 { target nvptx-*-* } } }
 subroutine impl
 !GCC$ ATTRIBUTES weak :: impl
 end subroutine
-- 
2.39.1

Re: nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)

[Harald Anlauf]

Hi Thomas,

On 2/14/23 10:35, Thomas Schwinge wrote:
> Hi!
>
> On 2023-02-13T18:50:23+0100, Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> Pushed as:
>>
>> commit 086a1df4374962787db37c1f0d1bd9beb828f9e3
>
>> On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
>>> There is one thing I cannot test, which is the handling of weak symbols
>>> on other platforms.  A quick glance at the C testcases suggests that
>>> someone with access to either an NVPTX or MingGW target might tell
>>> whether that particular target should be excluded.
>
> Indeed nvptx does use a different assembler syntax; I've pushed to
> master branch commit 8d8175869ca94c600e64e27b7676787b2a398f6e
> "nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'", see
> attached.

thanks for taking care of this.

> And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
> weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
> declarations (which, for example, in the C world then evaluate to
> zero-address unless actually defined)?  When I did a quick experiment,
> that didn't seem to work?  (But may be my fault, of course.)
>
> And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
> subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
> suppose; test case?), or also for weak "data" in some way (which, for
> example, in the C world then evaluates to a zero-address unless actually
> defined)?

It also works for functions, e.g.

integer function f ()
!GCC$ ATTRIBUTES weak :: f
   print *, "weak f"
   f = 0
end

Regarding symbols beyond procedures (subroutines, functions),
I had a look at what Crayftn supports.  Its manpage has:

```
WEAK

Syntax and use of the WEAK directive.
!DIR$ WEAK procedure_name[, procedure_name] ...
!DIR$ WEAK procedure_name= stub_name[, procedure_name1= stub_name1] ...

[...]

The WEAK directive supports the following arguments:

procedure_name
     A weak object in the form of a variable or procedure.
stub_name
     A stub procedure that exists in the code. The stub_name will be
called if a strong reference does not exist for procedure_name. The
stub_name procedure must have the same name and dummy argument list as
procedure_name.
```

However, testing e.g. with a module variable either gave an
error message or assembly that suggests that this does not work,
at least not with version cce/14.0.0.

> Could help to at least add a few more test cases, and clarify the
> documentation?

I'm not sure whether we need to support weak symbols other than
procedures in gfortran.  Maybe Rimvydas can comment on this.

We could clarify the documentation an reject e.g. variables
using:

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..75c04ad7ece 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,13 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
        && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
      set_decl_tls_model (decl, decl_default_tls_model (decl));

+  if ((sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+      && sym->attr.flavor != FL_PROCEDURE)
+    {
+      gfc_error ("Symbol %qs at %L has the WEAK attribute but is not a "
+                "procedure", sym->name, &sym->declared_at);
+    }
+
    gfc_finish_decl_attrs (decl, &sym->attr);
  }

This would reject code like

module m
   integer :: i, j
!GCC$ ATTRIBUTES weak :: j
end

weak-1.f90:18:17:

    18 |   integer :: i, j
       |                 1
Error: Symbol 'j' at (1) has the WEAK attribute but is not a procedure

Comments and thoughts?

Cheers,
Harald

>
> Grüße
>   Thomas
>
>
> -----------------
> 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

Re: nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)

[Harald Anlauf]

Hi Thomas,

On 2/14/23 10:35, Thomas Schwinge wrote:
> Hi!
> 
> On 2023-02-13T18:50:23+0100, Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> Pushed as:
>>
>> commit 086a1df4374962787db37c1f0d1bd9beb828f9e3
> 
>> On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
>>> There is one thing I cannot test, which is the handling of weak symbols
>>> on other platforms.  A quick glance at the C testcases suggests that
>>> someone with access to either an NVPTX or MingGW target might tell
>>> whether that particular target should be excluded.
> 
> Indeed nvptx does use a different assembler syntax; I've pushed to
> master branch commit 8d8175869ca94c600e64e27b7676787b2a398f6e
> "nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'", see
> attached.

thanks for taking care of this.

> And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
> weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
> declarations (which, for example, in the C world then evaluate to
> zero-address unless actually defined)?  When I did a quick experiment,
> that didn't seem to work?  (But may be my fault, of course.)
> 
> And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
> subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
> suppose; test case?), or also for weak "data" in some way (which, for
> example, in the C world then evaluates to a zero-address unless actually
> defined)?

It also works for functions, e.g.

integer function f ()
!GCC$ ATTRIBUTES weak :: f
   print *, "weak f"
   f = 0
end

Regarding symbols beyond procedures (subroutines, functions),
I had a look at what Crayftn supports.  Its manpage has:

```
WEAK

Syntax and use of the WEAK directive.
!DIR$ WEAK procedure_name[, procedure_name] ...
!DIR$ WEAK procedure_name= stub_name[, procedure_name1= stub_name1] ...

[...]

The WEAK directive supports the following arguments:

procedure_name
     A weak object in the form of a variable or procedure.
stub_name
     A stub procedure that exists in the code. The stub_name will be 
called if a strong reference does not exist for procedure_name. The 
stub_name procedure must have the same name and dummy argument list as 
procedure_name.
```

However, testing e.g. with a module variable either gave an
error message or assembly that suggests that this does not work,
at least not with version cce/14.0.0.

> Could help to at least add a few more test cases, and clarify the
> documentation?

I'm not sure whether we need to support weak symbols other than
procedures in gfortran.  Maybe Rimvydas can comment on this.

We could clarify the documentation an reject e.g. variables
using:

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..75c04ad7ece 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,13 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
        && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
      set_decl_tls_model (decl, decl_default_tls_model (decl));

+  if ((sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+      && sym->attr.flavor != FL_PROCEDURE)
+    {
+      gfc_error ("Symbol %qs at %L has the WEAK attribute but is not a "
+                "procedure", sym->name, &sym->declared_at);
+    }
+
    gfc_finish_decl_attrs (decl, &sym->attr);
  }

This would reject code like

module m
   integer :: i, j
!GCC$ ATTRIBUTES weak :: j
end

weak-1.f90:18:17:

    18 |   integer :: i, j
       |                 1
Error: Symbol 'j' at (1) has the WEAK attribute but is not a procedure

Comments and thoughts?

Cheers,
Harald

> 
> Grüße
>   Thomas
> 
> 
> -----------------
> 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

Re: Support for WEAK attribute, part 2

[Rimvydas Jasinskas]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf <anlauf@gmx.de> wrote:
> the patch is mostly fine, but there is a minor style issue:
>
> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
> +                  sym->name, &sym->declared_at, sym->attr.dummy
> +                  ? "dummy argument" : "local variable");
> +
>
> It is my understanding that this is not translation-friendly.
> Please use separate error texts for either case instead.
Interesting, I was under the impression this was fixed with OO-inlines
around the *.c rename.  In any case, adjusted in v2 to use:
+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    {
+      if (sym->attr.dummy)
+        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+               "dummy argument", sym->name, &sym->declared_at);
+      else
+        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+               "local variable", sym->name, &sym->declared_at);
+    }

> Do we need to really have that many separate files for all
> the tests?  Note that each separate file contributes to the
> time developers wait on regtesting to complete.  Some of the
> files essentially test only minor variations, like weak-2.f90
> and weak-3.f90.
These testcases are dg-compile and do not go through the "-O0 -O1 -O2
-O3 -Os" options like dg-run.  Combining the testcases does not reduce
gfortran.sum a lot:
-PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?impl
-PASS: gfortran.dg/weak-2.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-3.f90   -O   scan-assembler weak[^ \t]*[ \t]_?bar__
-PASS: gfortran.dg/weak-3.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-4.f90   -O   scan-assembler weak[^ \t]*[
\t]_?__foo_MOD_abc
-PASS: gfortran.dg/weak-4.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-5.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-6.f90   -O   (test for errors, line 3)
-PASS: gfortran.dg/weak-6.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-7.f90   -O   (test for errors, line 10)
-PASS: gfortran.dg/weak-7.f90   -O   (test for errors, line 6)
-PASS: gfortran.dg/weak-7.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-8.f90   -O   (test for errors, line 3)
-PASS: gfortran.dg/weak-8.f90   -O   (test for errors, line 7)
-PASS: gfortran.dg/weak-8.f90   -O  (test for excess errors)
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[
\t]_?__foo_MOD_abc
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?bar__
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?impl1
+PASS: gfortran.dg/weak-2.f90   -O  (test for excess errors)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 14)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 18)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 24)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 28)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 5)
+PASS: gfortran.dg/weak-3.f90   -O  (test for excess errors)

Only benefit is a bit less gfortran/f951 binaries invocations at
expense of potentially introducing issues in what was intended to be
tested.  There exists a partial(intentionally or not) sequential
file-scope namespace (like in C) how gfortran parses different units
in the same file.  Swapping unit order in the file can affect not only
code generation but diagnostic counts reported.  I tend to avoid
having more than one unit per source to avoid dealing with
"borrowing".  However with part3 now implemented after debugging, I
guess, samples could be combined to "accepts" + "rejects" two
testcases,  Done in v2.

> What is the purpose of testcase weak-5.f90?  It's valid
> Fortran, the common block /c/ shows in the assembler and
> does not interfere with the module variable c.
Removed.  Issue is not directly related to only the WEAK attributes.
Will be addressed in the future.

> Finally, please do not forget to CC patches to gcc-patches@
> so that others can see them.
Out of curiosity, what is the purpose of CC patches to gcc-patches
too?  Attachments are even available in web mailing list too, like in:
https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html

Regards,
Rimvydas

[-- Attachment #2: 0001-Fortran-Add-support-for-WEAK-attribute-for-variables.patch --]
[-- Type: text/x-patch, Size: 3923 bytes --]

From 5b83226c714b17780334b5bad9b17c2266af8232 Mon Sep 17 00:00:00 2001
From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
Date: Fri, 24 Feb 2023 04:41:00 +0000
Subject: Fortran: Add support for WEAK attribute for variables

 Add the rest of the weak-*.f90 testcases.

gcc/fortran/ChangeLog:

	* trans-decl.cc (gfc_finish_var_decl): Apply attribute.
	(generate_local_decl): Add diagnostic for dummy and local variables.

gcc/testsuite/ChangeLog:

	* gfortran.dg/weak-2.f90: New test.
	* gfortran.dg/weak-3.f90: New test.

Signed-off-by: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
---
 gcc/fortran/trans-decl.cc            | 14 +++++++++++++
 gcc/testsuite/gfortran.dg/weak-2.f90 | 26 ++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/weak-3.f90 | 30 ++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/weak-2.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-3.f90

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..474920966ec 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,10 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
       && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
     set_decl_tls_model (decl, decl_default_tls_model (decl));
 
+  /* Mark weak variables.  */
+  if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    declare_weak (decl);
+
   gfc_finish_decl_attrs (decl, &sym->attr);
 }
 
@@ -5885,6 +5889,16 @@ generate_local_decl (gfc_symbol * sym)
       if (!sym->attr.dummy && !sym->ns->proc_name->attr.entry_master)
 	generate_dependency_declarations (sym);
 
+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+	{
+	  if (sym->attr.dummy)
+	    gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+		       "dummy argument", sym->name, &sym->declared_at);
+	  else
+	    gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+		       "local variable", sym->name, &sym->declared_at);
+	}
+
       if (sym->attr.referenced)
 	gfc_get_symbol_decl (sym);
 
diff --git a/gcc/testsuite/gfortran.dg/weak-2.f90 b/gcc/testsuite/gfortran.dg/weak-2.f90
new file mode 100644
index 00000000000..3e0e877e903
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-2.f90
@@ -0,0 +1,26 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-skip-if "" { x86_64-*-mingw* } }
+! { dg-skip-if "" { nvptx-*-* } }
+
+! 1.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
+module foo
+implicit none
+!GCC$ ATTRIBUTES weak :: abc
+real :: abc(7)
+end module
+
+! 2.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" } }
+integer function impl1()
+implicit none
+!GCC$ ATTRIBUTES weak :: impl1
+end function
+
+! 3.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
+integer function impl2() bind(c,name='bar__')
+implicit none
+!GCC$ ATTRIBUTES weak :: impl2
+end function
diff --git a/gcc/testsuite/gfortran.dg/weak-3.f90 b/gcc/testsuite/gfortran.dg/weak-3.f90
new file mode 100644
index 00000000000..cfa845921ff
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-3.f90
@@ -0,0 +1,30 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+
+! 1.
+program foo1  ! { dg-error "weak declaration of 'foo1' must be public" "" }
+implicit none
+!GCC$ ATTRIBUTES weak :: foo1
+end program
+
+! 2.
+subroutine foo2
+implicit none
+contains
+ function dar() ! { dg-error "weak declaration of 'dar' must be public" "" }
+ integer :: dar
+!GCC$ ATTRIBUTES weak :: dar
+ end function
+ subroutine bar ! { dg-error "weak declaration of 'bar' must be public" "" }
+!GCC$ ATTRIBUTES weak :: bar
+ end subroutine
+end subroutine
+
+! 3.
+subroutine foo3(n) ! { dg-error "has the WEAK attribute but is a dummy argument" "" }
+implicit none
+integer :: n
+!GCC$ ATTRIBUTES weak :: n
+real :: abc       ! { dg-error "has the WEAK attribute but is a local variable" "" }
+!GCC$ ATTRIBUTES weak :: abc
+end subroutine
-- 
2.39.2

Re: Support for WEAK attribute, part 2

[Harald Anlauf]

Hi Rimvydas,

Am 24.02.23 um 06:16 schrieb Rimvydas Jasinskas via Gcc-patches:
> On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf <anlauf@gmx.de> wrote:
>> the patch is mostly fine, but there is a minor style issue:
>>
>> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
>> +       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
>> +                  sym->name, &sym->declared_at, sym->attr.dummy
>> +                  ? "dummy argument" : "local variable");
>> +
>>
>> It is my understanding that this is not translation-friendly.
>> Please use separate error texts for either case instead.
> Interesting, I was under the impression this was fixed with OO-inlines
> around the *.c rename.

if this is the case, I must have missed it.

 > In any case, adjusted in v2 to use:
> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +    {
> +      if (sym->attr.dummy)
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "dummy argument", sym->name, &sym->declared_at);
> +      else
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "local variable", sym->name, &sym->declared_at);
> +    }

This is ok.

> These testcases are dg-compile and do not go through the "-O0 -O1 -O2
> -O3 -Os" options like dg-run.  Combining the testcases does not reduce
> gfortran.sum a lot:

I wasn't thinking of gfortran.sum, it's about the total overhead of
the testsuite (dejagnu etc.).  But thanks for combining the tests!

>> Finally, please do not forget to CC patches to gcc-patches@
>> so that others can see them.
> Out of curiosity, what is the purpose of CC patches to gcc-patches
> too?  Attachments are even available in web mailing list too, like in:
> https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html

Well, patches should always go the gcc-patches@, see e.g.

https://gcc.gnu.org/gitwrite.html

On the other hand, many *Fortran* reviewers will ignore patches
there and look at them only when they are sent to fortran@.

Thanks for your patch, pushed as r13-6338-gbcbeebc498126c .

Harald

> Regards,
> Rimvydas

Re: Support for WEAK attribute, part 2

[Harald Anlauf]

Hi Rimvydas,

Am 24.02.23 um 06:16 schrieb Rimvydas Jasinskas via Gcc-patches:
> On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf <anlauf@gmx.de> wrote:
>> the patch is mostly fine, but there is a minor style issue:
>>
>> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
>> +       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
>> +                  sym->name, &sym->declared_at, sym->attr.dummy
>> +                  ? "dummy argument" : "local variable");
>> +
>>
>> It is my understanding that this is not translation-friendly.
>> Please use separate error texts for either case instead.
> Interesting, I was under the impression this was fixed with OO-inlines
> around the *.c rename.

if this is the case, I must have missed it.

 > In any case, adjusted in v2 to use:
> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +    {
> +      if (sym->attr.dummy)
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "dummy argument", sym->name, &sym->declared_at);
> +      else
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "local variable", sym->name, &sym->declared_at);
> +    }

This is ok.

> These testcases are dg-compile and do not go through the "-O0 -O1 -O2
> -O3 -Os" options like dg-run.  Combining the testcases does not reduce
> gfortran.sum a lot:

I wasn't thinking of gfortran.sum, it's about the total overhead of
the testsuite (dejagnu etc.).  But thanks for combining the tests!

>> Finally, please do not forget to CC patches to gcc-patches@
>> so that others can see them.
> Out of curiosity, what is the purpose of CC patches to gcc-patches
> too?  Attachments are even available in web mailing list too, like in:
> https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html

Well, patches should always go the gcc-patches@, see e.g.

https://gcc.gnu.org/gitwrite.html

On the other hand, many *Fortran* reviewers will ignore patches
there and look at them only when they are sent to fortran@.

Thanks for your patch, pushed as r13-6338-gbcbeebc498126c .

Harald

> Regards,
> Rimvydas

Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

Hi!

On 2023-02-24T07:16:51+0200, Rimvydas Jasinskas via Fortran <fortran@gcc.gnu.org> wrote:
> From 5b83226c714b17780334b5bad9b17c2266af8232 Mon Sep 17 00:00:00 2001
> From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
> Date: Fri, 24 Feb 2023 04:41:00 +0000
> Subject: Fortran: Add support for WEAK attribute for variables
>
>  Add the rest of the weak-*.f90 testcases.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/weak-2.f90
> @@ -0,0 +1,26 @@
> +! { dg-do compile }
> +! { dg-require-weak "" }
> +! { dg-skip-if "" { x86_64-*-mingw* } }
> +! { dg-skip-if "" { nvptx-*-* } }
> +[...]

Pushed to master branch commit b3c5933ee726004e4e47291d422dfe7ac3345062
"Enable 'gfortran.dg/weak-2.f90' for nvptx target", see attached.


I'm sorry I've not yet been able to look into the other items discussed
in this thread.


Grüße
 Thomas


-----------------
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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enable-gfortran.dg-weak-2.f90-for-nvptx-target.patch --]
[-- Type: text/x-diff, Size: 2037 bytes --]

From b3c5933ee726004e4e47291d422dfe7ac3345062 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 28 Mar 2023 22:26:30 +0200
Subject: [PATCH] Enable 'gfortran.dg/weak-2.f90' for nvptx target

Follow-up to commit bcbeebc498126c50d73809ec8a4bd0bff27ee97b
"Fortran: Add support for WEAK attribute for variables".

	gcc/testsuite/
	* gfortran.dg/weak-2.f90: Enable for nvptx target.
---
 gcc/testsuite/gfortran.dg/weak-2.f90 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gfortran.dg/weak-2.f90 b/gcc/testsuite/gfortran.dg/weak-2.f90
index 3e0e877e903..ab273a13b6c 100644
--- a/gcc/testsuite/gfortran.dg/weak-2.f90
+++ b/gcc/testsuite/gfortran.dg/weak-2.f90
@@ -1,10 +1,10 @@
 ! { dg-do compile }
 ! { dg-require-weak "" }
 ! { dg-skip-if "" { x86_64-*-mingw* } }
-! { dg-skip-if "" { nvptx-*-* } }
 
 ! 1.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.global \\.align 4 \\.u32 __foo_MOD_abc" 1 { target nvptx-*-* } } }
 module foo
 implicit none
 !GCC$ ATTRIBUTES weak :: abc
@@ -12,14 +12,16 @@ real :: abc(7)
 end module
 
 ! 2.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func \\(\\.param\\.u32 %value_out\\) impl1" 2 { target nvptx-*-* } } }
 integer function impl1()
 implicit none
 !GCC$ ATTRIBUTES weak :: impl1
 end function
 
 ! 3.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func \\(\\.param\\.u32 %value_out\\) bar__" 2 { target nvptx-*-* } } }
 integer function impl2() bind(c,name='bar__')
 implicit none
 !GCC$ ATTRIBUTES weak :: impl2
-- 
2.25.1