public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
@ 2013-01-04 23:31 Tobias Burnus
  2013-01-07 14:14 ` Tobias Burnus
  2013-02-13 18:20 ` Tobias Burnus
  0 siblings, 2 replies; 6+ messages in thread
From: Tobias Burnus @ 2013-01-04 23:31 UTC (permalink / raw)
  To: gcc patches, gfortran

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

This patch "removes" -fno-whole-file. (Actually, it turns it into "Ignore".)


Reasoning:

* -fwhole-file/-fno-whole-file was added in 4.5 to make the transition 
easier; -fwhole-file is the default since 4.6.

* There are many wrong-code issues and probably also some ICEs with 
-fno-whole file.

* The generated code of -fwhole-file is faster as it allows for inlining.

* -fno-whole-file has been deprecated since 4.6 and announced for removal.

* Code cleanup is always nice (diff -w): 17 insertions(+), 80 deletions(-)


Build and regtested on x86-64-gnu-linux.
OK for the trunk?


Tobias

PS: Dominique pointed out that PR 45128 is a -fwhole-file "regression". 
However, it mainly shows that gfortran needs the new array descriptor to 
fix such subpointer issues (and other PRs).

[-- Attachment #2: remove-whole-file.diff --]
[-- Type: text/x-patch, Size: 12581 bytes --]

2013-01-05  Tobias Burnus  <burnus@net-b.de>

	* gfortran.h (gfc_option_t): Remove flag_whole_file.
	* invoke.texi (-fno-whole-file): Remove.
	* lang.opt (fwhole-file): Change to Ignore.
	* options.c (gfc_init_options, gfc_post_options,
	gfc_handle_option): Remove !flag_whole_file handling
	* parse.c (resolve_all_program_units, translate_all_program_units,
	gfc_parse_file): Ditto.
	* resolve.c (resolve_global_procedure): Ditto.
	* trans-decl.c (gfc_get_symbol_decl, gfc_get_extern_function_decl,
	gfc_create_module_variable): Ditto.
	* trans-types.c (gfc_get_derived_type): Ditto.

diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 027cab6..b87bf64 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -2287,7 +2287,6 @@ typedef struct
   int flag_init_character;
   char flag_init_character_value;
   int flag_align_commons;
-  int flag_whole_file;
   int flag_protect_parens;
   int flag_realloc_lhs;
   int flag_aggressive_function_elimination;
diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi
index d7c3219..4ba94e5 100644
--- a/gcc/fortran/invoke.texi
+++ b/gcc/fortran/invoke.texi
@@ -182,7 +182,7 @@ and warnings}.
 -finit-real=@var{<zero|inf|-inf|nan|snan>} @gol
 -fmax-array-constructor=@var{n} -fmax-stack-var-size=@var{n}
 -fno-align-commons @gol
--fno-automatic -fno-protect-parens -fno-underscoring -fno-whole-file @gol
+-fno-automatic -fno-protect-parens -fno-underscoring @gol
 -fsecond-underscore -fpack-derived -frealloc-lhs -frecursive @gol
 -frepack-arrays -fshort-enums -fstack-arrays
 }
@@ -1293,22 +1293,6 @@ in the source, even if the names as seen by the linker are mangled to
 prevent accidental linking between procedures with incompatible
 interfaces.
 
-@item -fno-whole-file
-@opindex @code{fno-whole-file}
-This flag causes the compiler to resolve and translate each procedure in
-a file separately. 
-
-By default, the whole file is parsed and placed in a single front-end tree.
-During resolution, in addition to all the usual checks and fixups, references
-to external procedures that are in the same file effect resolution of
-that procedure, if not already done, and a check of the interfaces.  The
-dependences are resolved by changing the order in which the file is
-translated into the backend tree.  Thus, a procedure that is referenced
-is translated before the reference and the duplication of backend tree
-declarations eliminated.
-
-The @option{-fno-whole-file} option is deprecated and may lead to wrong code.
-
 @item -fsecond-underscore
 @opindex @code{fsecond-underscore}
 @cindex underscore
diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt
index 1535187..c885ff3 100644
--- a/gcc/fortran/lang.opt
+++ b/gcc/fortran/lang.opt
@@ -591,8 +591,8 @@ Fortran
 Append underscores to externally visible names
 
 fwhole-file
-Fortran
-Compile all program units at once and check all interfaces
+Fortran Ignore
+Does nothing.  Preserved for backward compatibility.
 
 fworking-directory
 Fortran
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index e05b935..1b2373b 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -126,7 +126,6 @@ gfc_init_options (unsigned int decoded_options_count,
   gfc_option.flag_real8_kind = 0;
   gfc_option.flag_dollar_ok = 0;
   gfc_option.flag_underscoring = 1;
-  gfc_option.flag_whole_file = 1;
   gfc_option.flag_f2c = 0;
   gfc_option.flag_second_underscore = -1;
   gfc_option.flag_implicit_none = 0;
@@ -266,14 +265,6 @@ gfc_post_options (const char **pfilename)
     sorry ("-fexcess-precision=standard for Fortran");
   flag_excess_precision_cmdline = EXCESS_PRECISION_FAST;
 
-  /* Whole program needs whole file mode.  */
-  if (flag_whole_program)
-    gfc_option.flag_whole_file = 1;
-
-  /* Enable whole-file mode if LTO is in effect.  */
-  if (flag_lto)
-    gfc_option.flag_whole_file = 1;
-
   /* Fortran allows associative math - but we cannot reassociate if
      we want traps or signed zeros. Cf. also flag_protect_parens.  */
   if (flag_associative_math == -1)
@@ -432,9 +423,6 @@ gfc_post_options (const char **pfilename)
       gfc_option.warn_tabs = 0;
     }
 
-  if (pedantic && gfc_option.flag_whole_file)
-    gfc_option.flag_whole_file = 2;
-
   /* Optimization implies front end optimization, unless the user
      specified it directly.  */
 
@@ -825,10 +813,6 @@ gfc_handle_option (size_t scode, const char *arg, int value,
       gfc_option.flag_underscoring = value;
       break;
 
-    case OPT_fwhole_file:
-      gfc_option.flag_whole_file = value;
-      break;
-
     case OPT_fsecond_underscore:
       gfc_option.flag_second_underscore = value;
       break;
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 659e9fc..5f2c33b 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -4379,8 +4379,7 @@ add_global_program (void)
 }
 
 
-/* Resolve all the program units when whole file scope option
-   is active. */
+/* Resolve all the program units. */
 static void
 resolve_all_program_units (gfc_namespace *gfc_global_ns_list)
 {
@@ -4421,9 +4420,8 @@ clean_up_modules (gfc_gsymbol *gsym)
 }
 
 
-/* Translate all the program units when whole file scope option
-   is active. This could be in a different order to resolution if
-   there are forward references in the file.  */
+/* Translate all the program units. This could be in a different order
+   to resolution if there are forward references in the file.  */
 static void
 translate_all_program_units (gfc_namespace *gfc_global_ns_list,
 			     bool main_in_tu)
@@ -4548,8 +4546,7 @@ loop:
       accept_statement (st);
       add_global_program ();
       parse_progunit (ST_NONE);
-      if (gfc_option.flag_whole_file)
-	goto prog_units;
+      goto prog_units;
       break;
 
     case ST_SUBROUTINE:
@@ -4557,8 +4554,7 @@ loop:
       push_state (&s, COMP_SUBROUTINE, gfc_new_block);
       accept_statement (st);
       parse_progunit (ST_NONE);
-      if (gfc_option.flag_whole_file)
-	goto prog_units;
+      goto prog_units;
       break;
 
     case ST_FUNCTION:
@@ -4566,8 +4562,7 @@ loop:
       push_state (&s, COMP_FUNCTION, gfc_new_block);
       accept_statement (st);
       parse_progunit (ST_NONE);
-      if (gfc_option.flag_whole_file)
-	goto prog_units;
+      goto prog_units;
       break;
 
     case ST_BLOCK_DATA:
@@ -4594,8 +4589,7 @@ loop:
       push_state (&s, COMP_PROGRAM, gfc_new_block);
       main_program_symbol (gfc_current_ns, "MAIN__");
       parse_progunit (st);
-      if (gfc_option.flag_whole_file)
-	goto prog_units;
+      goto prog_units;
       break;
     }
 
@@ -4612,19 +4606,9 @@ loop:
   if (s.state == COMP_MODULE)
     {
       gfc_dump_module (s.sym->name, errors_before == errors);
-      if (!gfc_option.flag_whole_file)
-	{
-	  if (errors == 0)
-	    gfc_generate_module_code (gfc_current_ns);
-	  pop_state ();
-	  gfc_done_2 ();
-	}
-      else
-	{
-	  gfc_current_ns->derived_types = gfc_derived_types;
-	  gfc_derived_types = NULL;
-	  goto prog_units;
-	}
+      gfc_current_ns->derived_types = gfc_derived_types;
+      gfc_derived_types = NULL;
+      goto prog_units;
     }
   else
     {
@@ -4657,9 +4641,6 @@ prog_units:
 
   done:
 
-  if (!gfc_option.flag_whole_file)
-    goto termination;
-
   /* Do the resolution.  */
   resolve_all_program_units (gfc_global_ns_list);
 
@@ -4678,8 +4659,6 @@ prog_units:
   /* Do the translation.  */
   translate_all_program_units (gfc_global_ns_list, seen_program);
 
-termination:
-
   gfc_end_source_files ();
   return SUCCESS;
 
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 54ac3c6..72e0b0a 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -2153,15 +2153,14 @@ resolve_global_procedure (gfc_symbol *sym, locus *where,
   if ((gsym->type != GSYM_UNKNOWN && gsym->type != type))
     gfc_global_used (gsym, where);
 
-  if (gfc_option.flag_whole_file
-	&& (sym->attr.if_source == IFSRC_UNKNOWN
-	    || sym->attr.if_source == IFSRC_IFBODY)
-	&& gsym->type != GSYM_UNKNOWN
-	&& gsym->ns
-	&& gsym->ns->resolved != -1
-	&& gsym->ns->proc_name
-	&& not_in_recursive (sym, gsym->ns)
-	&& not_entry_self_reference (sym, gsym->ns))
+  if ((sym->attr.if_source == IFSRC_UNKNOWN
+       || sym->attr.if_source == IFSRC_IFBODY)
+      && gsym->type != GSYM_UNKNOWN
+      && gsym->ns
+      && gsym->ns->resolved != -1
+      && gsym->ns->proc_name
+      && not_in_recursive (sym, gsym->ns)
+      && not_entry_self_reference (sym, gsym->ns))
     {
       gfc_symbol *def_sym;
 
@@ -2372,7 +2371,7 @@ resolve_global_procedure (gfc_symbol *sym, locus *where,
 		     "an explicit interface", sym->name, &sym->declared_at);
 	}
 
-      if (gfc_option.flag_whole_file == 1
+      if (!pedantic
 	  || ((gfc_option.warn_std & GFC_STD_LEGACY)
 	      && !(gfc_option.warn_std & GFC_STD_GNU)))
 	gfc_errors_to_warnings (1);
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
index 88f9c56..36b3a29 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -1340,15 +1340,14 @@ gfc_get_symbol_decl (gfc_symbol * sym)
       && sym->attr.flavor == FL_PARAMETER)
     intrinsic_array_parameter = true;
 
-  /* If use associated and whole file compilation, use the module
+  /* If use associated compilation, use the module
      declaration.  */
-  if (gfc_option.flag_whole_file
-	&& (sym->attr.flavor == FL_VARIABLE
-	    || sym->attr.flavor == FL_PARAMETER)
-	&& sym->attr.use_assoc
-	&& !intrinsic_array_parameter
-	&& sym->module
-	&& gfc_get_module_backend_decl (sym))
+  if ((sym->attr.flavor == FL_VARIABLE
+       || sym->attr.flavor == FL_PARAMETER)
+      && sym->attr.use_assoc
+      && !intrinsic_array_parameter
+      && sym->module
+      && gfc_get_module_backend_decl (sym))
     {
       if (sym->ts.type == BT_CLASS && sym->backend_decl)
 	GFC_DECL_CLASS(sym->backend_decl) = 1;
@@ -1638,12 +1637,11 @@ gfc_get_extern_function_decl (gfc_symbol * sym)
      return the backend_decl.  */
   gsym =  gfc_find_gsymbol (gfc_gsym_root, sym->name);
 
-  if (gfc_option.flag_whole_file
-	&& (!sym->attr.use_assoc || sym->attr.if_source != IFSRC_DECL)
-	&& !sym->backend_decl
-	&& gsym && gsym->ns
-	&& ((gsym->type == GSYM_SUBROUTINE) || (gsym->type == GSYM_FUNCTION))
-	&& (gsym->ns->proc_name->backend_decl || !sym->attr.intrinsic))
+  if ((!sym->attr.use_assoc || sym->attr.if_source != IFSRC_DECL)
+      && !sym->backend_decl
+      && gsym && gsym->ns
+      && ((gsym->type == GSYM_SUBROUTINE) || (gsym->type == GSYM_FUNCTION))
+      && (gsym->ns->proc_name->backend_decl || !sym->attr.intrinsic))
     {
       if (!gsym->ns->proc_name->backend_decl)
 	{
@@ -1695,9 +1693,7 @@ gfc_get_extern_function_decl (gfc_symbol * sym)
   if (sym->module)
     gsym =  gfc_find_gsymbol (gfc_gsym_root, sym->module);
 
-  if (gfc_option.flag_whole_file
-	&& gsym && gsym->ns
-	&& gsym->type == GSYM_MODULE)
+  if (gsym && gsym->ns && gsym->type == GSYM_MODULE)
     {
       gfc_symbol *s;
 
@@ -4035,8 +4031,7 @@ gfc_create_module_variable (gfc_symbol * sym)
       decl = sym->backend_decl;
       gcc_assert (sym->ns->proc_name->attr.flavor == FL_MODULE);
 
-      /* -fwhole-file mixes up the contexts so these asserts are unnecessary.  */
-      if (!(gfc_option.flag_whole_file && sym->attr.use_assoc))
+      if (!sym->attr.use_assoc)
 	{
 	  gcc_assert (TYPE_CONTEXT (decl) == NULL_TREE
 		      || TYPE_CONTEXT (decl) == sym->ns->proc_name->backend_decl);
diff --git a/gcc/fortran/trans-types.c b/gcc/fortran/trans-types.c
index 8394bf9..52ca23e 100644
--- a/gcc/fortran/trans-types.c
+++ b/gcc/fortran/trans-types.c
@@ -2372,19 +2372,16 @@ gfc_get_derived_type (gfc_symbol * derived)
     }
 
   /* If use associated, use the module type for this one.  */
-  if (gfc_option.flag_whole_file
-	&& derived->backend_decl == NULL
-	&& derived->attr.use_assoc
-	&& derived->module
-	&& gfc_get_module_backend_decl (derived))
+  if (derived->backend_decl == NULL
+      && derived->attr.use_assoc
+      && derived->module
+      && gfc_get_module_backend_decl (derived))
     goto copy_derived_types;
 
-  /* If a whole file compilation, the derived types from an earlier
-     namespace can be used as the canonical type.  */
-  if (gfc_option.flag_whole_file
-	&& derived->backend_decl == NULL
-	&& !derived->attr.use_assoc
-	&& gfc_global_ns_list)
+  /* The derived types from an earlier namespace can be used as the
+     canonical type.  */
+  if (derived->backend_decl == NULL && !derived->attr.use_assoc
+      && gfc_global_ns_list)
     {
       for (ns = gfc_global_ns_list;
 	   ns->translated && !got_canonical;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
  2013-01-04 23:31 [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file Tobias Burnus
@ 2013-01-07 14:14 ` Tobias Burnus
  2013-02-13 18:20 ` Tobias Burnus
  1 sibling, 0 replies; 6+ messages in thread
From: Tobias Burnus @ 2013-01-07 14:14 UTC (permalink / raw)
  To: gcc patches, gfortran

Early * ping * the patch below, i.e. 
http://gcc.gnu.org/ml/fortran/2013-01/msg00033.html

Other pending patches by me:
http://gcc.gnu.org/ml/fortran/2013-01/msg00049.html
http://gcc.gnu.org/ml/fortran/2013-01/msg00025.html

Other pending patches by …
Thomas: http://gcc.gnu.org/ml/fortran/2013-01/msg00000.html
Janne: http://gcc.gnu.org/ml/fortran/2013-01/msg00037.html

Tobias Burnus wrote:
> This patch "removes" -fno-whole-file. (Actually, it turns it into 
> "Ignore".)
>
>
> Reasoning:
>
> * -fwhole-file/-fno-whole-file was added in 4.5 to make the transition 
> easier; -fwhole-file is the default since 4.6.
>
> * There are many wrong-code issues and probably also some ICEs with 
> -fno-whole file.
>
> * The generated code of -fwhole-file is faster as it allows for inlining.
>
> * -fno-whole-file has been deprecated since 4.6 and announced for 
> removal.
>
> * Code cleanup is always nice (diff -w): 17 insertions(+), 80 
> deletions(-)
>
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?
>
>
> Tobias
>
> PS: Dominique pointed out that PR 45128 is a -fwhole-file 
> "regression". However, it mainly shows that gfortran needs the new 
> array descriptor to fix such subpointer issues (and other PRs).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
  2013-01-04 23:31 [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file Tobias Burnus
  2013-01-07 14:14 ` Tobias Burnus
@ 2013-02-13 18:20 ` Tobias Burnus
  2013-02-13 19:29   ` Steve Kargl
  1 sibling, 1 reply; 6+ messages in thread
From: Tobias Burnus @ 2013-02-13 18:20 UTC (permalink / raw)
  To: gcc patches, gfortran

*** PING ***

I think it is now a bit late for 4.8. Thus, I change my request to: OK 
for the 4.9 trunk?

Tobias

On January 5, 2013 00:31, Tobias Burnus wrote:
> This patch "removes" -fno-whole-file. (Actually, it turns it into 
> "Ignore".)
>
>
> Reasoning:
>
> * -fwhole-file/-fno-whole-file was added in 4.5 to make the transition 
> easier; -fwhole-file is the default since 4.6.
>
> * There are many wrong-code issues and probably also some ICEs with 
> -fno-whole file.
>
> * The generated code of -fwhole-file is faster as it allows for inlining.
>
> * -fno-whole-file has been deprecated since 4.6 and announced for 
> removal.
>
> * Code cleanup is always nice (diff -w): 17 insertions(+), 80 
> deletions(-)
>
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?
>
>
> Tobias
>
> PS: Dominique pointed out that PR 45128 is a -fwhole-file 
> "regression". However, it mainly shows that gfortran needs the new 
> array descriptor to fix such subpointer issues (and other PRs).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
  2013-02-13 18:20 ` Tobias Burnus
@ 2013-02-13 19:29   ` Steve Kargl
  0 siblings, 0 replies; 6+ messages in thread
From: Steve Kargl @ 2013-02-13 19:29 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: gcc patches, gfortran

On Wed, Feb 13, 2013 at 07:20:10PM +0100, Tobias Burnus wrote:
> *** PING ***
> 
> I think it is now a bit late for 4.8. Thus, I change my request to: OK 
> for the 4.9 trunk?
> 

IMHO, yes.  Don't know if others have an opinion, but
waiting any longer would seem to be counter productive.

-- 
Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
  2013-01-07 16:22 Dominique Dhumieres
@ 2013-01-07 18:23 ` Tobias Burnus
  0 siblings, 0 replies; 6+ messages in thread
From: Tobias Burnus @ 2013-01-07 18:23 UTC (permalink / raw)
  To: Dominique Dhumieres; +Cc: fortran, gcc-patches, pault

Dominique Dhumieres wrote:
> (1) Is there any hint of -fno-whole-file misbehavior (as suggested by the
> second point of the "Reasoning")?

The middle end is simply not prepared for having multiple declaration 
for a single file ("translation unit"), which leads to wrong 
optimizations and thus to wrong code.

I think the bug which triggeded my decision to fix all remaining known 
-fno-whole-file issues and then to enable it by default  was PR 44945. 
See also http://gcc.gnu.org/ml/fortran/2010-07/msg00321.html

But there are surely more. Using multiple declarations was simply a 
design bug in the original implementation of gfortran, which was only 
realized rather late - and then deferred. Paul was that brave to fix 
this by implementing -fwhole-file.

> As said in my comment below, if the answer is yes, then IMO this should be
> investigated and fixed before removal.

We did the investigation when switching to -fwhole-file by default. And 
no, I do not want to investigate why the middle end "misbehaves" with 
multiple declarations in a single translation unit - nor do I want to 
fix it. I am content if everything works with -fwhole-file. There is 
really not any advantage of having multiple declarations in a single TU.

> (2) Is the -fwhole-file status ZKB (zero known bug)?
> I am not 100% confident that this the case (see at least PR 45128).

I am not aware of any bug since 4.6. I am rather certain that we fixed 
all known bugs in this area. Neither having remaining mult-decls nor 
issues which wouldn't occur with -fno-whole-file. By now it simply makes 
more sense to fix a bug, ignoring whether it worked by chance with 
-fno-whole-file or not. All recent regressions seem to be unrelated. 
Thus, for users it makes more sense to use an older version or wait for 
the patch than to try -fno-while-file. Given the potential to generate 
wrong-code bugs, I wouldn't trust -fno-whole-file for any code which 
goes beyond a hello-world example.

PR45128 requires the new array descriptor; we have a kludge which allows 
sub-type pointer in some cases. Seemingly, that kludge works in one case 
with -fno-whole-file where it fails with -fwhole-file. But I will 
actively ignore all subpointer PRs until the new array descriptor is 
ready, which will be the right fix.

> (3) What is the confidence that there is no rampant bug in -fwhole-file?

Well, even if there is a bug which doesn't affect -fno-whole-file, using 
-fno-whole-file is no solution, given that -fno-whole-file has many 
known bugs.

And while I am willing to fix -fwhole-file bugs, I will ignore any 
-fno-whole-file ones.

> (4) Is there an easy replacement for -fno-whole-file?

Yes: -fwhole-file :-)  Really, you shouldn't think of -fno-whole-file 
anymore.

> Here the answer is clearly yes: put the different TUs in different files.

Or doing that ;-)

> My understanding of -fno-whole-file as well as -fno-realloc-lhs,
> -fno-frontend-optimize, or any of the -fno-* options is two fold:
>
> (i) to provide a workaround to user hit by a bug in the corresponding area,
> (ii) to help the maintainers to locate the bug or to change the default in
> case of a too severe bug.

Well, the main point of -fno-realloc-lhs is to disable the performance 
penalty of RHS. That is also helped to find some bugs was a side effect. 
For -fno-frontend-optimize I agree that its purpose was indeed to toggle 
the optimization separately from only using -O0 vs -O1.

But there is a difference between -frealloc-lhs, -ffrontend-optimize and 
-fwhole-file: The former two enable a specific feature or optimization 
and valid programs can (should) be produced with either version. By 
contrast, in terms of middle-end assumptions -fno-whole-file is always 
wrong.

I think (ii) does not really apply. We never changed the defaults when a 
serious bug popped up, we always fixed it. [Though we might have 
suggested a command-line option as workaround, cf. (i).] For 
-fno-whole-file, it also didn't help much with localizing the bug. For 
-f(no-)frontend-optimize I agree that it helped several times to isolate 
the bug.

>> * There are many wrong-code issues and probably also some ICEs with
>> -fno-whole file.
> I think the wording is misleading. I have checked bugzilla for open PR
> containing -fno-whole-file and I have found only 2 (48939 and 52621).
> None of them are related to wrong-code issue due to -fno-whole-file.

You have to search for mult-decl issues. -fwhole-file was the cure of 
that problem; as it had a couple of issues before July 2010 and wasn't 
enabled by default, almost no one used it. Hence, you don't find it in 
the log. And after -fwhole-file became the default, no one cared anymore 
about issues which occurred only with -fno-whole-file.

> My understanding of the -fno-whole-file option is that it is (should be)
> strictly equivalent to put all the TUs in separate files. If it exists an
> example for which this is not true, I think it would be a bug

But that's not how the middle end works. It expects that in a single 
file (translation unit), no multiple declarations for the same object exist.

We do not want to change the middle end. Besides, having a single decl 
per object allows for inlining and similar optimization. There is simply 
no reason for -fno-whole-file except that gfortran until 4.5 had it by 
default.

In principle, we could have done the switch without adding 
-f(no-)whole-file; having the flag was only useful because: (i) It 
allowed Paul to sensible put the patch into the trunk even though it was 
not yet completely working. (ii) It allowed to find out that PR44945 was 
only failing with -fno-whole-file. (iii) Mainly in 2010-07, it made it 
easier to find regressions in -fwhole-file. After 2010-07, most issues 
were solved, rendering -f(no-)whole-file less useful.

>> * The generated code of -fwhole-file is faster as it allows for inlining.
> This has nothing to do with the removal of -fno-whole-file.

No but with the addition of -fwhole-file. Without, inlining was 
essentially not possible.

> AFAICT most of the speeddup is achieved with -fwhole-program.

But that only works with a single file/TU, unless you use LTO. 
Additionally, -flto and -fwhole-program both enable -fwhole-file. Due to 
the multiple declarations issue, -fno-whole-file -fwhole-program would 
optimize many procedures away, which were actually used - leading to 
linking errors.

> OK, but no time table has been released so far.

In terms of the bugs, I was even considering to remove it for 4.6. I 
think by 2010-08 we had fixed nearly all -fwhole-file specific bugs - 
and no one wanted to go back to -fno-whole-file.

> Compiling PR 45128 with -fwhole-file gives an ICE: Segmentation fault, 
> while with -fno-whole-file it gives a linking error Undefined symbols: 
> "_span.0", referenced from: ... as it does if the two TUs are put is 
> different files. Dominique 

As said above: I will actively ignore subpointer bugs! Actually, before 
I thought that -fno-whole-file worked for that file, but as the linker 
error indicates, -fno-whole-file also doesn't work!

Tobias

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file
@ 2013-01-07 16:22 Dominique Dhumieres
  2013-01-07 18:23 ` Tobias Burnus
  0 siblings, 1 reply; 6+ messages in thread
From: Dominique Dhumieres @ 2013-01-07 16:22 UTC (permalink / raw)
  To: fortran; +Cc: gcc-patches, pault, burnus

Sorry for the late and lengthy answer.  To make the story short, I think
the decision to remove -fno-whole-file should be based on the answer to the
following questions:

(1) Is there any hint of -fno-whole-file misbehavior (as suggested by the 
second point of the "Reasoning")?

As said in my comment below, if the answer is yes, then IMO this should be 
investigated and fixed before removal.

(2) Is the -fwhole-file status ZKB (zero known bug)?

I am not 100% confident that this the case (see at least PR 45128).

(3) What is the confidence that there is no rampant bug in -fwhole-file?

While I cannot remember any recent PR in this area (thanks Paul), at the
same time I did not see recent reports (in buzilla or in comp.lang.fortran)
about the common errors which have plagued the f77 to f90 transition.

(4) Is there an easy replacement for -fno-whole-file?

Here the answer is clearly yes: put the different TUs in different files.

So to summarize, I think the key point is the answer to the first 
question. I think the answer is no, but this was not what I understood 
from the original post.

> This patch "removes" -fno-whole-file.  (Actually, it turns it into
> "Ignore".)
>
> Reasoning:
> 
> * -fwhole-file/-fno-whole-file was added in 4.5 to make the transition
> easier; -fwhole-file is the default since 4.6.

My understanding of -fno-whole-file as well as -fno-realloc-lhs, 
-fno-frontend-optimize, or any of the -fno-* options is two fold:

(i) to provide a workaround to user hit by a bug in the corresponding area,
(ii) to help the maintainers to locate the bug or to change the default in 
case of a too severe bug.

So the decision to remove an option should be based on the answer to the 
questions (2) and (3) above.

> * There are many wrong-code issues and probably also some ICEs with
> -fno-whole file.

I think the wording is misleading. I have checked bugzilla for open PR 
containing -fno-whole-file and I have found only 2 (48939 and 52621).
None of them are related to wrong-code issue due to -fno-whole-file.

My understanding of the -fno-whole-file option is that it is (should be) 
strictly equivalent to put all the TUs in separate files. If it exists an 
example for which this is not true, I think it would be a bug, a PR should 
be filled for it, and indeed, the removal of -fno-whole-file should be 
delayed after it the bug(s) is (are) fixed.

If the meaning of the quoted sentence is that -fwhole-file gives errors 
for invalid code (missing interface, argument mismatches, ...), hence 
prevents "wrong-code" issues, this is true. However such errors can only 
be catched if all the involved TUs are in the same file, e.g. I won't get 
any error if I mess up the arguments of a subroutine in the lapack library 
unless I add it (and its dependencies) to my source.

> * The generated code of -fwhole-file is faster as it allows for inlining.

This has nothing to do with the removal of -fno-whole-file.
AFAICT most of the speeddup is achieved with -fwhole-program.
In addition the code has to be in a single file. On platforms with the 
right linker and the right plugin (i.e. not Darwin) the speedup for split 
files can be obtained by compiling the different files with -flto.

> * -fno-whole-file has been deprecated since 4.6 and announced for
> removal.
>

OK, but no time table has been released so far.

> * Code cleanup is always nice (diff -w): 17 insertions(+), 80
> deletions(-)

OK
 
> Build and regtested on x86-64-gnu-linux.  
>
> OK for the trunk?
>
> PS: Dominique pointed out that PR 45128 is a -fwhole-file "regression".
> However, it mainly shows that gfortran needs the new array descriptor to
> fix such subpointer issues (and other PRs).

Compiling PR 45128 with -fwhole-file gives an ICE: Segmentation fault, 
while with -fno-whole-file it gives a linking error

Undefined symbols:
  "_span.0", referenced from:
...

as it does if the two TUs are put is different files.

Dominique

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-02-13 19:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-04 23:31 [Patch, Fortran] Remove the Fortran-only flag -fno-whole-file Tobias Burnus
2013-01-07 14:14 ` Tobias Burnus
2013-02-13 18:20 ` Tobias Burnus
2013-02-13 19:29   ` Steve Kargl
2013-01-07 16:22 Dominique Dhumieres
2013-01-07 18:23 ` Tobias Burnus

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).