* [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
@ 2010-07-16 14:05 Tobias Burnus
2010-07-16 14:40 ` Paul Richard Thomas
2010-07-16 14:48 ` Jakub Jelinek
0 siblings, 2 replies; 6+ messages in thread
From: Tobias Burnus @ 2010-07-16 14:05 UTC (permalink / raw)
To: gcc patches, gfortran
[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]
gfortran can produce the multiple declarations for module variables.
This causes problems when inlining is done as then the alias analysis
breaks.
For instance, the recent patch to the string intrinsics (marking some as
PURE) "caused" gfortran.dg/char_array_structure_constructor.f90 to break
with -m32 on x86_64-apple-darwin10.
The patch fixes this for -fwhole-file. Does anyone know why derived
types were excluded?
I have now added -fwhole-file to the test case to silence the error
(with this patch applied), but I think the real solution it to switch to
-fwhole-file by default.
Build and regtested on x86-64-linux. OK for the trunk? What about 4.5?
* * *
RFC: Is there any compelling reason not to switch to -fwhole-file by
default? I think we have slowly reached the state when there are more
bugs without that option than with that option.
Advantages
- Fixes some wrong-code issues
- Improves diagnostic
- Improves (non-LTO) optimizations
- More consistent codepath: The same for default, LTO and -fwhole-program
Disadvantage
- Some bugs, leading to wrong code
I think most wrong-code issues come apparent with -fwhole-program, which
allows for more optimizations - but that option implies -fwhole-file
(thus there is no change). Using -flto, one sees fewer problems as this
fixes some bad DECL.
Tobias
[-- Attachment #2: decl.diff --]
[-- Type: text/x-patch, Size: 1640 bytes --]
2010-07-16 Tobias Burnus <burnus@net-b.de>
PR fortran/44945
* trans-decl.c (gfc_get_symbol_decl): Use module decl with
-fwhole-file also for derived types.
2010-07-16 Tobias Burnus <burnus@net-b.de>
PR fortran/44945
* gfortran.dg/char_array_structure_constructor.f90: Add
-fwhole-file as dg-option as it otherwise fails on some
systems.
Index: gcc/fortran/trans-decl.c
===================================================================
--- gcc/fortran/trans-decl.c (revision 162255)
+++ gcc/fortran/trans-decl.c (working copy)
@@ -1149,11 +1149,9 @@ gfc_get_symbol_decl (gfc_symbol * sym)
return sym->backend_decl;
/* If use associated and whole file compilation, use the module
- declaration. This is only needed for intrinsic types because
- they are substituted for one another during optimization. */
+ declaration. */
if (gfc_option.flag_whole_file
&& sym->attr.flavor == FL_VARIABLE
- && sym->ts.type != BT_DERIVED
&& sym->attr.use_assoc
&& sym->module)
{
Index: gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90
===================================================================
--- gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (revision 162255)
+++ gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (working copy)
@@ -1,4 +1,9 @@
! { dg-do run }
+! { dg-options "-O3 -fwhole-file" }
+!
+! PR fortran/19107
+! -fwhole-file flag added for PR fortran/44945
+!
! This test the fix of PR19107, where character array actual
! arguments in derived type constructors caused an ICE.
! It also checks that the scalar counterparts are OK.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
2010-07-16 14:05 [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression") Tobias Burnus
@ 2010-07-16 14:40 ` Paul Richard Thomas
2010-07-22 13:59 ` Tobias Burnus
2010-07-16 14:48 ` Jakub Jelinek
1 sibling, 1 reply; 6+ messages in thread
From: Paul Richard Thomas @ 2010-07-16 14:40 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc patches, gfortran
Tobias,
OK for trunk and 4.5 after a delay of a week or two.
As I say elsewhere, I would be sure that the different treatment of
derived types came about form TYPE_CANONICAL not being set.
Thanks
Paul
On Fri, Jul 16, 2010 at 4:04 PM, Tobias Burnus <burnus@net-b.de> wrote:
> gfortran can produce the multiple declarations for module variables.
> This causes problems when inlining is done as then the alias analysis
> breaks.
>
> For instance, the recent patch to the string intrinsics (marking some as
> PURE) "caused" gfortran.dg/char_array_structure_constructor.f90 to break
> with -m32 on x86_64-apple-darwin10.
>
> The patch fixes this for -fwhole-file. Does anyone know why derived
> types were excluded?
>
> I have now added -fwhole-file to the test case to silence the error
> (with this patch applied), but I think the real solution it to switch to
> -fwhole-file by default.
>
> Build and regtested on x86-64-linux. OK for the trunk? What about 4.5?
>
> * * *
>
> RFC: Is there any compelling reason not to switch to -fwhole-file by
> default? I think we have slowly reached the state when there are more
> bugs without that option than with that option.
>
> Advantages
> - Fixes some wrong-code issues
> - Improves diagnostic
> - Improves (non-LTO) optimizations
> - More consistent codepath: The same for default, LTO and -fwhole-program
>
> Disadvantage
> - Some bugs, leading to wrong code
>
> I think most wrong-code issues come apparent with -fwhole-program, which
> allows for more optimizations - but that option implies -fwhole-file
> (thus there is no change). Using -flto, one sees fewer problems as this
> fixes some bad DECL.
>
> Tobias
>
--
The knack of flying is learning how to throw yourself at the ground and miss.
--Hitchhikers Guide to the Galaxy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
2010-07-16 14:05 [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression") Tobias Burnus
2010-07-16 14:40 ` Paul Richard Thomas
@ 2010-07-16 14:48 ` Jakub Jelinek
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Jelinek @ 2010-07-16 14:48 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc patches, gfortran
On Fri, Jul 16, 2010 at 04:04:41PM +0200, Tobias Burnus wrote:
> 2010-07-16 Tobias Burnus <burnus@net-b.de>
>
> PR fortran/44945
> * gfortran.dg/char_array_structure_constructor.f90: Add
> -fwhole-file as dg-option as it otherwise fails on some
> systems.
>
> --- gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (revision 162255)
> +++ gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (working copy)
> @@ -1,4 +1,9 @@
> ! { dg-do run }
> +! { dg-options "-O3 -fwhole-file" }
Shouldn't this be just "-fwhole-file"? Otherwise you test 4 times the same
command line, as -O3 comes after the -O? from cycling through the
optimization options.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
2010-07-16 14:40 ` Paul Richard Thomas
@ 2010-07-22 13:59 ` Tobias Burnus
2010-07-22 14:09 ` Tobias Burnus
2010-07-23 19:03 ` Paul Richard Thomas
0 siblings, 2 replies; 6+ messages in thread
From: Tobias Burnus @ 2010-07-22 13:59 UTC (permalink / raw)
To: Paul Richard Thomas; +Cc: gcc patches, gfortran
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
Dear all,
I have updated the patch.
On 07/16/2010 04:40 PM, Paul Richard Thomas wrote:
> On Fri, Jul 16, 2010 at 4:04 PM, Tobias Burnus <burnus@net-b.de> wrote:
>
>> gfortran can produce the multiple declarations for module variables.
>> This causes problems when inlining is done as then the alias analysis
>> breaks.
>>
> OK for trunk and 4.5 after a delay of a week or two.
>
As written in the PR: The patch has caused ICEs with -fwhole-file for
some derived types as the backend decl of the components was not copied.
This is fixed by the attached patch. Additionally, I removed the -O3
from the dg-options as suggested by Jakub.
Build and regtested with -fwhole-file enabled by default. (Without the
code path is not taken.) I do not see any ICEs in the test suite, found
one bug relating -fwhole-file and ENTRY (PR 45030), and several failures
due to do changed error/warning messages.
OK for the trunk - and, as suggested, for the 4.5 branch?
Regarding enabling -fwhole-file by default: I think one should do so as
soon as the ENTRY bug (PR 45030) is fixed. That gives us a long time
during 4.5 to fix the fall out bugs. (The next step is then to fix the
decl bugs, which cause -fwhole-program failues, but that's a separate
issue.)
Tobias
[-- Attachment #2: decl2.diff --]
[-- Type: text/x-patch, Size: 4241 bytes --]
2010-07-22 Tobias Burnus <burnus@net-b.de>
PR fortran/44945
* trans-decl.c (gfc_get_symbol_decl): Use module decl with
-fwhole-file also for derived types.
* trans-types.c (copy_dt_decls_ifequal): Remove static and
rename to gfc_copy_dt_decls_ifequal.
(gfc_get_derived_type): Update call.
* trans-types.h (gfc_copy_dt_decls_ifequal): Add prototype.
2010-07-22 Tobias Burnus <burnus@net-b.de>
PR fortran/44945
* gfortran.dg/char_array_structure_constructor.f90: Add
-fwhole-file as dg-option as it otherwise fails on some
systems.
Index: gcc/fortran/trans-types.c
===================================================================
--- gcc/fortran/trans-types.c (revision 162410)
+++ gcc/fortran/trans-types.c (working copy)
@@ -1882,8 +1882,8 @@ gfc_add_field_to_struct (tree context, t
the two derived type symbols are "equal", as described
in 4.4.2 and resolved by gfc_compare_derived_types. */
-static int
-copy_dt_decls_ifequal (gfc_symbol *from, gfc_symbol *to,
+int
+gfc_copy_dt_decls_ifequal (gfc_symbol *from, gfc_symbol *to,
bool from_gsym)
{
gfc_component *to_cm;
@@ -1994,7 +1994,7 @@ gfc_get_derived_type (gfc_symbol * deriv
gfc_find_symbol (derived->name, gsym->ns, 0, &s);
if (s && s->backend_decl)
{
- copy_dt_decls_ifequal (s, derived, true);
+ gfc_copy_dt_decls_ifequal (s, derived, true);
goto copy_derived_types;
}
}
@@ -2014,7 +2014,7 @@ gfc_get_derived_type (gfc_symbol * deriv
dt = ns->derived_types;
for (; dt && !canonical; dt = dt->next)
{
- copy_dt_decls_ifequal (dt->derived, derived, true);
+ gfc_copy_dt_decls_ifequal (dt->derived, derived, true);
if (derived->backend_decl)
got_canonical = true;
}
@@ -2181,7 +2181,7 @@ gfc_get_derived_type (gfc_symbol * deriv
copy_derived_types:
for (dt = gfc_derived_types; dt; dt = dt->next)
- copy_dt_decls_ifequal (derived, dt->derived, false);
+ gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
return derived->backend_decl;
}
Index: gcc/fortran/trans-types.h
===================================================================
--- gcc/fortran/trans-types.h (revision 162410)
+++ gcc/fortran/trans-types.h (working copy)
@@ -64,6 +64,7 @@ tree gfc_get_character_type_len_for_elty
tree gfc_sym_type (gfc_symbol *);
tree gfc_typenode_for_spec (gfc_typespec *);
+int gfc_copy_dt_decls_ifequal (gfc_symbol *, gfc_symbol *, bool);
tree gfc_get_function_type (gfc_symbol *);
Index: gcc/fortran/trans-decl.c
===================================================================
--- gcc/fortran/trans-decl.c (revision 162410)
+++ gcc/fortran/trans-decl.c (working copy)
@@ -1128,11 +1128,9 @@ gfc_get_symbol_decl (gfc_symbol * sym)
return sym->backend_decl;
/* If use associated and whole file compilation, use the module
- declaration. This is only needed for intrinsic types because
- they are substituted for one another during optimization. */
+ declaration. */
if (gfc_option.flag_whole_file
&& sym->attr.flavor == FL_VARIABLE
- && sym->ts.type != BT_DERIVED
&& sym->attr.use_assoc
&& sym->module)
{
@@ -1146,6 +1144,9 @@ gfc_get_symbol_decl (gfc_symbol * sym)
gfc_find_symbol (sym->name, gsym->ns, 0, &s);
if (s && s->backend_decl)
{
+ if (sym->ts.type == BT_DERIVED)
+ gfc_copy_dt_decls_ifequal (s->ts.u.derived, sym->ts.u.derived,
+ true);
if (sym->ts.type == BT_CHARACTER)
sym->ts.u.cl->backend_decl = s->ts.u.cl->backend_decl;
return s->backend_decl;
Index: gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90
===================================================================
--- gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (revision 162410)
+++ gcc/testsuite/gfortran.dg/char_array_structure_constructor.f90 (working copy)
@@ -1,4 +1,9 @@
! { dg-do run }
+! { dg-options "-fwhole-file" }
+!
+! PR fortran/19107
+! -fwhole-file flag added for PR fortran/44945
+!
! This test the fix of PR19107, where character array actual
! arguments in derived type constructors caused an ICE.
! It also checks that the scalar counterparts are OK.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
2010-07-22 13:59 ` Tobias Burnus
@ 2010-07-22 14:09 ` Tobias Burnus
2010-07-23 19:03 ` Paul Richard Thomas
1 sibling, 0 replies; 6+ messages in thread
From: Tobias Burnus @ 2010-07-22 14:09 UTC (permalink / raw)
To: Paul Richard Thomas; +Cc: gcc patches, gfortran
On 07/22/2010 03:59 PM, Tobias Burnus wrote:
> Build and regtested with -fwhole-file enabled by default. (Without the
> code path is not taken.) I do not see any ICEs in the test suite, found
> one bug relating -fwhole-file and ENTRY (PR 45030), and several failures
> due to do changed error/warning messages.
>
I forget to mention: Only 23 tests fail, which is actually a quite low
number and thus manageable when enabling -fwhole-file by default.
Of the failures, two are due to the ENTRY bug:
libgomp.fortran/retval1.f90
gfortran.fortran-torture/execute/entry_4.f90
And the rest seem to be either changed error messages, new warnings,
"missing" dumps (for some reasons before there were "ERROR:" + dumps,
now the dump is lacking in case of "ERROR:"), or test suite bugs
(mostly: mixing data types due to missing or wrong function return
declarations or argument mismatch):
gfortran.dg/bounds_check_strlen_1.f90
gfortran.dg/bounds_temporaries_1.f90
gfortran.dg/common_resize_1.f
gfortran.dg/entry_17.f90
gfortran.dg/func_decl_4.f90
gfortran.dg/g77/19990218-0.f
gfortran.dg/g77/19990218-1.f
gfortran.dg/g77/970625-2.f
gfortran.dg/generic_actual_arg.f90
gfortran.dg/global_references_1.f90
gfortran.dg/hollerith.f90
gfortran.dg/hollerith_legacy.f90
gfortran.dg/integer_exponentiation_2.f90
gfortran.dg/intrinsic_std_1.f90
gfortran.dg/loc_1.f90
gfortran.dg/pr20865.f90
gfortran.dg/pr37243.f
gfortran.dg/sizeof.f90
gfortran.dg/used_before_typed_4.f90
gfortran.dg/use_only_1.f90
libgomp.fortran/appendix-a/a.28.5.f90
Tobias
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression")
2010-07-22 13:59 ` Tobias Burnus
2010-07-22 14:09 ` Tobias Burnus
@ 2010-07-23 19:03 ` Paul Richard Thomas
1 sibling, 0 replies; 6+ messages in thread
From: Paul Richard Thomas @ 2010-07-23 19:03 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc patches, gfortran
Tobias,
>> OK for trunk and 4.5 after a delay of a week or two.
This is OK for trunk and 4.5 after the dust has settled - thanks for
seeing to it.
Cheers
Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-23 19:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-16 14:05 [Patch,Fortran] PR 44945 - Fix DECL of module variables (wrong-code "regression") Tobias Burnus
2010-07-16 14:40 ` Paul Richard Thomas
2010-07-22 13:59 ` Tobias Burnus
2010-07-22 14:09 ` Tobias Burnus
2010-07-23 19:03 ` Paul Richard Thomas
2010-07-16 14:48 ` Jakub Jelinek
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).