* [PATCH, Pointer Bounds Checker 13/x] Early versioning
@ 2014-05-29 11:05 Ilya Enkovich
2014-05-30 16:59 ` Jeff Law
0 siblings, 1 reply; 14+ messages in thread
From: Ilya Enkovich @ 2014-05-29 11:05 UTC (permalink / raw)
To: gcc-patches
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-05-29 11:05 [PATCH, Pointer Bounds Checker 13/x] Early versioning Ilya Enkovich
@ 2014-05-30 16:59 ` Jeff Law
2014-06-02 10:48 ` Ilya Enkovich
0 siblings, 1 reply; 14+ messages in thread
From: Jeff Law @ 2014-05-30 16:59 UTC (permalink / raw)
To: Ilya Enkovich, gcc-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-05-30 16:59 ` Jeff Law
@ 2014-06-02 10:48 ` Ilya Enkovich
2014-06-02 11:56 ` Richard Biener
2014-06-02 17:27 ` Jeff Law
0 siblings, 2 replies; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-02 10:48 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-02 10:48 ` Ilya Enkovich
@ 2014-06-02 11:56 ` Richard Biener
2014-06-02 12:02 ` Ilya Enkovich
2014-06-02 17:27 ` Jeff Law
1 sibling, 1 reply; 14+ messages in thread
From: Richard Biener @ 2014-06-02 11:56 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: Jeff Law, GCC Patches
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
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-02 11:56 ` Richard Biener
@ 2014-06-02 12:02 ` Ilya Enkovich
0 siblings, 0 replies; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-02 12:02 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, GCC Patches
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
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-02 10:48 ` Ilya Enkovich
2014-06-02 11:56 ` Richard Biener
@ 2014-06-02 17:27 ` Jeff Law
2014-06-03 5:55 ` Ilya Enkovich
1 sibling, 1 reply; 14+ messages in thread
From: Jeff Law @ 2014-06-02 17:27 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-02 17:27 ` Jeff Law
@ 2014-06-03 5:55 ` Ilya Enkovich
2014-06-03 9:29 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-03 5:55 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-03 5:55 ` Ilya Enkovich
@ 2014-06-03 9:29 ` Richard Biener
2014-06-04 6:46 ` Jeff Law
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2014-06-03 9:29 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: Jeff Law, gcc-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-03 9:29 ` Richard Biener
@ 2014-06-04 6:46 ` Jeff Law
2014-06-04 8:04 ` Ilya Enkovich
2014-06-04 10:00 ` Richard Biener
0 siblings, 2 replies; 14+ messages in thread
From: Jeff Law @ 2014-06-04 6:46 UTC (permalink / raw)
To: Richard Biener, Ilya Enkovich; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-04 6:46 ` Jeff Law
@ 2014-06-04 8:04 ` Ilya Enkovich
2014-06-04 10:00 ` Richard Biener
1 sibling, 0 replies; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-04 8:04 UTC (permalink / raw)
To: Jeff Law; +Cc: Richard Biener, gcc-patches
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-04 6:46 ` Jeff Law
2014-06-04 8:04 ` Ilya Enkovich
@ 2014-06-04 10:00 ` Richard Biener
2014-06-05 11:18 ` Ilya Enkovich
1 sibling, 1 reply; 14+ messages in thread
From: Richard Biener @ 2014-06-04 10:00 UTC (permalink / raw)
To: Jeff Law; +Cc: Ilya Enkovich, gcc-patches
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-04 10:00 ` Richard Biener
@ 2014-06-05 11:18 ` Ilya Enkovich
2014-06-05 11:58 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-05 11:18 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, gcc-patches
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-05 11:18 ` Ilya Enkovich
@ 2014-06-05 11:58 ` Richard Biener
2014-06-05 13:00 ` Ilya Enkovich
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2014-06-05 11:58 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: Jeff Law, gcc-patches
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, Pointer Bounds Checker 13/x] Early versioning
2014-06-05 11:58 ` Richard Biener
@ 2014-06-05 13:00 ` Ilya Enkovich
0 siblings, 0 replies; 14+ messages in thread
From: Ilya Enkovich @ 2014-06-05 13:00 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, gcc-patches
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-06-05 13:00 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-29 11:05 [PATCH, Pointer Bounds Checker 13/x] Early versioning Ilya Enkovich
2014-05-30 16:59 ` Jeff Law
2014-06-02 10:48 ` Ilya Enkovich
2014-06-02 11:56 ` Richard Biener
2014-06-02 12:02 ` Ilya Enkovich
2014-06-02 17:27 ` Jeff Law
2014-06-03 5:55 ` Ilya Enkovich
2014-06-03 9:29 ` Richard Biener
2014-06-04 6:46 ` Jeff Law
2014-06-04 8:04 ` Ilya Enkovich
2014-06-04 10:00 ` Richard Biener
2014-06-05 11:18 ` Ilya Enkovich
2014-06-05 11:58 ` Richard Biener
2014-06-05 13:00 ` Ilya Enkovich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).