On Tue, 1 Dec 2015, Alan Lawrence wrote:
> This follows on from discussion at
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03392.html
> To recap: Starting in r229479 and continuing until at least 229711, compiling
> polynom.c from spec2000 on aarch64-none-linux-gnu, with options
> -O3 -mcpu=cortex-a53 -ffast-math (on both cross, native bootstrapped, and
> native
> --disable-bootstrap compilers), produced a verify_gimple ICE after unswitch:
>
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c: In function
> 'NormalizeCoeffsListx':
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error:
> incompatible types in PHI argument 0
> TypHandle NormalizeCoeffsListx ( hdC )
> ^
> long int
>
> int
>
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location
> references block not in block tree
> l1_279 = PHI <1(28), l1_299(33)>
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: invalid
> PHI argument
>
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: internal
> compiler error: tree check: expected class 'type', have 'declaration'
> (namespace_decl) in useless_type_conversion_p, at gimple-expr.c:84
> 0xd110ef tree_class_check_failed(tree_node const*, tree_code_class, char
> const*, int, char const*)
> ../../gcc-fsf/gcc/tree.c:9643
> 0x82561b tree_class_check
> ../../gcc-fsf/gcc/tree.h:3042
> 0x82561b useless_type_conversion_p(tree_node*, tree_node*)
> ../../gcc-fsf/gcc/gimple-expr.c:84
> 0xaca043 verify_gimple_phi
> ../../gcc-fsf/gcc/tree-cfg.c:4673
> 0xaca043 verify_gimple_in_cfg(function*, bool)
> ../../gcc-fsf/gcc/tree-cfg.c:4967
> 0x9c2e0b execute_function_todo
> ../../gcc-fsf/gcc/passes.c:1967
> 0x9c360b do_per_function
> ../../gcc-fsf/gcc/passes.c:1659
> 0x9c3807 execute_todo
> ../../gcc-fsf/gcc/passes.c:2022
>
> I was not able to reduce the testcase below about 30k characters, with e.g.
> #define T_VOID 0
> .... T_VOID ....
> producing the ICE, but manually changing to
> .... 0 ....
> preventing the ICE; as did running the preprocessor as a separate step, or a
> wide variety of options (e.g. -fdump-tree-alias).
>
> In the end I traced this to loop_unswitch reading stale values from the edge
> redirect map, which is keyed on 'edge' (a pointer to struct edge_def); the map
> entries had been left there by pass_dominator (on a different function), and
> by
> "chance" the edge *pointers* were the same as to some current edge_defs (even
> though they pointed to structures created by different allocations, the first
> of which had since been freed). Hence the fragility of the testcase and
> environment.
>
> While the ICE is prevented merely by adding a call to
> redirect_edge_var_map_destroy at the end of pass_dominator::execute, given the
> fragility of the bug, difficulty of reducing the testcase, and the low
> overhead
> of emptying an already-empty map, I believe the right fix is to empty the map
> as often as can correctly do so, hence this patch - based substantially on
> Richard's comments in PR/68117.
>
> Bootstrapped + check-gcc + check-g++ on x86_64 linux, based on r231105; I've
> also built SPEC2000 on aarch64-none-linux-gnu by applying this patch (roughly)
> onto the previously-failing r229711, which also passes aarch64 bootstrap, and
> a more recent bootstrap on aarch64 is ongoing. Assuming/if no regressions
> there...
>
> Is this ok for trunk?
>
> This could also be a candidate for the 5.3 release; backporting depends only
> on
> the (fairly trivial) r230357.
Looks good to me (for both, but backport only after 5.3 is released). But
please wait for the discussion with Jeff to settle down.
Thanks,
Richard.
> gcc/ChangeLog:
>
> <DATE> Alan Lawrence <alan.lawre...@arm.com>
> Richard Biener <richard.guent...@gmail.com>
>
> * cfgexpand.c (pass_expand::execute): Replace call to
> redirect_edge_var_map_destroy with redirect_edge_var_map_empty.
> * tree-ssa.c (delete_tree_ssa): Likewise.
> * function.c (set_cfun): Call redirect_edge_var_map_empty.
> * passes.c (execute_one_ipa_transform_pass, execute_one_pass): Likewise.
> * tree-ssa.h (redirect_edge_var_map_destroy): Remove.
> (redirect_edge_var_map_empty): New.
> * tree-ssa.c (redirect_edge_var_map_destroy): Remove.
> (redirect_edge_var_map_empty): New.
>
> ---
> gcc/cfgexpand.c | 2 +-
> gcc/function.c | 2 ++
> gcc/passes.c | 2 ++
> gcc/tree-ssa.c | 8 ++++----
> gcc/tree-ssa.h | 2 +-
> 5 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 1990e10..ede1b82 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -6291,7 +6291,7 @@ pass_expand::execute (function *fun)
> expand_phi_nodes (&SA);
>
> /* Release any stale SSA redirection data. */
> - redirect_edge_var_map_destroy ();
> + redirect_edge_var_map_empty ();
>
> /* Register rtl specific functions for cfg. */
> rtl_register_cfg_hooks ();
> diff --git a/gcc/function.c b/gcc/function.c
> index 515d7c0..e452865 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -75,6 +75,7 @@ along with GCC; see the file COPYING3. If not see
> #include "tree-chkp.h"
> #include "rtl-chkp.h"
> #include "tree-dfa.h"
> +#include "tree-ssa.h"
>
> /* So we can assign to cfun in this file. */
> #undef cfun
> @@ -4798,6 +4799,7 @@ set_cfun (struct function *new_cfun)
> {
> cfun = new_cfun;
> invoke_set_current_function_hook (new_cfun ? new_cfun->decl :
> NULL_TREE);
> + redirect_edge_var_map_empty ();
> }
> }
>
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 0e23dcb..ba9bfc2 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -2218,6 +2218,7 @@ execute_one_ipa_transform_pass (struct cgraph_node
> *node,
> pass_fini_dump_file (pass);
>
> current_pass = NULL;
> + redirect_edge_var_map_empty ();
>
> /* Signal this is a suitable GC collection point. */
> if (!(todo_after & TODO_do_not_ggc_collect))
> @@ -2370,6 +2371,7 @@ execute_one_pass (opt_pass *pass)
> || pass->type != RTL_PASS);
>
> current_pass = NULL;
> + redirect_edge_var_map_empty ();
>
> if (todo_after & TODO_discard_function)
> {
> diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
> index 02fca4c..ddc7a65 100644
> --- a/gcc/tree-ssa.c
> +++ b/gcc/tree-ssa.c
> @@ -119,10 +119,10 @@ redirect_edge_var_map_vector (edge e)
> /* Clear the edge variable mappings. */
>
> void
> -redirect_edge_var_map_destroy (void)
> +redirect_edge_var_map_empty (void)
> {
> - delete edge_var_maps;
> - edge_var_maps = NULL;
> + if (edge_var_maps)
> + edge_var_maps->empty ();
> }
>
>
> @@ -1128,7 +1128,7 @@ delete_tree_ssa (struct function *fn)
> fn->gimple_df = NULL;
>
> /* We no longer need the edge variable maps. */
> - redirect_edge_var_map_destroy ();
> + redirect_edge_var_map_empty ();
> }
>
> /* Return true if EXPR is a useless type conversion, otherwise return
> diff --git a/gcc/tree-ssa.h b/gcc/tree-ssa.h
> index 3b5bd70..d33eb9c 100644
> --- a/gcc/tree-ssa.h
> +++ b/gcc/tree-ssa.h
> @@ -35,7 +35,7 @@ extern void redirect_edge_var_map_add (edge, tree, tree,
> source_location);
> extern void redirect_edge_var_map_clear (edge);
> extern void redirect_edge_var_map_dup (edge, edge);
> extern vec<edge_var_map> *redirect_edge_var_map_vector (edge);
> -extern void redirect_edge_var_map_destroy (void);
> +extern void redirect_edge_var_map_empty (void);
> extern edge ssa_redirect_edge (edge, basic_block);
> extern void flush_pending_stmts (edge);
> extern void gimple_replace_ssa_lhs (gimple *, tree);
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
