| Message ID | 20140605111833.GA7275@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 5, 2014 at 1:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 04 Jun 11:59, Richard Biener wrote: >> On Wed, Jun 4, 2014 at 8:46 AM, Jeff Law <law@redhat.com> wrote: >> > On 06/03/14 03:29, Richard Biener wrote: >> >> >> >> On Tue, Jun 3, 2014 at 7:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> >> >> wrote: >> >>> >> >>> 2014-06-02 21:27 GMT+04:00 Jeff Law <law@redhat.com>: >> >>>> >> >>>> On 06/02/14 04:48, Ilya Enkovich wrote: >> >>>>>> >> >>>>>> >> >>>>>> Hmm, so if I understand things correctly, src_fun has no loop >> >>>>>> structures attached, thus there's nothing to copy. Presumably at >> >>>>>> some later point we build loop structures for the copy from scratch? >> >>>>> >> >>>>> >> >>>>> I suppose it is just a simple bug with absent NULL pointer check. Here >> >>>>> is >> >>>>> original code: >> >>>>> >> >>>>> /* Duplicate the loop tree, if available and wanted. */ >> >>>>> if (loops_for_fn (src_cfun) != NULL >> >>>>> && current_loops != NULL) >> >>>>> { >> >>>>> copy_loops (id, entry_block_map->loop_father, >> >>>>> get_loop (src_cfun, 0)); >> >>>>> /* Defer to cfgcleanup to update loop-father fields of >> >>>>> basic-blocks. */ >> >>>>> loops_state_set (LOOPS_NEED_FIXUP); >> >>>>> } >> >>>>> >> >>>>> /* If the loop tree in the source function needed fixup, mark the >> >>>>> destination loop tree for fixup, too. */ >> >>>>> if (loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) >> >>>>> loops_state_set (LOOPS_NEED_FIXUP); >> >>>>> >> >>>>> As you may see we have check for absent loops structure in the first >> >>>>> if-statement and no check in the second one. I hit segfault and added >> >>>>> the >> >>>>> check. >> >>>> >> >>>> >> >>>> Downthread you indicated you're not in SSA form which might explain the >> >>>> inconsistency here. If so, then we need to make sure that the loop & df >> >>>> structures do get set up properly later. >> >>> >> >>> >> >>> That is what init_data_structures pass will do for us as Richard pointed. >> >>> Right? >> >> >> >> >> >> loops are set up during the CFG construction and thus are available >> >> everywhere. >> > >> > Which would argue that the hunk that checks for the loop tree's existence >> > before accessing it should not be needed. Ilya -- is it possible you hit >> > this prior to Richi's work to build the loop structures as part of CFG >> > construction and maintain them throughout compilation. >> >> That's likely. It's still on my list of janitor things to do to remove all >> those if (current_loops) checks ... > > I tried to remove this loops check and got no failures this time. So, here is a new patch version. > > Bootstrapped and tested on linux-x86_64. Ok (you can commit this now). Thanks, Richard. > Thanks, > Ilya > -- > gcc/ > > 2014-06-05 Ilya Enkovich <ilya.enkovich@intel.com> > > * tree-inline.c (tree_function_versioning): Check DF info existence > before accessing it. > > > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > index 4293241..2972346 100644 > --- a/gcc/tree-inline.c > +++ b/gcc/tree-inline.c > @@ -5350,8 +5350,9 @@ tree_function_versioning (tree old_decl, tree new_decl, > DECL_ARGUMENTS (new_decl) = DECL_ARGUMENTS (old_decl); > initialize_cfun (new_decl, old_decl, > old_entry_block->count); > - DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta > - = id.src_cfun->gimple_df->ipa_pta; > + if (DECL_STRUCT_FUNCTION (new_decl)->gimple_df) > + DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta > + = id.src_cfun->gimple_df->ipa_pta; > > /* Copy the function's static chain. */ > p = DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl;
2014-06-05 15:58 GMT+04:00 Richard Biener <richard.guenther@gmail.com>: > On Thu, Jun 5, 2014 at 1:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> On 04 Jun 11:59, Richard Biener wrote: >>> On Wed, Jun 4, 2014 at 8:46 AM, Jeff Law <law@redhat.com> wrote: >>> > On 06/03/14 03:29, Richard Biener wrote: >>> >> >>> >> On Tue, Jun 3, 2014 at 7:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> >>> >> wrote: >>> >>> >>> >>> 2014-06-02 21:27 GMT+04:00 Jeff Law <law@redhat.com>: >>> >>>> >>> >>>> On 06/02/14 04:48, Ilya Enkovich wrote: >>> >>>>>> >>> >>>>>> >>> >>>>>> Hmm, so if I understand things correctly, src_fun has no loop >>> >>>>>> structures attached, thus there's nothing to copy. Presumably at >>> >>>>>> some later point we build loop structures for the copy from scratch? >>> >>>>> >>> >>>>> >>> >>>>> I suppose it is just a simple bug with absent NULL pointer check. Here >>> >>>>> is >>> >>>>> original code: >>> >>>>> >>> >>>>> /* Duplicate the loop tree, if available and wanted. */ >>> >>>>> if (loops_for_fn (src_cfun) != NULL >>> >>>>> && current_loops != NULL) >>> >>>>> { >>> >>>>> copy_loops (id, entry_block_map->loop_father, >>> >>>>> get_loop (src_cfun, 0)); >>> >>>>> /* Defer to cfgcleanup to update loop-father fields of >>> >>>>> basic-blocks. */ >>> >>>>> loops_state_set (LOOPS_NEED_FIXUP); >>> >>>>> } >>> >>>>> >>> >>>>> /* If the loop tree in the source function needed fixup, mark the >>> >>>>> destination loop tree for fixup, too. */ >>> >>>>> if (loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) >>> >>>>> loops_state_set (LOOPS_NEED_FIXUP); >>> >>>>> >>> >>>>> As you may see we have check for absent loops structure in the first >>> >>>>> if-statement and no check in the second one. I hit segfault and added >>> >>>>> the >>> >>>>> check. >>> >>>> >>> >>>> >>> >>>> Downthread you indicated you're not in SSA form which might explain the >>> >>>> inconsistency here. If so, then we need to make sure that the loop & df >>> >>>> structures do get set up properly later. >>> >>> >>> >>> >>> >>> That is what init_data_structures pass will do for us as Richard pointed. >>> >>> Right? >>> >> >>> >> >>> >> loops are set up during the CFG construction and thus are available >>> >> everywhere. >>> > >>> > Which would argue that the hunk that checks for the loop tree's existence >>> > before accessing it should not be needed. Ilya -- is it possible you hit >>> > this prior to Richi's work to build the loop structures as part of CFG >>> > construction and maintain them throughout compilation. >>> >>> That's likely. It's still on my list of janitor things to do to remove all >>> those if (current_loops) checks ... >> >> I tried to remove this loops check and got no failures this time. So, here is a new patch version. >> >> Bootstrapped and tested on linux-x86_64. > > Ok (you can commit this now). Thanks! Committed to trunk Ilya > > Thanks, > Richard. > >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2014-06-05 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * tree-inline.c (tree_function_versioning): Check DF info existence >> before accessing it. >> >> >> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c >> index 4293241..2972346 100644 >> --- a/gcc/tree-inline.c >> +++ b/gcc/tree-inline.c >> @@ -5350,8 +5350,9 @@ tree_function_versioning (tree old_decl, tree new_decl, >> DECL_ARGUMENTS (new_decl) = DECL_ARGUMENTS (old_decl); >> initialize_cfun (new_decl, old_decl, >> old_entry_block->count); >> - DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta >> - = id.src_cfun->gimple_df->ipa_pta; >> + if (DECL_STRUCT_FUNCTION (new_decl)->gimple_df) >> + DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta >> + = id.src_cfun->gimple_df->ipa_pta; >> >> /* Copy the function's static chain. */ >> p = DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl;
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 4293241..2972346 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -5350,8 +5350,9 @@ tree_function_versioning (tree old_decl, tree new_decl, DECL_ARGUMENTS (new_decl) = DECL_ARGUMENTS (old_decl); initialize_cfun (new_decl, old_decl, old_entry_block->count); - DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta - = id.src_cfun->gimple_df->ipa_pta; + if (DECL_STRUCT_FUNCTION (new_decl)->gimple_df) + DECL_STRUCT_FUNCTION (new_decl)->gimple_df->ipa_pta + = id.src_cfun->gimple_df->ipa_pta; /* Copy the function's static chain. */ p = DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl;