[PATCH, Pointer Bounds Checker 13/x] Early versioning

[PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Jeff Law]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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
> 

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Richard Biener]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Jeff Law]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Richard Biener]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Jeff Law]

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

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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
>

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Richard Biener]

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
>

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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.

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;

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Richard Biener]

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;

Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning

[Ilya Enkovich]

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;