nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)
Checks
Commit Message
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
Comments
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
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(-)
@@ -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