Message ID | 20220125173213.1265f8e3@vepi2 |
---|---|
State | New |
Headers | show |
Series | [PR103970,Fortran,Coarray] Multi-image co_broadcast of derived type with allocatable components fails^ | expand |
Hi Andre', Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: > Hi all, > > attached patch fixes wrong code generation when broadcasting a derived type > containing allocatable and non-allocatable scalars. Furthermore does it prevent > broadcasting of coarray-tokens, which are always local this_image. Thus having > them on a different image makes no sense. > > Bootstrapped and regtested ok on x86_64-linux/F35. > > Ok, for trunk and backport to 12 and 11-branch after decent time? > > I perceived that 12 is closed for this kind of bugfix, therefore asking ok for > 13. I do not think that 12 is closed for bugfixing, especially not for fortran. And if my cursory reading of the patch is not misleading, the impact of the patch is really limited to coarrays. You may want to wait for another 1-2 days for additional comments. If not, it is OK from my side. Thanks for the patch! Harald > Regards, > Andre > -- > Andre Vehreschild * Email: vehre ad gmx dot de
Hi Harald, thanks for the fast review. I have submitted as c9c48ab7bad. Will wait for two weeks (reminder set :-)) before backporting to gcc-11. Thank you and regards, Andre On Tue, 25 Jan 2022 22:30:22 +0100 Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote: > Hi Andre', > > Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: > > Hi all, > > > > attached patch fixes wrong code generation when broadcasting a derived type > > containing allocatable and non-allocatable scalars. Furthermore does it > > prevent broadcasting of coarray-tokens, which are always local this_image. > > Thus having them on a different image makes no sense. > > > > Bootstrapped and regtested ok on x86_64-linux/F35. > > > > Ok, for trunk and backport to 12 and 11-branch after decent time? > > > > I perceived that 12 is closed for this kind of bugfix, therefore asking ok > > for 13. > > I do not think that 12 is closed for bugfixing, especially not for > fortran. And if my cursory reading of the patch is not misleading, > the impact of the patch is really limited to coarrays. > > You may want to wait for another 1-2 days for additional comments. > If not, it is OK from my side. > > Thanks for the patch! > > Harald > > > Regards, > > Andre > > -- > > Andre Vehreschild * Email: vehre ad gmx dot de > > -- Andre Vehreschild * Email: vehre ad gmx dot de
Hi Andre, your patch breaks bootstrapping: ../../repos/gcc/gcc/fortran/trans-array.cc: In function ‘tree_node* structure_alloc_comps(gfc_symbol*, tree, tree, int, int, int, gfc_co_subroutines_args*)’: ../../repos/gcc/gcc/fortran/trans-array.cc:9200:42: error: ‘cdesc’ may be used uninitialized [-Werror=maybe-uninitialized] 9200 | gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ ../../repos/gcc/gcc/fortran/trans-array.cc:9082:16: note: ‘cdesc’ was declared here 9082 | tree cdesc; | ^~~~~ cc1plus: all warnings being treated as errors make[3]: *** [Makefile:1143: fortran/trans-array.o] Error 1 Tobias On 28.01.22 10:07, Andre Vehreschild via Fortran wrote: > Hi Harald, > > thanks for the fast review. I have submitted as c9c48ab7bad. > > Will wait for two weeks (reminder set :-)) before backporting to gcc-11. > > Thank you and regards, > Andre > > On Tue, 25 Jan 2022 22:30:22 +0100 > Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote: > >> Hi Andre', >> >> Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: >>> Hi all, >>> >>> attached patch fixes wrong code generation when broadcasting a derived type >>> containing allocatable and non-allocatable scalars. Furthermore does it >>> prevent broadcasting of coarray-tokens, which are always local this_image. >>> Thus having them on a different image makes no sense. >>> >>> Bootstrapped and regtested ok on x86_64-linux/F35. >>> >>> Ok, for trunk and backport to 12 and 11-branch after decent time? >>> >>> I perceived that 12 is closed for this kind of bugfix, therefore asking ok >>> for 13. >> I do not think that 12 is closed for bugfixing, especially not for >> fortran. And if my cursory reading of the patch is not misleading, >> the impact of the patch is really limited to coarrays. >> >> You may want to wait for another 1-2 days for additional comments. >> If not, it is OK from my side. >> >> Thanks for the patch! >> >> Harald >> >>> Regards, >>> Andre >>> -- >>> Andre Vehreschild * Email: vehre ad gmx dot de >> > > -- > Andre Vehreschild * Email: vehre ad gmx dot de ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Hi Tobias, ups, sorry, reverted immediately. Regards, Andre On Fri, 28 Jan 2022 10:27:26 +0100 Tobias Burnus <tobias@codesourcery.com> wrote: > Hi Andre, > > your patch breaks bootstrapping: > > ../../repos/gcc/gcc/fortran/trans-array.cc: In function ‘tree_node* > structure_alloc_comps(gfc_symbol*, tree, tree, int, int, int, > gfc_co_subroutines_args*)’: > ../../repos/gcc/gcc/fortran/trans-array.cc:9200:42: error: ‘cdesc’ may be > used uninitialized [-Werror=maybe-uninitialized] 9200 | > gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); | > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > ../../repos/gcc/gcc/fortran/trans-array.cc:9082:16: note: ‘cdesc’ was > declared here 9082 | tree cdesc; | ^~~~~ cc1plus: > all warnings being treated as errors make[3]: *** [Makefile:1143: > fortran/trans-array.o] Error 1 > > Tobias > > On 28.01.22 10:07, Andre Vehreschild via Fortran wrote: > > Hi Harald, > > > > thanks for the fast review. I have submitted as c9c48ab7bad. > > > > Will wait for two weeks (reminder set :-)) before backporting to gcc-11. > > > > Thank you and regards, > > Andre > > > > On Tue, 25 Jan 2022 22:30:22 +0100 > > Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote: > > > >> Hi Andre', > >> > >> Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: > >>> Hi all, > >>> > >>> attached patch fixes wrong code generation when broadcasting a derived > >>> type containing allocatable and non-allocatable scalars. Furthermore does > >>> it prevent broadcasting of coarray-tokens, which are always local > >>> this_image. Thus having them on a different image makes no sense. > >>> > >>> Bootstrapped and regtested ok on x86_64-linux/F35. > >>> > >>> Ok, for trunk and backport to 12 and 11-branch after decent time? > >>> > >>> I perceived that 12 is closed for this kind of bugfix, therefore asking ok > >>> for 13. > >> I do not think that 12 is closed for bugfixing, especially not for > >> fortran. And if my cursory reading of the patch is not misleading, > >> the impact of the patch is really limited to coarrays. > >> > >> You may want to wait for another 1-2 days for additional comments. > >> If not, it is OK from my side. > >> > >> Thanks for the patch! > >> > >> Harald > >> > >>> Regards, > >>> Andre > >>> -- > >>> Andre Vehreschild * Email: vehre ad gmx dot de > >> > > > > -- > > Andre Vehreschild * Email: vehre ad gmx dot de > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 > München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas > Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht > München, HRB 106955
Hi Tobias, I don't know why that bootstrapped initially. I fixed the patch (naming a ``` else /* Prevent warning. */ cdesc = NULL_TREE; ``` obvious) and rerun bootstrap making sure to purge everything beforehand. It did not break bootstrap on x86_64-linux/f35. Hope it doesn't elsewhere with submit 26e237fb5b8. Thanks for pointing this out. Regards, Andre On Fri, 28 Jan 2022 10:36:23 +0100 Andre Vehreschild via Fortran <fortran@gcc.gnu.org> wrote: > Hi Tobias, > > ups, sorry, reverted immediately. > > Regards, > Andre > > On Fri, 28 Jan 2022 10:27:26 +0100 > Tobias Burnus <tobias@codesourcery.com> wrote: > > > Hi Andre, > > > > your patch breaks bootstrapping: > > > > ../../repos/gcc/gcc/fortran/trans-array.cc: In function ‘tree_node* > > structure_alloc_comps(gfc_symbol*, tree, tree, int, int, int, > > gfc_co_subroutines_args*)’: > > ../../repos/gcc/gcc/fortran/trans-array.cc:9200:42: error: ‘cdesc’ may be > > used uninitialized [-Werror=maybe-uninitialized] 9200 | > > gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); | > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > > ../../repos/gcc/gcc/fortran/trans-array.cc:9082:16: note: ‘cdesc’ was > > declared here 9082 | tree cdesc; | ^~~~~ cc1plus: > > all warnings being treated as errors make[3]: *** [Makefile:1143: > > fortran/trans-array.o] Error 1 > > > > Tobias > > > > On 28.01.22 10:07, Andre Vehreschild via Fortran wrote: > > > Hi Harald, > > > > > > thanks for the fast review. I have submitted as c9c48ab7bad. > > > > > > Will wait for two weeks (reminder set :-)) before backporting to gcc-11. > > > > > > Thank you and regards, > > > Andre > > > > > > On Tue, 25 Jan 2022 22:30:22 +0100 > > > Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote: > > > > > >> Hi Andre', > > >> > > >> Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: > > >>> Hi all, > > >>> > > >>> attached patch fixes wrong code generation when broadcasting a derived > > >>> type containing allocatable and non-allocatable scalars. Furthermore > > >>> does it prevent broadcasting of coarray-tokens, which are always local > > >>> this_image. Thus having them on a different image makes no sense. > > >>> > > >>> Bootstrapped and regtested ok on x86_64-linux/F35. > > >>> > > >>> Ok, for trunk and backport to 12 and 11-branch after decent time? > > >>> > > >>> I perceived that 12 is closed for this kind of bugfix, therefore asking > > >>> ok for 13. > > >> I do not think that 12 is closed for bugfixing, especially not for > > >> fortran. And if my cursory reading of the patch is not misleading, > > >> the impact of the patch is really limited to coarrays. > > >> > > >> You may want to wait for another 1-2 days for additional comments. > > >> If not, it is OK from my side. > > >> > > >> Thanks for the patch! > > >> > > >> Harald > > >> > > >>> Regards, > > >>> Andre > > >>> -- > > >>> Andre Vehreschild * Email: vehre ad gmx dot de > > >> > > > > > > -- > > > Andre Vehreschild * Email: vehre ad gmx dot de > > ----------------- > > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, > > 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: > > Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; > > Registergericht München, HRB 106955 > >
Hi all, two weeks have passed with no complains about the patch for PR103970. Therefore backported and pushed to gcc-11 as 680ee9c3332. Regards, Andre On Fri, 28 Jan 2022 12:39:17 +0100 Andre Vehreschild <vehre@gmx.de> wrote: > Hi Tobias, > > I don't know why that bootstrapped initially. I fixed the patch (naming a > ``` > else > /* Prevent warning. */ > cdesc = NULL_TREE; > ``` > obvious) and rerun bootstrap making sure to purge everything beforehand. It > did not break bootstrap on x86_64-linux/f35. Hope it doesn't elsewhere with > submit 26e237fb5b8. > > Thanks for pointing this out. > > Regards, > Andre > > On Fri, 28 Jan 2022 10:36:23 +0100 > Andre Vehreschild via Fortran <fortran@gcc.gnu.org> wrote: > > > Hi Tobias, > > > > ups, sorry, reverted immediately. > > > > Regards, > > Andre > > > > On Fri, 28 Jan 2022 10:27:26 +0100 > > Tobias Burnus <tobias@codesourcery.com> wrote: > > > > > Hi Andre, > > > > > > your patch breaks bootstrapping: > > > > > > ../../repos/gcc/gcc/fortran/trans-array.cc: In function ‘tree_node* > > > structure_alloc_comps(gfc_symbol*, tree, tree, int, int, int, > > > gfc_co_subroutines_args*)’: > > > ../../repos/gcc/gcc/fortran/trans-array.cc:9200:42: error: ‘cdesc’ may be > > > used uninitialized [-Werror=maybe-uninitialized] 9200 | > > > gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); | > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > > > ../../repos/gcc/gcc/fortran/trans-array.cc:9082:16: note: ‘cdesc’ was > > > declared here 9082 | tree cdesc; | ^~~~~ cc1plus: > > > all warnings being treated as errors make[3]: *** [Makefile:1143: > > > fortran/trans-array.o] Error 1 > > > > > > Tobias > > > > > > On 28.01.22 10:07, Andre Vehreschild via Fortran wrote: > > > > Hi Harald, > > > > > > > > thanks for the fast review. I have submitted as c9c48ab7bad. > > > > > > > > Will wait for two weeks (reminder set :-)) before backporting to gcc-11. > > > > > > > > Thank you and regards, > > > > Andre > > > > > > > > On Tue, 25 Jan 2022 22:30:22 +0100 > > > > Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote: > > > > > > > >> Hi Andre', > > > >> > > > >> Am 25.01.22 um 17:32 schrieb Andre Vehreschild via Fortran: > > > >>> Hi all, > > > >>> > > > >>> attached patch fixes wrong code generation when broadcasting a derived > > > >>> type containing allocatable and non-allocatable scalars. Furthermore > > > >>> does it prevent broadcasting of coarray-tokens, which are always local > > > >>> this_image. Thus having them on a different image makes no sense. > > > >>> > > > >>> Bootstrapped and regtested ok on x86_64-linux/F35. > > > >>> > > > >>> Ok, for trunk and backport to 12 and 11-branch after decent time? > > > >>> > > > >>> I perceived that 12 is closed for this kind of bugfix, therefore > > > >>> asking ok for 13. > > > >> I do not think that 12 is closed for bugfixing, especially not for > > > >> fortran. And if my cursory reading of the patch is not misleading, > > > >> the impact of the patch is really limited to coarrays. > > > >> > > > >> You may want to wait for another 1-2 days for additional comments. > > > >> If not, it is OK from my side. > > > >> > > > >> Thanks for the patch! > > > >> > > > >> Harald > > > >> > > > >>> Regards, > > > >>> Andre > > > >>> -- > > > >>> Andre Vehreschild * Email: vehre ad gmx dot de > > > >> > > > > > > > > -- > > > > Andre Vehreschild * Email: vehre ad gmx dot de > > > ----------------- > > > 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 > > > > > >
diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc index 2f0c8a4d412..1234932aaff 100644 --- a/gcc/fortran/trans-array.cc +++ b/gcc/fortran/trans-array.cc @@ -9102,6 +9102,10 @@ structure_alloc_comps (gfc_symbol * der_type, tree decl, continue; } + /* Do not broadcast a caf_token. These are local to the image. */ + if (attr->caf_token) + continue; + add_when_allocated = NULL_TREE; if (cmp_has_alloc_comps && !c->attr.pointer && !c->attr.proc_pointer) @@ -9134,10 +9138,13 @@ structure_alloc_comps (gfc_symbol * der_type, tree decl, if (attr->dimension) { tmp = gfc_get_element_type (TREE_TYPE (comp)); - ubound = gfc_full_array_size (&tmpblock, comp, - c->ts.type == BT_CLASS - ? CLASS_DATA (c)->as->rank - : c->as->rank); + if (!GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (comp))) + ubound = GFC_TYPE_ARRAY_SIZE (TREE_TYPE (comp)); + else + ubound = gfc_full_array_size (&tmpblock, comp, + c->ts.type == BT_CLASS + ? CLASS_DATA (c)->as->rank + : c->as->rank); } else { @@ -9145,26 +9152,36 @@ structure_alloc_comps (gfc_symbol * der_type, tree decl, ubound = build_int_cst (gfc_array_index_type, 1); } - cdesc = gfc_get_array_type_bounds (tmp, 1, 0, &gfc_index_one_node, - &ubound, 1, - GFC_ARRAY_ALLOCATABLE, false); + /* Treat strings like arrays. Or the other way around, do not + * generate an additional array layer for scalar components. */ + if (attr->dimension || c->ts.type == BT_CHARACTER) + { + cdesc = gfc_get_array_type_bounds (tmp, 1, 0, &gfc_index_one_node, + &ubound, 1, + GFC_ARRAY_ALLOCATABLE, false); - cdesc = gfc_create_var (cdesc, "cdesc"); - DECL_ARTIFICIAL (cdesc) = 1; + cdesc = gfc_create_var (cdesc, "cdesc"); + DECL_ARTIFICIAL (cdesc) = 1; - gfc_add_modify (&tmpblock, gfc_conv_descriptor_dtype (cdesc), - gfc_get_dtype_rank_type (1, tmp)); - gfc_conv_descriptor_lbound_set (&tmpblock, cdesc, - gfc_index_zero_node, - gfc_index_one_node); - gfc_conv_descriptor_stride_set (&tmpblock, cdesc, - gfc_index_zero_node, - gfc_index_one_node); - gfc_conv_descriptor_ubound_set (&tmpblock, cdesc, - gfc_index_zero_node, ubound); + gfc_add_modify (&tmpblock, gfc_conv_descriptor_dtype (cdesc), + gfc_get_dtype_rank_type (1, tmp)); + gfc_conv_descriptor_lbound_set (&tmpblock, cdesc, + gfc_index_zero_node, + gfc_index_one_node); + gfc_conv_descriptor_stride_set (&tmpblock, cdesc, + gfc_index_zero_node, + gfc_index_one_node); + gfc_conv_descriptor_ubound_set (&tmpblock, cdesc, + gfc_index_zero_node, ubound); + } if (attr->dimension) - comp = gfc_conv_descriptor_data_get (comp); + { + if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (comp))) + comp = gfc_conv_descriptor_data_get (comp); + else + comp = gfc_build_addr_expr (NULL_TREE, comp); + } else { gfc_se se; @@ -9172,14 +9189,18 @@ structure_alloc_comps (gfc_symbol * der_type, tree decl, gfc_init_se (&se, NULL); comp = gfc_conv_scalar_to_descriptor (&se, comp, - c->ts.type == BT_CLASS - ? CLASS_DATA (c)->attr - : c->attr); - comp = gfc_build_addr_expr (NULL_TREE, comp); + c->ts.type == BT_CLASS + ? CLASS_DATA (c)->attr + : c->attr); + if (c->ts.type == BT_CHARACTER) + comp = gfc_build_addr_expr (NULL_TREE, comp); gfc_add_block_to_block (&tmpblock, &se.pre); } - gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); + if (attr->dimension || c->ts.type == BT_CHARACTER) + gfc_conv_descriptor_data_set (&tmpblock, cdesc, comp); + else + cdesc = comp; tree fndecl; diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc index fccf0a9b229..8a3636ca5b2 100644 --- a/gcc/fortran/trans-intrinsic.cc +++ b/gcc/fortran/trans-intrinsic.cc @@ -11211,24 +11211,31 @@ conv_co_collective (gfc_code *code) return gfc_finish_block (&block); } + gfc_symbol *derived = code->ext.actual->expr->ts.type == BT_DERIVED + ? code->ext.actual->expr->ts.u.derived : NULL; + /* Handle the array. */ gfc_init_se (&argse, NULL); - if (code->ext.actual->expr->rank == 0) - { - symbol_attribute attr; - gfc_clear_attr (&attr); - gfc_init_se (&argse, NULL); - gfc_conv_expr (&argse, code->ext.actual->expr); - gfc_add_block_to_block (&block, &argse.pre); - gfc_add_block_to_block (&post_block, &argse.post); - array = gfc_conv_scalar_to_descriptor (&argse, argse.expr, attr); - array = gfc_build_addr_expr (NULL_TREE, array); - } - else + if (!derived || !derived->attr.alloc_comp + || code->resolved_isym->id != GFC_ISYM_CO_BROADCAST) { - argse.want_pointer = 1; - gfc_conv_expr_descriptor (&argse, code->ext.actual->expr); - array = argse.expr; + if (code->ext.actual->expr->rank == 0) + { + symbol_attribute attr; + gfc_clear_attr (&attr); + gfc_init_se (&argse, NULL); + gfc_conv_expr (&argse, code->ext.actual->expr); + gfc_add_block_to_block (&block, &argse.pre); + gfc_add_block_to_block (&post_block, &argse.post); + array = gfc_conv_scalar_to_descriptor (&argse, argse.expr, attr); + array = gfc_build_addr_expr (NULL_TREE, array); + } + else + { + argse.want_pointer = 1; + gfc_conv_expr_descriptor (&argse, code->ext.actual->expr); + array = argse.expr; + } } gfc_add_block_to_block (&block, &argse.pre); @@ -11289,9 +11296,6 @@ conv_co_collective (gfc_code *code) gcc_unreachable (); } - gfc_symbol *derived = code->ext.actual->expr->ts.type == BT_DERIVED - ? code->ext.actual->expr->ts.u.derived : NULL; - if (derived && derived->attr.alloc_comp && code->resolved_isym->id == GFC_ISYM_CO_BROADCAST) /* The derived type has the attribute 'alloc_comp'. */ diff --git a/gcc/testsuite/gfortran.dg/coarray_collectives_18.f90 b/gcc/testsuite/gfortran.dg/coarray_collectives_18.f90 new file mode 100644 index 00000000000..c83899de0e5 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/coarray_collectives_18.f90 @@ -0,0 +1,37 @@ +! { dg-do compile } +! { dg-additional-options "-fdump-tree-original -fcoarray=lib" } +! +! PR 103970 +! Test case inspired by code submitted by Damian Rousson + +program main + + implicit none + + type foo_t + integer i + integer, allocatable :: j + end type + + type(foo_t) foo + integer, parameter :: source_image = 1 + + if (this_image() == source_image) then + foo = foo_t(2,3) + else + allocate(foo%j) + end if + call co_broadcast(foo, source_image) + + if ((foo%i /= 2) .or. (foo%j /= 3)) error stop 1 + sync all + +end program + +! Wrong code generation produced too many temp descriptors +! leading to stacked descriptors handed to the co_broadcast. +! This lead to access to non exsitant memory in opencoarrays. +! In single image mode just checking for reduced number of +! descriptors is possible, i.e., execute always works. +! { dg-final { scan-tree-dump-times "desc\\.\[0-9\]+" 12 "original" } } +