Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
Hi Harald,
thank you very much for ok'ing this large patch. Merged as
gcc-15-1965-ge4f2f46e015
Looking forward to get (no) bug reports ;-)
Thanks again,
Andre
On Wed, 10 Jul 2024 20:52:37 +0200
Harald Anlauf wrote:
> Hi Andre,
>
> Am 10.07.24 um 10:45 schrieb Andre Vehreschild:
> > Hi Harald,
> >
> > thanks for the review. I totally agree, that this patch has gotten
> > bigger than I expected (and wanted). But things are as they are.
> >
> > About the coding style: I have worked in so many projects, that I
> > consider a consistent coding style luxury. I esp. do not have my
> > own one anymore. The formating you are seeing in my patches is the
> > result of clang-format with the provided parameter file in
> > contrib/clang-format. I was happy to have a tool to do the
> > formatting, that I could integrate into my IDE, because previously
> > it was hard to mimic the GNU style. I try to get to the GNU style
> > as good as possible, where I consider clang-format doing garbage.
> >
> > I see that clang-format has a "very specific opinion" on how to
> > format the lines you mentioned, but it will "correct" them any time
> > I change them and touch them later. I now have forbidden
> > clang-format to touch the code lines, but this means to add
> > formatter specific comments. Is this ok?
>
> yes, this is much better now! Thanks.
>
> (I entirely rely on Emacs' formatting when working with C. Sometimes
> the indentation at first may appear unexpected, but in most of these
> cases I find that it helps to just use explicit parentheses to
> convince Emacs. This is documented.)
>
> > About the assumed size arrays, that was a small change and is added
> > now.
>
> Great!
>
> > Note, the runtime part of the patch (pr96992_3p1.patch) did not
> > change and is therefore not updated.
> >
> > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for mainline?
>
> Yes, this is OK now.
>
> Thanks for the patch and your patience ;-)
>
> Harald
>
>
> > Regards,
> > Andre
> >
> > On Fri, 5 Jul 2024 22:10:16 +0200
> > Harald Anlauf wrote:
> >
> >> Hi Andre,
> >>
> >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild:
> >>> Hi Harald,
> >>>
> >>> I am sorry for the long delay, but fixing the negative stride
> >>> lead from one issue to the next. I finally got a version that
> >>> does not regress. Please have a look.
> >>>
> >>> This patch has two parts:
> >>> 1. The runtime library part in pr96992_3p1.patch and
> >>> 2. the compiler changes in pr96992_3p2.patch.
> >>>
> >>> In my branch also the two patches from Paul for pr59104 and
> >>> pr102689 are living, which might lead to small shifts during
> >>> application of the patches.
> >>>
> >>> NOTE, this patch adds internal packing and unpacking of class
> >>> arrays similar to the regular pack and unpack. I think this is
> >>> necessary, because the regular un-/pack does not use the vptr's
> >>> _copy routine for moving data and therefore may produce bugs.
> >>>
> >>> The un-/pack_class routines are yet only used for converting a
> >>> derived type array to a class array. Extending their use when a
> >>> UN-/PACK() is applied on a class array is still to be done (as
> >>> part of another PR).
> >>>
> >>> Regtests fine on x86_64-pc-linux-gnu/ Fedora 39.
> >>
> >> this is a really huge patch to review, and I am not sure that I
> >> can do this without help from others. Paul? Anybody else?
> >>
> >> As far as I can tell for now:
> >>
> >> - pr96992_3p1.patch (the libgfortran part) looks good to me.
> >>
> >> - git had some whitespace issues with pr96992_3p2.patch as
> >> attached, but I could fix that locally and do some testing
> >> parallel to reading.
> >>
> >> A few advance comments on the latter patch:
> >>
> >> - my understanding is that the PR at the end of a summary line
> >> should be like in:
> >>
> >> Fortran: Fix rejecting class arrays of different ranks as storage
> >> association argument [PR96992]
> >>
> >> I was told that this helps people explicitly scanning for the
> >> PR number in that place.
> >>
> >> - some rewrites of logical conditions change the coding style from
> >> what it recommended GNU coding style, and I find the more
> >> compact way used in some places harder to grok (but that may be
> >> just me). Example:
> >>
> >> @@ -8850,20 +8857,24 @@ gfc_conv_array_parameter (gfc_se * se,
> >> gfc_expr
> >> * expr, bool g77,
> >> /* There is no need to pack and unpack the array, if it is
> >> contiguous and not a deferred- or assumed-shape array, or if it is
> >> simply contiguous. */
> >> - no_pack = ((sym && sym->as
> >> -&& !sym->attr.pointer
> >> -&& sym->as->type != AS_DEFERRED
> >> -&& sym->as->type != AS_ASSUMED_RANK
> >> -&& sym->as->type != AS_ASSUMED_SHAPE)
> >> -||
> >> - (ref && ref->u.ar.as
> >> -&& ref->u.ar.as->type != AS_DEFERRED
> >> + no_pack = false;
> >> + gfc_array_spec *as;
> >> + if (sym)
> >> +{
> >> + symbol_attribute
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild wrote: > > Hi Harald, > > thank you very much for ok'ing this large patch. Merged as > gcc-15-1965-ge4f2f46e015 > > Looking forward to get (no) bug reports ;-) This seems to break bootstrap with ../../gcc/gcc/fortran/trans-array.cc: In function ‘void gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const gfc_symbol*, const char*, tree_node**, tree_node**, tree_node**)’: ../../gcc/gcc/fortran/trans-array.cc:9135:41: error: ‘pack_attr’ may be used uninitialized [-Werror=maybe-uninitialized] 9135 | tmp = build_call_expr_loc (input_location, | ^~~~ 9136 | gfor_fndecl_in_unpack_class, 4, tmp, | 9137 | packedptr, | ~~ 9138 | size_in_bytes (TREE_TYPE (ctree)), | ~~ 9139 | pack_attr); | ~~ ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ was declared here 8665 | tree pack_attr; |^ cc1plus: all warnings being treated as errors make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > Thanks again, > > Andre > > On Wed, 10 Jul 2024 20:52:37 +0200 > Harald Anlauf wrote: > > > Hi Andre, > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > Hi Harald, > > > > > > thanks for the review. I totally agree, that this patch has gotten > > > bigger than I expected (and wanted). But things are as they are. > > > > > > About the coding style: I have worked in so many projects, that I > > > consider a consistent coding style luxury. I esp. do not have my > > > own one anymore. The formating you are seeing in my patches is the > > > result of clang-format with the provided parameter file in > > > contrib/clang-format. I was happy to have a tool to do the > > > formatting, that I could integrate into my IDE, because previously > > > it was hard to mimic the GNU style. I try to get to the GNU style > > > as good as possible, where I consider clang-format doing garbage. > > > > > > I see that clang-format has a "very specific opinion" on how to > > > format the lines you mentioned, but it will "correct" them any time > > > I change them and touch them later. I now have forbidden > > > clang-format to touch the code lines, but this means to add > > > formatter specific comments. Is this ok? > > > > yes, this is much better now! Thanks. > > > > (I entirely rely on Emacs' formatting when working with C. Sometimes > > the indentation at first may appear unexpected, but in most of these > > cases I find that it helps to just use explicit parentheses to > > convince Emacs. This is documented.) > > > > > About the assumed size arrays, that was a small change and is added > > > now. > > > > Great! > > > > > Note, the runtime part of the patch (pr96992_3p1.patch) did not > > > change and is therefore not updated. > > > > > > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for mainline? > > > > Yes, this is OK now. > > > > Thanks for the patch and your patience ;-) > > > > Harald > > > > > > > Regards, > > > Andre > > > > > > On Fri, 5 Jul 2024 22:10:16 +0200 > > > Harald Anlauf wrote: > > > > > >> Hi Andre, > > >> > > >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild: > > >>> Hi Harald, > > >>> > > >>> I am sorry for the long delay, but fixing the negative stride > > >>> lead from one issue to the next. I finally got a version that > > >>> does not regress. Please have a look. > > >>> > > >>> This patch has two parts: > > >>> 1. The runtime library part in pr96992_3p1.patch and > > >>> 2. the compiler changes in pr96992_3p2.patch. > > >>> > > >>> In my branch also the two patches from Paul for pr59104 and > > >>> pr102689 are living, which might lead to small shifts during > > >>> application of the patches. > > >>> > > >>> NOTE, this patch adds internal packing and unpacking of class > > >>> arrays similar to the regular pack and unpack. I think this is > > >>> necessary, because the regular un-/pack does not use the vptr's > > >>> _copy routine for moving data and therefore may produce bugs. > > >>> > > >>> The un-/pack_class routines are yet only used for converting a > > >>> derived type array to a class array. Extending their use when a > > >>> UN-/PACK() is applied on a class array is still to be done (as > > >>> part of another PR). > > >>> > > >>> Regtests fine on x86_64-pc-linux-gnu/ Fedora 39. > > >> > > >> this is a really huge patch to review, and I am not sure that I > > >> can do this without help from others. Paul? Anybody else? > > >> > > >> As far as I can tell for now: > > >> > > >> - pr96992_3p1.patch (the libgfortran part) looks good to me. > > >> > > >> - git had some whitespace issues with pr96992_3p2.patch as > > >>
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
On Thu, Jul 11, 2024 at 10:54 AM Richard Biener wrote: > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild wrote: > > > > Hi Harald, > > > > thank you very much for ok'ing this large patch. Merged as > > gcc-15-1965-ge4f2f46e015 > > > > Looking forward to get (no) bug reports ;-) > > This seems to break bootstrap with > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const gfc_symbol*, > const char*, tree_node**, tree_node**, tree_node**)’: > ../../gcc/gcc/fortran/trans-array.cc:9135:41: error: ‘pack_attr’ may > be used uninitialized [-Werror=maybe-uninitialized] > 9135 | tmp = build_call_expr_loc (input_location, > | ^~~~ > 9136 | > gfor_fndecl_in_unpack_class, 4, tmp, > | > > 9137 | packedptr, > | ~~ > 9138 | size_in_bytes > (TREE_TYPE (ctree)), > | > ~~ > 9139 | pack_attr); > | ~~ > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ was declared > here > 8665 | tree pack_attr; > |^ > cc1plus: all warnings being treated as errors > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 It seems to be a false positive but GCCs little mind is too weak to prove that (yes, we error on the side of emitting a diagnostic if we can't prove it's initialized) Richard. > > > Thanks again, > > > > Andre > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > Harald Anlauf wrote: > > > > > Hi Andre, > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > Hi Harald, > > > > > > > > thanks for the review. I totally agree, that this patch has gotten > > > > bigger than I expected (and wanted). But things are as they are. > > > > > > > > About the coding style: I have worked in so many projects, that I > > > > consider a consistent coding style luxury. I esp. do not have my > > > > own one anymore. The formating you are seeing in my patches is the > > > > result of clang-format with the provided parameter file in > > > > contrib/clang-format. I was happy to have a tool to do the > > > > formatting, that I could integrate into my IDE, because previously > > > > it was hard to mimic the GNU style. I try to get to the GNU style > > > > as good as possible, where I consider clang-format doing garbage. > > > > > > > > I see that clang-format has a "very specific opinion" on how to > > > > format the lines you mentioned, but it will "correct" them any time > > > > I change them and touch them later. I now have forbidden > > > > clang-format to touch the code lines, but this means to add > > > > formatter specific comments. Is this ok? > > > > > > yes, this is much better now! Thanks. > > > > > > (I entirely rely on Emacs' formatting when working with C. Sometimes > > > the indentation at first may appear unexpected, but in most of these > > > cases I find that it helps to just use explicit parentheses to > > > convince Emacs. This is documented.) > > > > > > > About the assumed size arrays, that was a small change and is added > > > > now. > > > > > > Great! > > > > > > > Note, the runtime part of the patch (pr96992_3p1.patch) did not > > > > change and is therefore not updated. > > > > > > > > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for mainline? > > > > > > Yes, this is OK now. > > > > > > Thanks for the patch and your patience ;-) > > > > > > Harald > > > > > > > > > > Regards, > > > > Andre > > > > > > > > On Fri, 5 Jul 2024 22:10:16 +0200 > > > > Harald Anlauf wrote: > > > > > > > >> Hi Andre, > > > >> > > > >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild: > > > >>> Hi Harald, > > > >>> > > > >>> I am sorry for the long delay, but fixing the negative stride > > > >>> lead from one issue to the next. I finally got a version that > > > >>> does not regress. Please have a look. > > > >>> > > > >>> This patch has two parts: > > > >>> 1. The runtime library part in pr96992_3p1.patch and > > > >>> 2. the compiler changes in pr96992_3p2.patch. > > > >>> > > > >>> In my branch also the two patches from Paul for pr59104 and > > > >>> pr102689 are living, which might lead to small shifts during > > > >>> application of the patches. > > > >>> > > > >>> NOTE, this patch adds internal packing and unpacking of class > > > >>> arrays similar to the regular pack and unpack. I think this is > > > >>> necessary, because the regular un-/pack does not use the vptr's > > > >>> _copy routine for moving data and therefore may produce bugs. > > > >>> > > > >>> The un-/pack_class routines are yet only used for converting a > > > >>> derived type array to a class array. Extending their use when a > > > >>> UN-/PACK() is applied o
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
On Thu, Jul 11, 2024 at 11:04 AM Andre Vehreschild wrote: > > Hi Richard, > > I am sorry to hear that. Shall I revert? I would suggest to instead fix by initializing the variable with NULL (and a comment). > - Andre > On Thu, 11 Jul 2024 10:57:48 +0200 > Richard Biener wrote: > > > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener > > wrote: > > > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild > > > wrote: > > > > > > > > Hi Harald, > > > > > > > > thank you very much for ok'ing this large patch. Merged as > > > > gcc-15-1965-ge4f2f46e015 > > > > > > > > Looking forward to get (no) bug reports ;-) > > > > > > This seems to break bootstrap with > > > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const > > > gfc_symbol*, const char*, tree_node**, tree_node**, tree_node**)’: > > > ../../gcc/gcc/fortran/trans-array.cc:9135:41: error: ‘pack_attr’ may > > > be used uninitialized [-Werror=maybe-uninitialized] > > > 9135 | tmp = build_call_expr_loc (input_location, > > > | ^~~~ > > > 9136 | > > > gfor_fndecl_in_unpack_class, 4, tmp, > > > | > > > > > > 9137 | packedptr, > > > | ~~ > > > 9138 | size_in_bytes > > > (TREE_TYPE (ctree)), > > > | > > > ~~ > > > 9139 | pack_attr); > > > | ~~ > > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ was > > > declared here 8665 | tree pack_attr; > > > |^ > > > cc1plus: all warnings being treated as errors > > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > > > > It seems to be a false positive but GCCs little mind is too weak to > > prove that (yes, we error on the side of emitting a diagnostic if we > > can't prove it's initialized) > > > > Richard. > > > > > > > > > Thanks again, > > > > > > > > Andre > > > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > > > Harald Anlauf wrote: > > > > > > > > > Hi Andre, > > > > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > > > Hi Harald, > > > > > > > > > > > > thanks for the review. I totally agree, that this patch has > > > > > > gotten bigger than I expected (and wanted). But things are as > > > > > > they are. > > > > > > > > > > > > About the coding style: I have worked in so many projects, > > > > > > that I consider a consistent coding style luxury. I esp. do > > > > > > not have my own one anymore. The formating you are seeing in > > > > > > my patches is the result of clang-format with the provided > > > > > > parameter file in contrib/clang-format. I was happy to have a > > > > > > tool to do the formatting, that I could integrate into my > > > > > > IDE, because previously it was hard to mimic the GNU style. I > > > > > > try to get to the GNU style as good as possible, where I > > > > > > consider clang-format doing garbage. > > > > > > > > > > > > I see that clang-format has a "very specific opinion" on how > > > > > > to format the lines you mentioned, but it will "correct" them > > > > > > any time I change them and touch them later. I now have > > > > > > forbidden clang-format to touch the code lines, but this > > > > > > means to add formatter specific comments. Is this ok? > > > > > > > > > > yes, this is much better now! Thanks. > > > > > > > > > > (I entirely rely on Emacs' formatting when working with C. > > > > > Sometimes the indentation at first may appear unexpected, but > > > > > in most of these cases I find that it helps to just use > > > > > explicit parentheses to convince Emacs. This is documented.) > > > > > > > > > > > About the assumed size arrays, that was a small change and is > > > > > > added now. > > > > > > > > > > Great! > > > > > > > > > > > Note, the runtime part of the patch (pr96992_3p1.patch) did > > > > > > not change and is therefore not updated. > > > > > > > > > > > > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for > > > > > > mainline? > > > > > > > > > > Yes, this is OK now. > > > > > > > > > > Thanks for the patch and your patience ;-) > > > > > > > > > > Harald > > > > > > > > > > > > > > > > Regards, > > > > > > Andre > > > > > > > > > > > > On Fri, 5 Jul 2024 22:10:16 +0200 > > > > > > Harald Anlauf wrote: > > > > > > > > > > > >> Hi Andre, > > > > > >> > > > > > >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild: > > > > > >>> Hi Harald, > > > > > >>> > > > > > >>> I am sorry for the long delay, but fixing the negative > > > > > >>> stride lead from one issue to the next. I finally got a > > > > > >>> version that does not regress. Please have a look. > > > > > >>> > > > > > >>> This patch has two parts: > > > > > >>> 1. T
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
Hi Richard, I am sorry to hear that. Shall I revert? - Andre On Thu, 11 Jul 2024 10:57:48 +0200 Richard Biener wrote: > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener > wrote: > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild > > wrote: > > > > > > Hi Harald, > > > > > > thank you very much for ok'ing this large patch. Merged as > > > gcc-15-1965-ge4f2f46e015 > > > > > > Looking forward to get (no) bug reports ;-) > > > > This seems to break bootstrap with > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const > > gfc_symbol*, const char*, tree_node**, tree_node**, tree_node**)’: > > ../../gcc/gcc/fortran/trans-array.cc:9135:41: error: ‘pack_attr’ may > > be used uninitialized [-Werror=maybe-uninitialized] > > 9135 | tmp = build_call_expr_loc (input_location, > > | ^~~~ > > 9136 | > > gfor_fndecl_in_unpack_class, 4, tmp, > > | > > > > 9137 | packedptr, > > | ~~ > > 9138 | size_in_bytes > > (TREE_TYPE (ctree)), > > | > > ~~ > > 9139 | pack_attr); > > | ~~ > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ was > > declared here 8665 | tree pack_attr; > > |^ > > cc1plus: all warnings being treated as errors > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > > It seems to be a false positive but GCCs little mind is too weak to > prove that (yes, we error on the side of emitting a diagnostic if we > can't prove it's initialized) > > Richard. > > > > > > Thanks again, > > > > > > Andre > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > > Harald Anlauf wrote: > > > > > > > Hi Andre, > > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > > Hi Harald, > > > > > > > > > > thanks for the review. I totally agree, that this patch has > > > > > gotten bigger than I expected (and wanted). But things are as > > > > > they are. > > > > > > > > > > About the coding style: I have worked in so many projects, > > > > > that I consider a consistent coding style luxury. I esp. do > > > > > not have my own one anymore. The formating you are seeing in > > > > > my patches is the result of clang-format with the provided > > > > > parameter file in contrib/clang-format. I was happy to have a > > > > > tool to do the formatting, that I could integrate into my > > > > > IDE, because previously it was hard to mimic the GNU style. I > > > > > try to get to the GNU style as good as possible, where I > > > > > consider clang-format doing garbage. > > > > > > > > > > I see that clang-format has a "very specific opinion" on how > > > > > to format the lines you mentioned, but it will "correct" them > > > > > any time I change them and touch them later. I now have > > > > > forbidden clang-format to touch the code lines, but this > > > > > means to add formatter specific comments. Is this ok? > > > > > > > > yes, this is much better now! Thanks. > > > > > > > > (I entirely rely on Emacs' formatting when working with C. > > > > Sometimes the indentation at first may appear unexpected, but > > > > in most of these cases I find that it helps to just use > > > > explicit parentheses to convince Emacs. This is documented.) > > > > > > > > > About the assumed size arrays, that was a small change and is > > > > > added now. > > > > > > > > Great! > > > > > > > > > Note, the runtime part of the patch (pr96992_3p1.patch) did > > > > > not change and is therefore not updated. > > > > > > > > > > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for > > > > > mainline? > > > > > > > > Yes, this is OK now. > > > > > > > > Thanks for the patch and your patience ;-) > > > > > > > > Harald > > > > > > > > > > > > > Regards, > > > > > Andre > > > > > > > > > > On Fri, 5 Jul 2024 22:10:16 +0200 > > > > > Harald Anlauf wrote: > > > > > > > > > >> Hi Andre, > > > > >> > > > > >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild: > > > > >>> Hi Harald, > > > > >>> > > > > >>> I am sorry for the long delay, but fixing the negative > > > > >>> stride lead from one issue to the next. I finally got a > > > > >>> version that does not regress. Please have a look. > > > > >>> > > > > >>> This patch has two parts: > > > > >>> 1. The runtime library part in pr96992_3p1.patch and > > > > >>> 2. the compiler changes in pr96992_3p2.patch. > > > > >>> > > > > >>> In my branch also the two patches from Paul for pr59104 and > > > > >>> pr102689 are living, which might lead to small shifts during > > > > >>> application of the patches. > > > > >>> > > > > >>> NOTE, this patch adds internal packing and u
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
Hi Richard, would that be sufficient? Bootstrap is still running for me... From c30c2cf829a094ba5e4c2c31333bed6e8c0d32af Mon Sep 17 00:00:00 2001 From: Andre Vehreschild Date: Thu, 11 Jul 2024 11:21:04 +0200 Subject: [PATCH] [Fortran] Fix bootstrap broken by gcc-15-1965-ge4f2f46e015 gcc/fortran/ChangeLog: * trans-array.cc (gfc_conv_array_parameter): Init variable to NULL_TREE to fix bootstrap. --- gcc/fortran/trans-array.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc index 0fffa07495c..5558ab69969 100644 --- a/gcc/fortran/trans-array.cc +++ b/gcc/fortran/trans-array.cc @@ -8750,7 +8750,7 @@ gfc_conv_array_parameter (gfc_se *se, gfc_expr *expr, bool g77, tree stmt; tree parent = DECL_CONTEXT (current_function_decl); tree ctree; - tree pack_attr; + tree pack_attr = NULL_TREE; /* Set when packing class arrays. */ bool full_array_var; bool this_array_result; bool contiguous; -- 2.45.2 Sorry for the breakage. Regards, Andre On Thu, 11 Jul 2024 11:06:47 +0200 Richard Biener wrote: > On Thu, Jul 11, 2024 at 11:04 AM Andre Vehreschild > wrote: > > > > Hi Richard, > > > > I am sorry to hear that. Shall I revert? > > I would suggest to instead fix by initializing the variable with NULL > (and a comment). > > > - Andre > > On Thu, 11 Jul 2024 10:57:48 +0200 > > Richard Biener wrote: > > > > > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener > > > wrote: > > > > > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild > > > > wrote: > > > > > > > > > > Hi Harald, > > > > > > > > > > thank you very much for ok'ing this large patch. Merged as > > > > > gcc-15-1965-ge4f2f46e015 > > > > > > > > > > Looking forward to get (no) bug reports ;-) > > > > > > > > This seems to break bootstrap with > > > > > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > > > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const > > > > gfc_symbol*, const char*, tree_node**, tree_node**, > > > > tree_node**)’: ../../gcc/gcc/fortran/trans-array.cc:9135:41: > > > > error: ‘pack_attr’ may be used uninitialized > > > > [-Werror=maybe-uninitialized] 9135 | tmp = > > > > build_call_expr_loc (input_location, | > > > > ^~~~ 9136 | > > > > gfor_fndecl_in_unpack_class, 4, tmp, > > > > | > > > > > > > > 9137 | packedptr, > > > > | ~~ > > > > 9138 | size_in_bytes > > > > (TREE_TYPE (ctree)), > > > > | > > > > ~~ > > > > 9139 | pack_attr); > > > > | ~~ > > > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ > > > > was declared here 8665 | tree pack_attr; > > > > |^ > > > > cc1plus: all warnings being treated as errors > > > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > > > > > > It seems to be a false positive but GCCs little mind is too weak > > > to prove that (yes, we error on the side of emitting a diagnostic > > > if we can't prove it's initialized) > > > > > > Richard. > > > > > > > > > > > > Thanks again, > > > > > > > > > > Andre > > > > > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > > > > Harald Anlauf wrote: > > > > > > > > > > > Hi Andre, > > > > > > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > > > > Hi Harald, > > > > > > > > > > > > > > thanks for the review. I totally agree, that this patch > > > > > > > has gotten bigger than I expected (and wanted). But > > > > > > > things are as they are. > > > > > > > > > > > > > > About the coding style: I have worked in so many projects, > > > > > > > that I consider a consistent coding style luxury. I esp. > > > > > > > do not have my own one anymore. The formating you are > > > > > > > seeing in my patches is the result of clang-format with > > > > > > > the provided parameter file in contrib/clang-format. I > > > > > > > was happy to have a tool to do the formatting, that I > > > > > > > could integrate into my IDE, because previously it was > > > > > > > hard to mimic the GNU style. I try to get to the GNU > > > > > > > style as good as possible, where I consider clang-format > > > > > > > doing garbage. > > > > > > > > > > > > > > I see that clang-format has a "very specific opinion" on > > > > > > > how to format the lines you mentioned, but it will > > > > > > > "correct" them any time I change them and touch them > > > > > > > later. I now have forbidden clang-format to touch the > > > > > > > code lines, but this means to add formatter specific > > > > > > > comments. Is this ok? > > > > > > > > > > > > yes, this is much better now! Thanks. > > > > > > > > > > > > (I en
RE: Lower zeroing array assignment to memset for allocatable arrays
> -Original Message-
> From: Harald Anlauf
> Sent: Thursday, July 11, 2024 12:53 AM
> To: Prathamesh Kulkarni ; gcc-
> patc...@gcc.gnu.org; fortran@gcc.gnu.org
> Subject: Re: Lower zeroing array assignment to memset for allocatable
> arrays
>
> External email: Use caution opening links or attachments
>
>
> Hi Prathamesh,
>
> Am 10.07.24 um 13:22 schrieb Prathamesh Kulkarni:
> > Hi,
> > The attached patch lowers zeroing array assignment to memset for
> allocatable arrays.
> >
> > For example:
> > subroutine test(z, n)
> > implicit none
> > integer :: n
> > real(4), allocatable :: z(:,:,:)
> >
> > allocate(z(n, 8192, 2048))
> > z = 0
> > end subroutine
> >
> > results in following call to memset instead of 3 nested loops for z
> = 0:
> > (void) __builtin_memset ((void *) z->data, 0, (unsigned long)
> > MAX_EXPR dim[0].ubound - z->dim[0].lbound, -1> + 1) *
> > (MAX_EXPR dim[1].ubound - z->dim[1].lbound, -1> + 1)) *
> (MAX_EXPR
> > dim[2].ubound - z->dim[2].lbound, -1> + 1)) * 4));
> >
> > The patch significantly improves speedup for an internal Fortran
> application on AArch64 -mcpu=grace (and potentially on other AArch64
> cores too).
> > Bootstrapped+tested on aarch64-linux-gnu.
> > Does the patch look OK to commit ?
>
> no, it is NOT ok.
>
> Consider:
>
> subroutine test0 (n, z)
>implicit none
>integer :: n
>real, pointer :: z(:,:,:) ! need not be contiguous!
>z = 0
> end subroutine
>
> After your patch this also generates a memset, but this cannot be true
> in general. One would need to have a test on contiguity of the array
> before memset can be used.
>
> In principle this is a nice idea, and IIRC there exists a very old PR
> on this (by Thomas König?). So it might be worth pursuing.
Hi Harald,
Thanks for the suggestions!
The attached patch checks gfc_is_simply_contiguous(expr, true, false) before
lowering to memset,
which avoids generating memset for your example above.
Bootstrapped+tested on aarch64-linux-gnu.
Does the attached patch look OK ?
Signed-off-by: Prathamesh Kulkarni
Thanks,
Prathamesh
>
> Thanks,
> Harald
>
>
> > Signed-off-by: Prathamesh Kulkarni
> >
> > Thanks,
> > Prathamesh
Lower zeroing array assignment to memset for allocatable arrays.
gcc/fortran/ChangeLog:
* trans-expr.cc (gfc_trans_zero_assign): Handle allocatable arrays.
gcc/testsuite/ChangeLog:
* gfortran.dg/array_memset_3.f90: New test.
Signed-off-by: Prathamesh Kulkarni
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 477c2720187..f9a7f70b2a3 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -11515,18 +11515,24 @@ gfc_trans_zero_assign (gfc_expr * expr)
type = TREE_TYPE (dest);
if (POINTER_TYPE_P (type))
type = TREE_TYPE (type);
- if (!GFC_ARRAY_TYPE_P (type))
-return NULL_TREE;
-
- /* Determine the length of the array. */
- len = GFC_TYPE_ARRAY_SIZE (type);
- if (!len || TREE_CODE (len) != INTEGER_CST)
+ if (GFC_ARRAY_TYPE_P (type))
+{
+ /* Determine the length of the array. */
+ len = GFC_TYPE_ARRAY_SIZE (type);
+ if (!len || TREE_CODE (len) != INTEGER_CST)
+ return NULL_TREE;
+}
+ else if (GFC_DESCRIPTOR_TYPE_P (type)
+ && gfc_is_simply_contiguous (expr, true, false))
+{
+ if (POINTER_TYPE_P (TREE_TYPE (dest)))
+ dest = build_fold_indirect_ref_loc (input_location, dest);
+ len = gfc_conv_descriptor_size (dest, GFC_TYPE_ARRAY_RANK (type));
+ dest = gfc_conv_descriptor_data_get (dest);
+}
+ else
return NULL_TREE;
- tmp = TYPE_SIZE_UNIT (gfc_get_element_type (type));
- len = fold_build2_loc (input_location, MULT_EXPR, gfc_array_index_type, len,
-fold_convert (gfc_array_index_type, tmp));
-
/* If we are zeroing a local array avoid taking its address by emitting
a = {} instead. */
if (!POINTER_TYPE_P (TREE_TYPE (dest)))
@@ -11534,6 +11540,11 @@ gfc_trans_zero_assign (gfc_expr * expr)
dest, build_constructor (TREE_TYPE (dest),
NULL));
+ /* Multiply len by element size. */
+ tmp = TYPE_SIZE_UNIT (gfc_get_element_type (type));
+ len = fold_build2_loc (input_location, MULT_EXPR, gfc_array_index_type,
+len, fold_convert (gfc_array_index_type, tmp));
+
/* Convert arguments to the correct types. */
dest = fold_convert (pvoid_type_node, dest);
len = fold_convert (size_type_node, len);
diff --git a/gcc/testsuite/gfortran.dg/array_memset_3.f90
b/gcc/testsuite/gfortran.dg/array_memset_3.f90
new file mode 100644
index 000..753006f7a91
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/array_memset_3.f90
@@ -0,0 +1,45 @@
+! { dg-do compile }
+! { dg-options "-O2 -fdump-tree-original" }
+
+subroutine test1(n)
+ implicit none
+integer(8) :: n
+real(4), allocatable :: z(:,:,:)
+
+allocate(z(n, 100, 200))
+z = 0
+end subroutine
+
+s
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
On Thu, Jul 11, 2024 at 11:24 AM Andre Vehreschild wrote: > > Hi Richard, > > would that be sufficient? Bootstrap is still running for me... Yes. Richard. > From c30c2cf829a094ba5e4c2c31333bed6e8c0d32af Mon Sep 17 00:00:00 2001 > From: Andre Vehreschild > Date: Thu, 11 Jul 2024 11:21:04 +0200 > Subject: [PATCH] [Fortran] Fix bootstrap broken by gcc-15-1965-ge4f2f46e015 > > gcc/fortran/ChangeLog: > > * trans-array.cc (gfc_conv_array_parameter): Init variable to > NULL_TREE to fix bootstrap. > --- > gcc/fortran/trans-array.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc > index 0fffa07495c..5558ab69969 100644 > --- a/gcc/fortran/trans-array.cc > +++ b/gcc/fortran/trans-array.cc > @@ -8750,7 +8750,7 @@ gfc_conv_array_parameter (gfc_se *se, gfc_expr *expr, > bool g77, >tree stmt; >tree parent = DECL_CONTEXT (current_function_decl); >tree ctree; > - tree pack_attr; > + tree pack_attr = NULL_TREE; /* Set when packing class arrays. */ >bool full_array_var; >bool this_array_result; >bool contiguous; > -- > 2.45.2 > > Sorry for the breakage. > > Regards, > Andre > > On Thu, 11 Jul 2024 11:06:47 +0200 > Richard Biener wrote: > > > On Thu, Jul 11, 2024 at 11:04 AM Andre Vehreschild > > wrote: > > > > > > Hi Richard, > > > > > > I am sorry to hear that. Shall I revert? > > > > I would suggest to instead fix by initializing the variable with NULL > > (and a comment). > > > > > - Andre > > > On Thu, 11 Jul 2024 10:57:48 +0200 > > > Richard Biener wrote: > > > > > > > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener > > > > wrote: > > > > > > > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild > > > > > wrote: > > > > > > > > > > > > Hi Harald, > > > > > > > > > > > > thank you very much for ok'ing this large patch. Merged as > > > > > > gcc-15-1965-ge4f2f46e015 > > > > > > > > > > > > Looking forward to get (no) bug reports ;-) > > > > > > > > > > This seems to break bootstrap with > > > > > > > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > > > > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const > > > > > gfc_symbol*, const char*, tree_node**, tree_node**, > > > > > tree_node**)’: ../../gcc/gcc/fortran/trans-array.cc:9135:41: > > > > > error: ‘pack_attr’ may be used uninitialized > > > > > [-Werror=maybe-uninitialized] 9135 | tmp = > > > > > build_call_expr_loc (input_location, | > > > > > ^~~~ 9136 | > > > > > gfor_fndecl_in_unpack_class, 4, tmp, > > > > > | > > > > > > > > > > 9137 | packedptr, > > > > > | ~~ > > > > > 9138 | size_in_bytes > > > > > (TREE_TYPE (ctree)), > > > > > | > > > > > ~~ > > > > > 9139 | pack_attr); > > > > > | ~~ > > > > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: ‘pack_attr’ > > > > > was declared here 8665 | tree pack_attr; > > > > > |^ > > > > > cc1plus: all warnings being treated as errors > > > > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > > > > > > > > It seems to be a false positive but GCCs little mind is too weak > > > > to prove that (yes, we error on the side of emitting a diagnostic > > > > if we can't prove it's initialized) > > > > > > > > Richard. > > > > > > > > > > > > > > > Thanks again, > > > > > > > > > > > > Andre > > > > > > > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > > > > > Harald Anlauf wrote: > > > > > > > > > > > > > Hi Andre, > > > > > > > > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > > > > > Hi Harald, > > > > > > > > > > > > > > > > thanks for the review. I totally agree, that this patch > > > > > > > > has gotten bigger than I expected (and wanted). But > > > > > > > > things are as they are. > > > > > > > > > > > > > > > > About the coding style: I have worked in so many projects, > > > > > > > > that I consider a consistent coding style luxury. I esp. > > > > > > > > do not have my own one anymore. The formating you are > > > > > > > > seeing in my patches is the result of clang-format with > > > > > > > > the provided parameter file in contrib/clang-format. I > > > > > > > > was happy to have a tool to do the formatting, that I > > > > > > > > could integrate into my IDE, because previously it was > > > > > > > > hard to mimic the GNU style. I try to get to the GNU > > > > > > > > style as good as possible, where I consider clang-format > > > > > > > > doing garbage. > > > > > > > > > > > > > > > > I see that clang-format has a "very specific opinion" on > > > > > > > > how to format the lines you mentioned, but it will > > > > > > > > "correct
Re: [Fortran, Patch, PR 96992, V4] Fix Class arrays of different ranks are rejected as storage association argument
Hi Richard, bootstrap finally passed and the fix is now merged as gcc-15-1971-gb9513c6746b Thanks for your patience. - Andre On Thu, 11 Jul 2024 14:01:02 +0200 Richard Biener wrote: > On Thu, Jul 11, 2024 at 11:24 AM Andre Vehreschild > wrote: > > > > Hi Richard, > > > > would that be sufficient? Bootstrap is still running for me... > > Yes. > > Richard. > > > From c30c2cf829a094ba5e4c2c31333bed6e8c0d32af Mon Sep 17 00:00:00 > > 2001 From: Andre Vehreschild > > Date: Thu, 11 Jul 2024 11:21:04 +0200 > > Subject: [PATCH] [Fortran] Fix bootstrap broken by > > gcc-15-1965-ge4f2f46e015 > > > > gcc/fortran/ChangeLog: > > > > * trans-array.cc (gfc_conv_array_parameter): Init variable > > to NULL_TREE to fix bootstrap. > > --- > > gcc/fortran/trans-array.cc | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc > > index 0fffa07495c..5558ab69969 100644 > > --- a/gcc/fortran/trans-array.cc > > +++ b/gcc/fortran/trans-array.cc > > @@ -8750,7 +8750,7 @@ gfc_conv_array_parameter (gfc_se *se, > > gfc_expr *expr, bool g77, tree stmt; > >tree parent = DECL_CONTEXT (current_function_decl); > >tree ctree; > > - tree pack_attr; > > + tree pack_attr = NULL_TREE; /* Set when packing class arrays. */ > >bool full_array_var; > >bool this_array_result; > >bool contiguous; > > -- > > 2.45.2 > > > > Sorry for the breakage. > > > > Regards, > > Andre > > > > On Thu, 11 Jul 2024 11:06:47 +0200 > > Richard Biener wrote: > > > > > On Thu, Jul 11, 2024 at 11:04 AM Andre Vehreschild > > > wrote: > > > > > > > > Hi Richard, > > > > > > > > I am sorry to hear that. Shall I revert? > > > > > > I would suggest to instead fix by initializing the variable with > > > NULL (and a comment). > > > > > > > - Andre > > > > On Thu, 11 Jul 2024 10:57:48 +0200 > > > > Richard Biener wrote: > > > > > > > > > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener > > > > > wrote: > > > > > > > > > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild > > > > > > wrote: > > > > > > > > > > > > > > Hi Harald, > > > > > > > > > > > > > > thank you very much for ok'ing this large patch. Merged as > > > > > > > gcc-15-1965-ge4f2f46e015 > > > > > > > > > > > > > > Looking forward to get (no) bug reports ;-) > > > > > > > > > > > > This seems to break bootstrap with > > > > > > > > > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void > > > > > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const > > > > > > gfc_symbol*, const char*, tree_node**, tree_node**, > > > > > > tree_node**)’: ../../gcc/gcc/fortran/trans-array.cc:9135:41: > > > > > > error: ‘pack_attr’ may be used uninitialized > > > > > > [-Werror=maybe-uninitialized] 9135 | tmp = > > > > > > build_call_expr_loc (input_location, | > > > > > > ^~~~ 9136 | > > > > > > gfor_fndecl_in_unpack_class, 4, tmp, > > > > > > | > > > > > > > > > > > > 9137 | packedptr, > > > > > > | ~~ > > > > > > 9138 | > > > > > > size_in_bytes (TREE_TYPE (ctree)), > > > > > > | > > > > > > ~~ > > > > > > 9139 | pack_attr); > > > > > > | ~~ > > > > > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note: > > > > > > ‘pack_attr’ was declared here 8665 | tree pack_attr; > > > > > > |^ > > > > > > cc1plus: all warnings being treated as errors > > > > > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1 > > > > > > > > > > > > > > > > It seems to be a false positive but GCCs little mind is too > > > > > weak to prove that (yes, we error on the side of emitting a > > > > > diagnostic if we can't prove it's initialized) > > > > > > > > > > Richard. > > > > > > > > > > > > > > > > > > Thanks again, > > > > > > > > > > > > > > Andre > > > > > > > > > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200 > > > > > > > Harald Anlauf wrote: > > > > > > > > > > > > > > > Hi Andre, > > > > > > > > > > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild: > > > > > > > > > Hi Harald, > > > > > > > > > > > > > > > > > > thanks for the review. I totally agree, that this > > > > > > > > > patch has gotten bigger than I expected (and wanted). > > > > > > > > > But things are as they are. > > > > > > > > > > > > > > > > > > About the coding style: I have worked in so many > > > > > > > > > projects, that I consider a consistent coding style > > > > > > > > > luxury. I esp. do not have my own one anymore. The > > > > > > > > > formating you are seeing in my patches is the result > > > > > > > > > of clang-format with the provided parameter file in > > > > > > > > > contrib/clang-format. I was happy to have a tool to > > > > >
[Patch, Fortran, PR88624, v1] Fix Rejects allocatable coarray passed as a dummy argument
Hi all,
attached patch fixes using of coarrays as dummy arguments. The coarray
dummy argument was not dereferenced correctly, which is fixed now.
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline.
Regards,
Andre
--
Andre Vehreschild * Email: vehre ad gcc dot gnu dot org
From 374ab1eec7621136de2d9f642b8abf13de197a41 Mon Sep 17 00:00:00 2001
From: Andre Vehreschild
Date: Thu, 11 Jul 2024 10:07:12 +0200
Subject: [PATCH] [Fortran] Fix Rejects allocatable coarray passed as a dummy
argument [88624]
Coarray parameters of procedures/functions need to be dereffed, because
they are references to the descriptor but the routine expected the
descriptor directly.
PR fortran/88624
gcc/fortran/ChangeLog:
* trans-expr.cc (gfc_conv_procedure_call): Treat
pointers/references (e.g. from parameters) correctly by derefing
them.
gcc/testsuite/ChangeLog:
* gfortran.dg/coarray/dummy_1.f90: Add calling function trough
function.
---
gcc/fortran/trans-expr.cc | 35 +--
gcc/testsuite/gfortran.dg/coarray/dummy_1.f90 | 2 ++
2 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 60495f199dc..0eba029a67a 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -7797,16 +7797,26 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
&& CLASS_DATA (fsym)->attr.codimension
&& !CLASS_DATA (fsym)->attr.allocatable)))
{
- tree caf_decl, caf_type;
+ tree caf_decl, caf_type, caf_desc = NULL_TREE;
tree offset, tmp2;
caf_decl = gfc_get_tree_for_caf_expr (e);
caf_type = TREE_TYPE (caf_decl);
-
- if (GFC_DESCRIPTOR_TYPE_P (caf_type)
- && (GFC_TYPE_ARRAY_AKIND (caf_type) == GFC_ARRAY_ALLOCATABLE
- || GFC_TYPE_ARRAY_AKIND (caf_type) == GFC_ARRAY_POINTER))
- tmp = gfc_conv_descriptor_token (caf_decl);
+ if (POINTER_TYPE_P (caf_type)
+ && GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (caf_type)))
+ caf_desc = TREE_TYPE (caf_type);
+ else if (GFC_DESCRIPTOR_TYPE_P (caf_type))
+ caf_desc = caf_type;
+
+ if (caf_desc
+ && (GFC_TYPE_ARRAY_AKIND (caf_desc) == GFC_ARRAY_ALLOCATABLE
+ || GFC_TYPE_ARRAY_AKIND (caf_desc) == GFC_ARRAY_POINTER))
+ {
+ tmp = POINTER_TYPE_P (TREE_TYPE (caf_decl))
+ ? build_fold_indirect_ref (caf_decl)
+ : caf_decl;
+ tmp = gfc_conv_descriptor_token (tmp);
+ }
else if (DECL_LANG_SPECIFIC (caf_decl)
&& GFC_DECL_TOKEN (caf_decl) != NULL_TREE)
tmp = GFC_DECL_TOKEN (caf_decl);
@@ -7819,8 +7829,8 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
vec_safe_push (stringargs, tmp);
- if (GFC_DESCRIPTOR_TYPE_P (caf_type)
- && GFC_TYPE_ARRAY_AKIND (caf_type) == GFC_ARRAY_ALLOCATABLE)
+ if (caf_desc
+ && GFC_TYPE_ARRAY_AKIND (caf_desc) == GFC_ARRAY_ALLOCATABLE)
offset = build_int_cst (gfc_array_index_type, 0);
else if (DECL_LANG_SPECIFIC (caf_decl)
&& GFC_DECL_CAF_OFFSET (caf_decl) != NULL_TREE)
@@ -7830,8 +7840,13 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
else
offset = build_int_cst (gfc_array_index_type, 0);
- if (GFC_DESCRIPTOR_TYPE_P (caf_type))
- tmp = gfc_conv_descriptor_data_get (caf_decl);
+ if (caf_desc)
+ {
+ tmp = POINTER_TYPE_P (TREE_TYPE (caf_decl))
+ ? build_fold_indirect_ref (caf_decl)
+ : caf_decl;
+ tmp = gfc_conv_descriptor_data_get (tmp);
+ }
else
{
gcc_assert (POINTER_TYPE_P (caf_type));
diff --git a/gcc/testsuite/gfortran.dg/coarray/dummy_1.f90 b/gcc/testsuite/gfortran.dg/coarray/dummy_1.f90
index 33e95853ad4..c437b2a10fc 100644
--- a/gcc/testsuite/gfortran.dg/coarray/dummy_1.f90
+++ b/gcc/testsuite/gfortran.dg/coarray/dummy_1.f90
@@ -66,5 +66,7 @@
if (lcobound(A, dim=1) /= 2) STOP 13
if (ucobound(A, dim=1) /= 3) STOP 14
if (lcobound(A, dim=2) /= 5) STOP 15
+
+call sub4(A) ! Check PR88624 is fixed.
end subroutine sub5
end
--
2.45.2
[Patch, Fortran, PR84244, v1] Fix ICE in recompute_tree_invariant_for_addr_expr, at tree.c:4535
Hi all,
the attached patch fixes a segfault in the compiler, where for pointer
components of a derived type the caf_token in the component was not
set, when the derived was previously used outside of a coarray.
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
Regards,
Andre
--
Andre Vehreschild * Email: vehre ad gcc dot gnu dot org
From 88f209316a980fbe78423d6aba747bb6b7fd404f Mon Sep 17 00:00:00 2001
From: Andre Vehreschild
Date: Thu, 11 Jul 2024 15:44:56 +0200
Subject: [PATCH] [Fortran] Fix ICE in recompute_tree_invariant_for_addr_expr,
at tree.c:4535 [PR84244]
Declaring an unused function with a derived type having a pointer
component and using that derived type as a coarray, lead the compiler to
ICE because the caf_token for the pointer was not linked into the
component correctly.
PR fortran/84244
gcc/fortran/ChangeLog:
* trans-types.cc (gfc_get_derived_type): When a caf_sub_token is
generated for a component, link it to the component it is
generated for (the previous one).
gcc/testsuite/ChangeLog:
* gfortran.dg/coarray/ptr_comp_5.f08: New test.
---
gcc/fortran/trans-types.cc| 6 +-
.../gfortran.dg/coarray/ptr_comp_5.f08| 19 +++
2 files changed, 24 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gfortran.dg/coarray/ptr_comp_5.f08
diff --git a/gcc/fortran/trans-types.cc b/gcc/fortran/trans-types.cc
index c76cdca4eae..83c0708ccbd 100644
--- a/gcc/fortran/trans-types.cc
+++ b/gcc/fortran/trans-types.cc
@@ -2647,7 +2647,7 @@ gfc_get_derived_type (gfc_symbol * derived, int codimen)
tree *chain = NULL;
bool got_canonical = false;
bool unlimited_entity = false;
- gfc_component *c;
+ gfc_component *c, *last_c = nullptr;
gfc_namespace *ns;
tree tmp;
bool coarray_flag, class_coarray_flag;
@@ -2947,10 +2947,14 @@ gfc_get_derived_type (gfc_symbol * derived, int codimen)
types. */
if (class_coarray_flag || !c->backend_decl)
c->backend_decl = field;
+ if (c->attr.caf_token && last_c)
+ last_c->caf_token = field;
if (c->attr.pointer && (c->attr.dimension || c->attr.codimension)
&& !(c->ts.type == BT_DERIVED && strcmp (c->name, "_data") == 0))
GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;
+
+ last_c = c;
}
/* Now lay out the derived type, including the fields. */
diff --git a/gcc/testsuite/gfortran.dg/coarray/ptr_comp_5.f08 b/gcc/testsuite/gfortran.dg/coarray/ptr_comp_5.f08
new file mode 100644
index 000..ed3a8db13fa
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/coarray/ptr_comp_5.f08
@@ -0,0 +1,19 @@
+! { dg-do compile }
+
+! Check PR84244 does not ICE anymore.
+
+program ptr_comp_5
+ integer, target :: dest = 42
+ type t
+integer, pointer :: p
+ end type
+ type(t) :: o[*]
+
+ o%p => dest
+contains
+ ! This unused routine is crucial for the ICE.
+ function f(x)
+type(t), intent(in) ::x
+ end function
+end program
+
--
2.45.2
[PATCH] fortran: Factor the evaluation of MINLOCK/MAXLOC's BACK argument
From: Mikael Morin
Hello,
I discovered this while testing the inline MINLOC/MAXLOC (aka PR90608) patches.
Regression tested on x86_64-linux.
OK for master?
-- 8< --
Move the evaluation of the BACK argument out of the loop in the inline code
generated for MINLOC or MAXLOC. For that, add a new (scalar) element
associated with BACK to the scalarization loop chain, evaluate the argument
with the context of that element, and let the scalarizer do its job.
The problem was not only a missed optimisation, but also a wrong code
one in the cases where the expression associated with BACK is not free of
side-effects, making multiple evaluations observable.
The new tests check the evaluation count of the BACK argument, and try to
cover all the variations (with/out NANs, constant or unknown shape, absent
or scalar or array MASK) supported by the inline implementation of the
functions. Care has been taken to not check the case of a constant .FALSE.
MASK, for which the evaluation of BACK can be elided.
gcc/fortran/ChangeLog:
* trans-intrinsic.cc (gfc_conv_intrinsic_minmaxloc): Create a new
scalar scalarization chain element if BACK is present. Add it to
the loop. Set the scalarization chain before evaluating the
argument.
gcc/testsuite/ChangeLog:
* gfortran.dg/maxloc_5.f90: New test.
* gfortran.dg/minloc_5.f90: New test.
---
gcc/fortran/trans-intrinsic.cc | 10 +
gcc/testsuite/gfortran.dg/maxloc_5.f90 | 257 +
gcc/testsuite/gfortran.dg/minloc_5.f90 | 257 +
3 files changed, 524 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/maxloc_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/minloc_5.f90
diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc
index 5ea10e84060..cadbd177452 100644
--- a/gcc/fortran/trans-intrinsic.cc
+++ b/gcc/fortran/trans-intrinsic.cc
@@ -5325,6 +5325,7 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
gfc_actual_arglist *actual;
gfc_ss *arrayss;
gfc_ss *maskss;
+ gfc_ss *backss;
gfc_se arrayse;
gfc_se maskse;
gfc_expr *arrayexpr;
@@ -5390,6 +5391,11 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
&& maskexpr->symtree->n.sym->attr.dummy
&& maskexpr->symtree->n.sym->attr.optional;
backexpr = actual->next->next->expr;
+ if (backexpr)
+backss = gfc_get_scalar_ss (gfc_ss_terminator, backexpr);
+ else
+backss = nullptr;
+
nonempty = NULL;
if (maskexpr && maskexpr->rank != 0)
{
@@ -5449,6 +5455,9 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
if (maskss)
gfc_add_ss_to_loop (&loop, maskss);
+ if (backss)
+gfc_add_ss_to_loop (&loop, backss);
+
gfc_add_ss_to_loop (&loop, arrayss);
/* Initialize the loop. */
@@ -5535,6 +5544,7 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
gfc_add_block_to_block (&block, &arrayse.pre);
gfc_init_se (&backse, NULL);
+ backse.ss = backss;
gfc_conv_expr_val (&backse, backexpr);
gfc_add_block_to_block (&block, &backse.pre);
diff --git a/gcc/testsuite/gfortran.dg/maxloc_5.f90
b/gcc/testsuite/gfortran.dg/maxloc_5.f90
new file mode 100644
index 000..5d722450c8f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/maxloc_5.f90
@@ -0,0 +1,257 @@
+! { dg-do run }
+!
+! Check that the evaluation of MAXLOC's BACK argument is made only once
+! before the scalarisation loops.
+
+program p
+ implicit none
+ integer, parameter :: data10(*) = (/ 7, 4, 7, 6, 6, 4, 6, 3, 9, 8 /)
+ logical, parameter :: mask10(*) = (/ .false., .true., .false., &
+ .false., .true., .true., &
+ .true. , .true., .false., &
+ .false. /)
+ integer :: calls_count = 0
+ call check_int_const_shape
+ call check_int_const_shape_scalar_mask
+ call check_int_const_shape_array_mask
+ call check_int_const_shape_optional_mask_present
+ call check_int_const_shape_optional_mask_absent
+ call check_int_const_shape_empty
+ call check_int_alloc
+ call check_int_alloc_scalar_mask
+ call check_int_alloc_array_mask
+ call check_int_alloc_empty
+ call check_real_const_shape
+ call check_real_const_shape_scalar_mask
+ call check_real_const_shape_array_mask
+ call check_real_const_shape_optional_mask_present
+ call check_real_const_shape_optional_mask_absent
+ call check_real_const_shape_empty
+ call check_real_alloc
+ call check_real_alloc_scalar_mask
+ call check_real_alloc_array_mask
+ call check_real_alloc_empty
+contains
+ function get_scalar_false()
+logical :: get_scalar_false
+calls_count = calls_count + 1
+get_scalar_false = .false.
+ end function
+ subroutine check_int_const_shape()
+integer :: a(10)
+logical :: m(10)
+integer :: r
+a = data10
+calls_count = 0
+r = ma
Re: Lower zeroing array assignment to memset for allocatable arrays
Hi Prathamesh! Am 11.07.24 um 12:16 schrieb Prathamesh Kulkarni: -Original Message- From: Harald Anlauf Sent: Thursday, July 11, 2024 12:53 AM To: Prathamesh Kulkarni ; gcc- patc...@gcc.gnu.org; fortran@gcc.gnu.org Subject: Re: Lower zeroing array assignment to memset for allocatable arrays External email: Use caution opening links or attachments Hi Prathamesh, Am 10.07.24 um 13:22 schrieb Prathamesh Kulkarni: Hi, The attached patch lowers zeroing array assignment to memset for allocatable arrays. For example: subroutine test(z, n) implicit none integer :: n real(4), allocatable :: z(:,:,:) allocate(z(n, 8192, 2048)) z = 0 end subroutine results in following call to memset instead of 3 nested loops for z = 0: (void) __builtin_memset ((void *) z->data, 0, (unsigned long) MAX_EXPR dim[0].ubound - z->dim[0].lbound, -1> + 1) * (MAX_EXPR dim[1].ubound - z->dim[1].lbound, -1> + 1)) * (MAX_EXPR dim[2].ubound - z->dim[2].lbound, -1> + 1)) * 4)); The patch significantly improves speedup for an internal Fortran application on AArch64 -mcpu=grace (and potentially on other AArch64 cores too). Bootstrapped+tested on aarch64-linux-gnu. Does the patch look OK to commit ? no, it is NOT ok. Consider: subroutine test0 (n, z) implicit none integer :: n real, pointer :: z(:,:,:) ! need not be contiguous! z = 0 end subroutine After your patch this also generates a memset, but this cannot be true in general. One would need to have a test on contiguity of the array before memset can be used. In principle this is a nice idea, and IIRC there exists a very old PR on this (by Thomas König?). So it might be worth pursuing. Hi Harald, Thanks for the suggestions! The attached patch checks gfc_is_simply_contiguous(expr, true, false) before lowering to memset, which avoids generating memset for your example above. This is much better, as it avoids generating false memsets where it should not. However, you now miss cases where the array is a component reference, as in: subroutine test_dt (dt) implicit none type t real, allocatable :: x(:,:,:) ! contiguous! real, pointer, contiguous :: y(:,:,:) ! contiguous! real, pointer :: z(:,:,:) ! need not be contiguous! end type t type(t) :: dt dt% x = 0 ! memset possible! dt% y = 0 ! memset possible! dt% z = 0 ! memset NOT possible! end subroutine You'll need to cycle through the component references and apply the check for contiguity to the ultimate component, not the top level. Can you have another look? Thanks, Harald Bootstrapped+tested on aarch64-linux-gnu. Does the attached patch look OK ? Signed-off-by: Prathamesh Kulkarni Thanks, Prathamesh Thanks, Harald Signed-off-by: Prathamesh Kulkarni Thanks, Prathamesh
Re: [PATCH] fortran: Factor the evaluation of MINLOCK/MAXLOC's BACK argument
Hi Mikael,
Am 11.07.24 um 21:55 schrieb Mikael Morin:
From: Mikael Morin
Hello,
I discovered this while testing the inline MINLOC/MAXLOC (aka PR90608) patches.
Regression tested on x86_64-linux.
OK for master?
this is a nice finding! (NAG seems to fail on the cases with
array size 0, while Intel gets it right.)
The commit message promises to cover all variations ("with/out NANs"?)
but I fail to see these. Were these removed in the submission?
Otherwise the patch looks pretty simple and is OK for mainline.
But do not forget to s/MINLOCK/MINLOC/ in the summary.
Thanks for the patch!
Harald
-- 8< --
Move the evaluation of the BACK argument out of the loop in the inline code
generated for MINLOC or MAXLOC. For that, add a new (scalar) element
associated with BACK to the scalarization loop chain, evaluate the argument
with the context of that element, and let the scalarizer do its job.
The problem was not only a missed optimisation, but also a wrong code
one in the cases where the expression associated with BACK is not free of
side-effects, making multiple evaluations observable.
The new tests check the evaluation count of the BACK argument, and try to
cover all the variations (with/out NANs, constant or unknown shape, absent
or scalar or array MASK) supported by the inline implementation of the
functions. Care has been taken to not check the case of a constant .FALSE.
MASK, for which the evaluation of BACK can be elided.
gcc/fortran/ChangeLog:
* trans-intrinsic.cc (gfc_conv_intrinsic_minmaxloc): Create a new
scalar scalarization chain element if BACK is present. Add it to
the loop. Set the scalarization chain before evaluating the
argument.
gcc/testsuite/ChangeLog:
* gfortran.dg/maxloc_5.f90: New test.
* gfortran.dg/minloc_5.f90: New test.
---
gcc/fortran/trans-intrinsic.cc | 10 +
gcc/testsuite/gfortran.dg/maxloc_5.f90 | 257 +
gcc/testsuite/gfortran.dg/minloc_5.f90 | 257 +
3 files changed, 524 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/maxloc_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/minloc_5.f90
diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc
index 5ea10e84060..cadbd177452 100644
--- a/gcc/fortran/trans-intrinsic.cc
+++ b/gcc/fortran/trans-intrinsic.cc
@@ -5325,6 +5325,7 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
gfc_actual_arglist *actual;
gfc_ss *arrayss;
gfc_ss *maskss;
+ gfc_ss *backss;
gfc_se arrayse;
gfc_se maskse;
gfc_expr *arrayexpr;
@@ -5390,6 +5391,11 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
&& maskexpr->symtree->n.sym->attr.dummy
&& maskexpr->symtree->n.sym->attr.optional;
backexpr = actual->next->next->expr;
+ if (backexpr)
+backss = gfc_get_scalar_ss (gfc_ss_terminator, backexpr);
+ else
+backss = nullptr;
+
nonempty = NULL;
if (maskexpr && maskexpr->rank != 0)
{
@@ -5449,6 +5455,9 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
if (maskss)
gfc_add_ss_to_loop (&loop, maskss);
+ if (backss)
+gfc_add_ss_to_loop (&loop, backss);
+
gfc_add_ss_to_loop (&loop, arrayss);
/* Initialize the loop. */
@@ -5535,6 +5544,7 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr *
expr, enum tree_code op)
gfc_add_block_to_block (&block, &arrayse.pre);
gfc_init_se (&backse, NULL);
+ backse.ss = backss;
gfc_conv_expr_val (&backse, backexpr);
gfc_add_block_to_block (&block, &backse.pre);
diff --git a/gcc/testsuite/gfortran.dg/maxloc_5.f90
b/gcc/testsuite/gfortran.dg/maxloc_5.f90
new file mode 100644
index 000..5d722450c8f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/maxloc_5.f90
@@ -0,0 +1,257 @@
+! { dg-do run }
+!
+! Check that the evaluation of MAXLOC's BACK argument is made only once
+! before the scalarisation loops.
+
+program p
+ implicit none
+ integer, parameter :: data10(*) = (/ 7, 4, 7, 6, 6, 4, 6, 3, 9, 8 /)
+ logical, parameter :: mask10(*) = (/ .false., .true., .false., &
+ .false., .true., .true., &
+ .true. , .true., .false., &
+ .false. /)
+ integer :: calls_count = 0
+ call check_int_const_shape
+ call check_int_const_shape_scalar_mask
+ call check_int_const_shape_array_mask
+ call check_int_const_shape_optional_mask_present
+ call check_int_const_shape_optional_mask_absent
+ call check_int_const_shape_empty
+ call check_int_alloc
+ call check_int_alloc_scalar_mask
+ call check_int_alloc_array_mask
+ call check_int_alloc_empty
+ call check_real_const_shape
+ call check_real_const_shape_scalar_mask
+ call check_real_const_shape_array_mask
+ call check_real_const_shape_optional_mask_present
+ call check_real_const_shape_optional_mask_absen
