| Message ID | 20140529110526.GB30323@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 05/29/14 05:05, Ilya Enkovich wrote: > Hi, > > This patch allows to perform function versioning when some structures are not available yet. It is required to make clones for Pointer Bounds Checker right after SSA build. > > Bootstrapped and tested on linux-x86_64. > > Thanks, > Ilya > -- > gcc/ > > 2014-05-29 Ilya Enkovich <ilya.enkovich@intel.com> > > * tree-inline.c (copy_cfg_body): Check loop tree > existence before accessing it. > (tree_function_versioning): Check DF info existence > before accessing it. > > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > index 4293241..23fef90 100644 > --- a/gcc/tree-inline.c > +++ b/gcc/tree-inline.c > @@ -2544,7 +2544,8 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale, > > /* 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) > + if (loops_for_fn (src_cfun) > + && loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) > loops_state_set (LOOPS_NEED_FIXUP); 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? Similarly for the PTA info, we just build it from scratch in the copy at some point? Assuming the answers to both are yes, then this patch is OK for the trunk when the rest of the patches are approved. It's not great, bit it's OK. jeff
On 30 May 10:59, Jeff Law wrote: > On 05/29/14 05:05, Ilya Enkovich wrote: > >Hi, > > > >This patch allows to perform function versioning when some structures are not available yet. It is required to make clones for Pointer Bounds Checker right after SSA build. > > > >Bootstrapped and tested on linux-x86_64. > > > >Thanks, > >Ilya > >-- > >gcc/ > > > >2014-05-29 Ilya Enkovich <ilya.enkovich@intel.com> > > > > * tree-inline.c (copy_cfg_body): Check loop tree > > existence before accessing it. > > (tree_function_versioning): Check DF info existence > > before accessing it. > > > >diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > >index 4293241..23fef90 100644 > >--- a/gcc/tree-inline.c > >+++ b/gcc/tree-inline.c > >@@ -2544,7 +2544,8 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale, > > > > /* 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) > >+ if (loops_for_fn (src_cfun) > >+ && loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) > > loops_state_set (LOOPS_NEED_FIXUP); > 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. > > Similarly for the PTA info, we just build it from scratch in the > copy at some point? Here we also have conditional access like /* Reset the escaped solution. */ if (cfun->gimple_df) pt_solution_reset (&cfun->gimple_df->escaped); and following unconditional I've fixed. > > Assuming the answers to both are yes, then this patch is OK for the > trunk when the rest of the patches are approved. It's not great, > bit it's OK. Thanks! Ilya > > jeff >
On Mon, Jun 2, 2014 at 12:48 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 30 May 10:59, Jeff Law wrote: >> On 05/29/14 05:05, Ilya Enkovich wrote: >> >Hi, >> > >> >This patch allows to perform function versioning when some structures are not available yet. It is required to make clones for Pointer Bounds Checker right after SSA build. >> > >> >Bootstrapped and tested on linux-x86_64. >> > >> >Thanks, >> >Ilya >> >-- >> >gcc/ >> > >> >2014-05-29 Ilya Enkovich <ilya.enkovich@intel.com> >> > >> > * tree-inline.c (copy_cfg_body): Check loop tree >> > existence before accessing it. >> > (tree_function_versioning): Check DF info existence >> > before accessing it. >> > >> >diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c >> >index 4293241..23fef90 100644 >> >--- a/gcc/tree-inline.c >> >+++ b/gcc/tree-inline.c >> >@@ -2544,7 +2544,8 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale, >> > >> > /* 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) >> >+ if (loops_for_fn (src_cfun) >> >+ && loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) >> > loops_state_set (LOOPS_NEED_FIXUP); >> 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. Actually after SSA is built we always have loops (loops are built once we build the CFG which happens earlier). So all the above checks are no longer necessary now. >> >> Similarly for the PTA info, we just build it from scratch in the >> copy at some point? > > Here we also have conditional access like > > /* Reset the escaped solution. */ > if (cfun->gimple_df) > pt_solution_reset (&cfun->gimple_df->escaped); > > and following unconditional I've fixed. Likewise. The init_data_structures pass initializes this (init_tree_ssa). So I'm not sure why you need all this given we are in SSA form? Richard. >> >> Assuming the answers to both are yes, then this patch is OK for the >> trunk when the rest of the patches are approved. It's not great, >> bit it's OK. > > Thanks! > Ilya > >> >> jeff >>
2014-06-02 15:56 GMT+04:00 Richard Biener <richard.guenther@gmail.com>: > On Mon, Jun 2, 2014 at 12:48 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> On 30 May 10:59, Jeff Law wrote: >>> On 05/29/14 05:05, Ilya Enkovich wrote: >>> >Hi, >>> > >>> >This patch allows to perform function versioning when some structures are not available yet. It is required to make clones for Pointer Bounds Checker right after SSA build. >>> > >>> >Bootstrapped and tested on linux-x86_64. >>> > >>> >Thanks, >>> >Ilya >>> >-- >>> >gcc/ >>> > >>> >2014-05-29 Ilya Enkovich <ilya.enkovich@intel.com> >>> > >>> > * tree-inline.c (copy_cfg_body): Check loop tree >>> > existence before accessing it. >>> > (tree_function_versioning): Check DF info existence >>> > before accessing it. >>> > >>> >diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c >>> >index 4293241..23fef90 100644 >>> >--- a/gcc/tree-inline.c >>> >+++ b/gcc/tree-inline.c >>> >@@ -2544,7 +2544,8 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale, >>> > >>> > /* 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) >>> >+ if (loops_for_fn (src_cfun) >>> >+ && loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) >>> > loops_state_set (LOOPS_NEED_FIXUP); >>> 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. > > Actually after SSA is built we always have loops (loops are built once > we build the CFG which happens earlier). So all the above checks > are no longer necessary now. > >>> >>> Similarly for the PTA info, we just build it from scratch in the >>> copy at some point? >> >> Here we also have conditional access like >> >> /* Reset the escaped solution. */ >> if (cfun->gimple_df) >> pt_solution_reset (&cfun->gimple_df->escaped); >> >> and following unconditional I've fixed. > > Likewise. The init_data_structures pass initializes this (init_tree_ssa). > > So I'm not sure why you need all this given we are in SSA form? Instrumentation clones are created before we are in SSA form. I do it before early local passes. Ilya > > Richard. > >>> >>> Assuming the answers to both are yes, then this patch is OK for the >>> trunk when the rest of the patches are approved. It's not great, >>> bit it's OK. >> >> Thanks! >> Ilya >> >>> >>> jeff >>>
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. Jeff
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? Ilya > > Jeff
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. the df structures are set up in init_data_structures pass which is run before going into SSA form (I'd like to somehow cleanup that area). Richard. > Ilya > >> >> Jeff
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. > the df structures are set up in init_data_structures pass which is run before > going into SSA form (I'd like to somehow cleanup that area). OK. So this part should be approved since we've established this code is running prior to going into SSA form. jeff
2014-06-04 10:46 GMT+04:00 Jeff Law <law@redhat.com>: > 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. CFG build pass is in lowering stage and happens before versioning. Probably I just hit some bug and hide it with my check. I'll try to remove this check and reproduce a segfault to see why it happens. Ilya > > > >> the df structures are set up in init_data_structures pass which is run >> before >> going into SSA form (I'd like to somehow cleanup that area). > > OK. So this part should be approved since we've established this code is > running prior to going into SSA form. > > jeff >
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 ... >> the df structures are set up in init_data_structures pass which is run >> before >> going into SSA form (I'd like to somehow cleanup that area). > > OK. So this part should be approved since we've established this code is > running prior to going into SSA form. Agreed. Thanks, Richard. > jeff >
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 4293241..23fef90 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -2544,7 +2544,8 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale, /* 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) + if (loops_for_fn (src_cfun) + && loops_for_fn (src_cfun)->state & LOOPS_NEED_FIXUP) loops_state_set (LOOPS_NEED_FIXUP); if (gimple_in_ssa_p (cfun)) @@ -5350,8 +5351,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;