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

  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
  

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
  

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
  

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