Re: [PATCH] throttle down dataflow verification

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Paolo Bonzini wrote:
> The amount of verification that is done now is way too expensive to be
> the default forever.  On the other had, having all of the scanning
> verified after each pass has been quite useful to shake the bugs and
> will be useful for developers of the RTL part of the compiler.
>
> Therefore, a while ago the verification of dataflow was made
> conditional on --enable-checking=...,df,... and the new checking flag
> was made the default.  Now, it's time to remove it from the default
> and, at the same time, have some dataflow checking happen with
> --enable-checking too. This saves 4% on a bootstrap with default
> flags, and actually more can be saved.
>
> As to which passes to leave the checking on in the default level
> checking on, we chose after the loop optimizations and after prologue
> and epilogue generation would be natural places because both of these
> were very problematic when i was developing this stuff
>
> Prologue and epilogue generation because is a good
> place to find problems when someone has tampered with the machine
> descriptions, though in general Honza's RTL-sharing verifier will
> catch most md-related problems too.
>
> Zdenek's code is also one of the first users of DF, which makes it a
> little different sometimes from the other clients.  This could be
> removed though, saving further on a bootstrap.
>
> We also added it to see.c because if that code is ever activated, this
> will force the developer to get his/her act together.  That code is
> somewhat a mess and making it trigger quickly will help get the
> problems resolved.
>
> The register allocators are not really an issue, because it rescans
> everything after this anyway but it might be useful to verify right
> before the start of ra, and right after toggling reload_completed.
>
> Another thing that could be restricted to --enable-checking=df is the
> verification of transfer functions (so that only the verification of
> the scanning is active at --enable-checking=yes).
>
I think that i agree with this.  the only way to get failures with
either the transfer functions or the dataflow solutions is to use
unauthorized api's to either add or delete insns from a block or change
the cfg.

No place in the compiler does this except see.c (and it has been marked
with the sign of the devil) and with reasonable reviewing, we should be
able to keep this clean.
> A note: due to the architecture of dataflow, it's only possible to do
> verification after df_analyze.  To keep the behavior of the compiler
> as similar as possible between --enable-checking and
> --disable-checking, I'm only *scheduling* verification at the end of
> the above passes, leaving the actual verification to be done at the
> beginning of the next pass.
>
> Bootstrapped i686-pc-linux-gnu, ok for mainline?
>
> Paolo
>   
Ok for mainline.

thanks

kenny

> ------------------------------------------------------------------------
>
> 2008-08-05  Paolo Bonzini  <bonzini@gnu.org>
>
> 	* configure.ac: Remove --enable-checking=df from default settings.
> 	* tree-pass.h (TODO_df_verify): New.  Shift TODO_mark_first_instance.
> 	* df-core.c (df_finish_pass) [ENABLE_CHECKING]: Schedule verification
> 	if the parameter is true.
> 	(df_analyze) [!ENABLE_DF_CHECKING]: Also do verification if the
> 	DF_VERIFY_SCHEDULED flag is true.
> 	* df.h (enum df_changeable_flags): Add DF_VERIFY_SCHEDULED.
> 	(df_finish_pass): Adjust prototype.
> 	* passes.c (execute_todo): Schedule verification if TODO_df_verify is
> 	true.
>
> 	* see.c (pass_see): Add TODO_df_verify.
> 	* loop-init.c (pass_rtl_move_loop_invariants): Add TODO_df_verify.
> 	* global.c (rest_of_handle_global_alloc): Schedule verification
> 	after the pass.
> 	* local-alloc.c (rest_of_handle_local_alloc): Schedule verification
> 	before the pass.
> 	* function.c (pass_thread_prologue_and_epilogue): Add TODO_df_verify.
> 	* gcse.c (rest_of_handle_gcse): Adjust call to df_finish_pass.
> 	* loop-iv.c (iv_analysis_done): Schedule verification after the pass.
>
> 	* config/sh/sh.c (sh_output_mi_thunk): Remove dead code.
> 	* config/ia64/ia64.c (ia64_reorg): Adjust call to df_finish_pass.
> 	* config/bfin/bfin.c (bfin_reorg): Adjust call to df_finish_pass.
>
> Index: see.c
> ===================================================================
> --- see.c	(revision 127208)
> +++ see.c	(working copy)
> @@ -3831,6 +3831,7 @@ struct tree_opt_pass pass_see =
>    0,					/* properties_provided */
>    0,					/* properties_destroyed */
>    0,					/* todo_flags_start */
> +  TODO_df_verify |
>    TODO_df_finish |
>    TODO_dump_func,			/* todo_flags_finish */
>    'u'					/* letter */
> Index: tree-pass.h
> ===================================================================
> --- tree-pass.h	(revision 127208)
> +++ tree-pass.h	(working copy)
> @@ -222,8 +222,11 @@ struct dump_file_info
>     the instance before it is destroyed.  */
>  #define TODO_df_finish                  (1 << 16)
>  
> +/* Call df_verify at the end of the pass if checking is enabled.  */
> +#define TODO_df_verify                  (1 << 17)
> +
>  /* Internally used for the first instance of a pass.  */
> -#define TODO_mark_first_instance	(1 << 17)
> +#define TODO_mark_first_instance	(1 << 18)
>  
>  #define TODO_update_ssa_any		\
>      (TODO_update_ssa			\
> Index: df-core.c
> ===================================================================
> --- df-core.c	(revision 127208)
> +++ df-core.c	(working copy)
> @@ -79,7 +79,7 @@ Here is an example of using the dataflow
>  
>        df_dump (stderr);
>  
> -      df_finish_pass ();
> +      df_finish_pass (false);
>  
>  DF_[ru,rd,urec,ri,chain]_ADD_PROBLEM adds a problem, defined by an
>  instance to struct df_problem, to the set of problems solved in this
> @@ -633,7 +633,7 @@ df_remove_problem (struct dataflow *dflo
>     of the changeable_flags.  */
>  
>  void
> -df_finish_pass (void)
> +df_finish_pass (bool verify ATTRIBUTE_UNUSED)
>  {
>    int i;
>    int removed = 0;
> @@ -694,6 +694,11 @@ df_finish_pass (void)
>    df_set_clean_cfg ();
>  #endif
>  #endif
> +
> +#ifdef ENABLE_CHECKING
> +  if (verify)
> +    df->changeable_flags |= DF_VERIFY_SCHEDULED;
> +#endif
>  }
>  
>  
> @@ -1100,9 +1103,10 @@ df_analyze (void)
>    if (dump_file)
>      fprintf (dump_file, "df_analyze called\n");
>  
> -#ifdef ENABLE_DF_CHECKING
> -  df_verify ();
> -#endif 
> +#ifndef ENABLE_DF_CHECKING
> +  if (df->changeable_flags & DF_VERIFY_SCHEDULED)
> +#endif
> +    df_verify ();
>  
>    for (i = 0; i < df->n_blocks; i++)
>      bitmap_set_bit (current_all_blocks, df->postorder[i]);
> Index: loop-init.c
> ===================================================================
> --- loop-init.c	(revision 127208)
> +++ loop-init.c	(working copy)
> @@ -251,7 +251,8 @@ struct tree_opt_pass pass_rtl_move_loop_
>    0,                                    /* properties_provided */
>    0,                                    /* properties_destroyed */
>    0,                                    /* todo_flags_start */ 
> -  TODO_df_finish |                      /* This is shutting down the instance in loop_invariant.c  */
> +  TODO_df_verify |
> +  TODO_df_finish |
>    TODO_dump_func,                       /* todo_flags_finish */
>    'L'                                   /* letter */
>  };
> Index: global.c
> ===================================================================
> --- global.c	(revision 127208)
> +++ global.c	(working copy)
> @@ -2081,7 +2081,7 @@ rest_of_handle_global_alloc (void)
>       just rescan everything.  Not that df_rescan_all_insns is not
>       going to help here because it does not touch the artificial uses
>       and defs.  */
> -  df_finish_pass ();
> +  df_finish_pass (true);
>    if (optimize > 1)
>      df_live_add_problem ();
>    df_scan_alloc (NULL);
> Index: local-alloc.c
> ===================================================================
> --- local-alloc.c	(revision 127208)
> +++ local-alloc.c	(working copy)
> @@ -2515,6 +2515,9 @@ rest_of_handle_local_alloc (void)
>       we are doing optimization.  */
>    if (optimize)
>      df_urec_add_problem ();
> +#ifdef ENABLE_CHECKING
> +  df->changeable_flags |= DF_VERIFY_SCHEDULED;
> +#endif
>    df_analyze ();
>    regstat_init_n_sets_and_refs ();
>    regstat_compute_ri ();
> Index: configure.ac
> ===================================================================
> --- configure.ac	(revision 127208)
> +++ configure.ac	(working copy)
> @@ -359,7 +359,7 @@ for check in release $ac_checking_flags
>  do
>  	case $check in
>  	# these set all the flags to specific states
> -	yes)		ac_assert_checking=1 ; ac_checking=1 ; ac_df_checking=1 ;
> +	yes)		ac_assert_checking=1 ; ac_checking=1 ; ac_df_checking= ;
>  			ac_fold_checking= ; ac_gc_checking=1 ;
>  			ac_gc_always_collect= ; ac_rtl_checking= ;
>  			ac_rtlflag_checking=1 ; ac_runtime_checking=1 ;
> Index: function.c
> ===================================================================
> --- function.c	(revision 127208)
> +++ function.c	(working copy)
> @@ -5514,6 +5514,7 @@ struct tree_opt_pass pass_thread_prologu
>    0,                                    /* properties_destroyed */
>    TODO_verify_flow,                     /* todo_flags_start */
>    TODO_dump_func |
> +  TODO_df_verify |
>    TODO_df_finish |
>    TODO_ggc_collect,                     /* todo_flags_finish */
>    'w'                                   /* letter */
> Index: df.h
> ===================================================================
> --- df.h	(revision 127208)
> +++ df.h	(working copy)
> @@ -407,7 +407,9 @@ enum df_changeable_flags 
>    /* Cause df_insn_rescan df_notes_rescan and df_insn_delete, to
>    return after marking the insn for later processing.  This allows all
>    rescans to be batched.  */
> -  DF_DEFER_INSN_RESCAN    = 32
> +  DF_DEFER_INSN_RESCAN    = 32,
> +
> +  DF_VERIFY_SCHEDULED     = 64
>  };
>  
>  /* Two of these structures are inline in df, one for the uses and one
> @@ -807,7 +809,7 @@ extern enum df_changeable_flags df_set_f
>  extern enum df_changeable_flags df_clear_flags (enum df_changeable_flags);
>  extern void df_set_blocks (bitmap);
>  extern void df_remove_problem (struct dataflow *);
> -extern void df_finish_pass (void);
> +extern void df_finish_pass (bool);
>  extern void df_analyze_problem (struct dataflow *, bitmap, int *, int);
>  extern void df_analyze (void);
>  extern int df_get_n_blocks (enum df_flow_dir);
> Index: gcse.c
> ===================================================================
> --- gcse.c	(revision 127208)
> +++ gcse.c	(working copy)
> @@ -6706,7 +6706,7 @@ rest_of_handle_gcse (void)
>      {
>        timevar_push (TV_CSE);
>        tem2 = cse_main (get_insns (), max_reg_num ());
> -      df_finish_pass ();
> +      df_finish_pass (false);
>        purge_all_dead_edges ();
>        delete_trivially_dead_insns (get_insns (), max_reg_num ());
>        timevar_pop (TV_CSE);
> Index: loop-iv.c
> ===================================================================
> --- loop-iv.c	(revision 127208)
> +++ loop-iv.c	(working copy)
> @@ -1266,7 +1266,7 @@ iv_analysis_done (void)
>      {
>        clear_iv_info ();
>        clean_slate = true;
> -      df_finish_pass ();
> +      df_finish_pass (true);
>        htab_delete (bivs);
>        free (iv_ref_table);
>        iv_ref_table = NULL;
> Index: passes.c
> ===================================================================
> --- passes.c	(revision 127208)
> +++ passes.c	(working copy)
> @@ -1013,7 +1013,7 @@ execute_todo (unsigned int flags)
>    /* Now that the dumping has been done, we can get rid of the optional 
>       df problems.  */
>    if (flags & TODO_df_finish)
> -    df_finish_pass ();
> +    df_finish_pass ((flags & TODO_df_verify) != 0);
>  }
>  
>  /* Verify invariants that should hold between passes.  This is a place
> Index: config/sh/sh.c
> ===================================================================
> --- config/sh/sh.c	(revision 127208)
> +++ config/sh/sh.c	(working copy)
> @@ -10257,22 +10257,6 @@ sh_output_mi_thunk (FILE *file, tree thu
>    final (insns, file, 1);
>    final_end_function ();
>  
> -#if 0
> -  if (optimize > 0)
> -    {
> -      /* Release all memory allocated by df.  */
> -      if (rtl_df)
> -	{
> -	  df_finish (rtl_df);
> -	  rtl_df = NULL;
> -	}
> -
> -      /* Release the bitmap obstacks.  */
> -      bitmap_obstack_release (&reg_obstack);
> -      bitmap_obstack_release (NULL);
> -    }
> -#endif
> -
>    reload_completed = 0;
>    epilogue_completed = 0;
>  }
> Index: config/ia64/ia64.c
> ===================================================================
> --- config/ia64/ia64.c	(revision 127208)
> +++ config/ia64/ia64.c	(working copy)
> @@ -8625,7 +8625,7 @@ ia64_reorg (void)
>        variable_tracking_main ();
>        timevar_pop (TV_VAR_TRACKING);
>      }
> -  df_finish_pass ();
> +  df_finish_pass (false);
>  }
>  
>  /* Return true if REGNO is used by the epilogue.  */
> Index: config/bfin/bfin.c
> ===================================================================
> --- config/bfin/bfin.c	(revision 127208)
> +++ config/bfin/bfin.c	(working copy)
> @@ -4534,7 +4534,7 @@ bfin_reorg (void)
>        reorder_var_tracking_notes ();
>        timevar_pop (TV_VAR_TRACKING);
>      }
> -  df_finish_pass ();
> +  df_finish_pass (false);
>  }
>  
>  /* Handle interrupt_handler, exception_handler and nmi_handler function
>