* Re: Support for NOINLINE attribute [not found] ` <CAFmAMQ2+f-JsDrXGw0y+nJCTHk=D2+k1PM9kSLz=7JiNeUwEoQ@mail.gmail.com> @ 2023-02-12 21:28 ` Harald Anlauf 2023-02-13 17:50 ` Harald Anlauf 0 siblings, 1 reply; 10+ messages in thread From: Harald Anlauf @ 2023-02-12 21:28 UTC (permalink / raw) To: Rimvydas Jasinskas; +Cc: fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for NOINLINE attribute 2023-02-12 21:28 ` Support for NOINLINE attribute Harald Anlauf @ 2023-02-13 17:50 ` Harald Anlauf 2023-02-13 17:50 ` Harald Anlauf 2023-02-14 9:35 ` nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) Thomas Schwinge 0 siblings, 2 replies; 10+ messages in thread From: Harald Anlauf @ 2023-02-13 17:50 UTC (permalink / raw) To: Rimvydas Jasinskas; +Cc: fortran, gcc-patches 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for NOINLINE attribute 2023-02-13 17:50 ` Harald Anlauf @ 2023-02-13 17:50 ` Harald Anlauf 2023-02-14 9:35 ` nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) Thomas Schwinge 1 sibling, 0 replies; 10+ messages in thread From: Harald Anlauf @ 2023-02-13 17:50 UTC (permalink / raw) To: gcc-patches; +Cc: fortran 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) 2023-02-13 17:50 ` Harald Anlauf 2023-02-13 17:50 ` Harald Anlauf @ 2023-02-14 9:35 ` Thomas Schwinge 2023-02-14 19:55 ` Harald Anlauf 1 sibling, 1 reply; 10+ messages in thread From: Thomas Schwinge @ 2023-02-14 9:35 UTC (permalink / raw) To: Harald Anlauf, Rimvydas Jasinskas, fortran, gcc-patches; +Cc: Tom de Vries [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) 2023-02-14 9:35 ` nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) Thomas Schwinge @ 2023-02-14 19:55 ` Harald Anlauf 2023-02-14 19:55 ` Harald Anlauf [not found] ` <CAFmAMQ04TtgTu1oZnzFSWw9pkgK0tRmt8ptpx1Xz4K0gjDCaag@mail.gmail.com> 0 siblings, 2 replies; 10+ messages in thread From: Harald Anlauf @ 2023-02-14 19:55 UTC (permalink / raw) To: Thomas Schwinge, Rimvydas Jasinskas, fortran, gcc-patches; +Cc: Tom de Vries 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) 2023-02-14 19:55 ` Harald Anlauf @ 2023-02-14 19:55 ` Harald Anlauf [not found] ` <CAFmAMQ04TtgTu1oZnzFSWw9pkgK0tRmt8ptpx1Xz4K0gjDCaag@mail.gmail.com> 1 sibling, 0 replies; 10+ messages in thread From: Harald Anlauf @ 2023-02-14 19:55 UTC (permalink / raw) To: gcc-patches; +Cc: fortran 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CAFmAMQ04TtgTu1oZnzFSWw9pkgK0tRmt8ptpx1Xz4K0gjDCaag@mail.gmail.com>]
[parent not found: <trinity-bb49b3d4-7455-4c7b-89d9-a9a857e68651-1676584221172@3c-app-gmx-bs44>]
[parent not found: <CAFmAMQ0bzmO1bmCy_5men10qmmv2K9G6jRgSk6XnY+HW-VVtLA@mail.gmail.com>]
[parent not found: <trinity-475912d3-f7e9-4ec9-b5c7-66d0cad9e63e-1677185631233@3c-app-gmx-bap67>]
* Re: Support for WEAK attribute, part 2 [not found] ` <trinity-475912d3-f7e9-4ec9-b5c7-66d0cad9e63e-1677185631233@3c-app-gmx-bap67> @ 2023-02-24 5:16 ` Rimvydas Jasinskas 2023-02-24 22:03 ` Harald Anlauf 2023-03-28 21:06 ` Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2) Thomas Schwinge 0 siblings, 2 replies; 10+ messages in thread From: Rimvydas Jasinskas @ 2023-02-24 5:16 UTC (permalink / raw) To: Harald Anlauf; +Cc: Rimvydas Jasinskas via Fortran, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for WEAK attribute, part 2 2023-02-24 5:16 ` Support for WEAK attribute, part 2 Rimvydas Jasinskas @ 2023-02-24 22:03 ` Harald Anlauf 2023-02-24 22:03 ` Harald Anlauf 2023-03-28 21:06 ` Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2) Thomas Schwinge 1 sibling, 1 reply; 10+ messages in thread From: Harald Anlauf @ 2023-02-24 22:03 UTC (permalink / raw) To: gcc-patches; +Cc: fortran 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Support for WEAK attribute, part 2 2023-02-24 22:03 ` Harald Anlauf @ 2023-02-24 22:03 ` Harald Anlauf 0 siblings, 0 replies; 10+ messages in thread From: Harald Anlauf @ 2023-02-24 22:03 UTC (permalink / raw) To: Rimvydas Jasinskas; +Cc: Rimvydas Jasinskas via Fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2) 2023-02-24 5:16 ` Support for WEAK attribute, part 2 Rimvydas Jasinskas 2023-02-24 22:03 ` Harald Anlauf @ 2023-03-28 21:06 ` Thomas Schwinge 1 sibling, 0 replies; 10+ messages in thread From: Thomas Schwinge @ 2023-03-28 21:06 UTC (permalink / raw) To: Rimvydas Jasinskas, fortran, gcc-patches; +Cc: Harald Anlauf, Tom de Vries [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-28 21:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAFmAMQ1KXtMfR=jPjKpBQfBp=RxFWNWZ23EhjMe=C8GjUBZBwA@mail.gmail.com> [not found] ` <trinity-4b3fd457-1be1-4c15-b277-628cc43074b2-1676063244052@3c-app-gmx-bs43> [not found] ` <CAFmAMQ08t9rKAC9w_x+Z2Dj=LJUok8DT2B02qR3U6hvtnpk7Ug@mail.gmail.com> [not found] ` <trinity-d9f0601c-cc39-43be-bae7-2b985656599c-1676150813891@3c-app-gmx-bap36> [not found] ` <CAFmAMQ2+f-JsDrXGw0y+nJCTHk=D2+k1PM9kSLz=7JiNeUwEoQ@mail.gmail.com> 2023-02-12 21:28 ` Support for NOINLINE attribute Harald Anlauf 2023-02-13 17:50 ` Harald Anlauf 2023-02-13 17:50 ` Harald Anlauf 2023-02-14 9:35 ` nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute) Thomas Schwinge 2023-02-14 19:55 ` Harald Anlauf 2023-02-14 19:55 ` Harald Anlauf [not found] ` <CAFmAMQ04TtgTu1oZnzFSWw9pkgK0tRmt8ptpx1Xz4K0gjDCaag@mail.gmail.com> [not found] ` <trinity-bb49b3d4-7455-4c7b-89d9-a9a857e68651-1676584221172@3c-app-gmx-bs44> [not found] ` <CAFmAMQ0bzmO1bmCy_5men10qmmv2K9G6jRgSk6XnY+HW-VVtLA@mail.gmail.com> [not found] ` <trinity-475912d3-f7e9-4ec9-b5c7-66d0cad9e63e-1677185631233@3c-app-gmx-bap67> 2023-02-24 5:16 ` Support for WEAK attribute, part 2 Rimvydas Jasinskas 2023-02-24 22:03 ` Harald Anlauf 2023-02-24 22:03 ` Harald Anlauf 2023-03-28 21:06 ` Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2) Thomas Schwinge
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).