Re: Check that unlinked uses do not contain ssa-names when renaming.
On 10/16/14 06:11, Tom de Vries wrote: On 16-10-14 10:14, Richard Biener wrote: I've tried to implement the 'light verification pass' you describe above, and I've checked that the error in my patch is found, also when I remove the trigger for the verification error from my patch. Bootstrapped and reg-tested on x86_64 (with the ENABLE_CHECKING guarding removed, in order to ensure the code is active). OK for trunk? [ ... ] Thanks for tackling this Tom... It's something I probably should have done ages ago. Jeff
Re: Check that unlinked uses do not contain ssa-names when renaming.
On Thu, Oct 16, 2014 at 3:57 PM, Tom de Vries wrote:
> On 16-10-14 14:20, Richard Biener wrote:
>>>
>>> Richard,
>>> >
>>> >I've implemented the changes listed above, and also made the message a
>>> > bit
>>> >more verbose:
>>> >...
>>> >kernels-2.c: In function ‘main’:
>>> >kernels-2.c:41:5: error: statement uses released SSA name
>>> > for (COUNTERTYPE ii = 0; ii < N; ii++)
>>> > ^
>>> ># .MEM_57 = VDEF <.MEM_79>
>>> >.omp_data_arr.10 ={v} {CLOBBER};
>>> >The use of .MEM_79 should have been replaced or marked for renaming
>>
>> ^^^ or marked for renaming is not correct, only replacing is
>>
>
> I've checked in the version with "should have been replaced".
>
> I was trying to mention both possibilities that you mentioned here:
> https://gcc.gnu.org/ml/gcc-patches/2011-10/msg00977.html:
> ...
> Whoever unlinks the vuse (by removing its definition) has to replace it with
> something valid, which is either the bare symbol .MEM, or the VUSE
> associated with the removed VDEF (thus, as unlink_stmt_vdef does).
> ...
>
> So, I hope better formulated this time, what I intended to state was:
> ...
> The use of .MEM_79 should have been replaced with either the underlying
> symbol or a valid SSA name.
> ...
>
> But perhaps that's not generally valid? I've browsed tree-into-ssa.c a bit,
> and I only find the 'replace with underlying symbol' for virtual operands.
Yeah, that's only valid for virtual operands.
Richard.
> Thanks,
> - Tom
>
Re: Check that unlinked uses do not contain ssa-names when renaming.
On 16-10-14 14:20, Richard Biener wrote:
Richard,
>
>I've implemented the changes listed above, and also made the message a bit
>more verbose:
>...
>kernels-2.c: In function ‘main’:
>kernels-2.c:41:5: error: statement uses released SSA name
> for (COUNTERTYPE ii = 0; ii < N; ii++)
> ^
># .MEM_57 = VDEF <.MEM_79>
>.omp_data_arr.10 ={v} {CLOBBER};
>The use of .MEM_79 should have been replaced or marked for renaming
^^^ or marked for renaming is not correct, only replacing is
I've checked in the version with "should have been replaced".
I was trying to mention both possibilities that you mentioned here:
https://gcc.gnu.org/ml/gcc-patches/2011-10/msg00977.html:
...
Whoever unlinks the vuse (by removing its definition) has to replace it with
something valid, which is either the bare symbol .MEM, or the VUSE associated
with the removed VDEF (thus, as unlink_stmt_vdef does).
...
So, I hope better formulated this time, what I intended to state was:
...
The use of .MEM_79 should have been replaced with either the underlying symbol
or a valid SSA name.
...
But perhaps that's not generally valid? I've browsed tree-into-ssa.c a bit, and
I only find the 'replace with underlying symbol' for virtual operands.
Thanks,
- Tom
Re: Check that unlinked uses do not contain ssa-names when renaming.
On Thu, Oct 16, 2014 at 2:11 PM, Tom de Vries wrote:
> On 16-10-14 10:14, Richard Biener wrote:
>>
>> On Thu, Oct 16, 2014 at 9:20 AM, Tom de Vries
>> wrote:
>>>
>>> On 08/10/12 11:24, Richard Guenther wrote:
On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries
wrote:
>
> Richard,
>
> attached patch checks that unlinked uses do not contain ssa-names when
> renaming.
>
> This assert triggers when compiling (without the fix) the PR54735
> example.
>
> AFAIU, it was due to chance that we caught the PR54735 bug by hitting
> the
> verification failure, because the new vdef introduced by renaming
> happened to be
> the same name as the ssa name referenced in the invalid unlinked use
> (in terms
> of maybe_replace_use: rdef == use).
>
> The assert from this patch catches all cases that an unlinked use
> contains an
> ssa-name.
>
> Bootstrapped and reg-tested on x86_64 (Ada inclusive).
>
> OK for trunk?
I don't think that is exactly what we should assert here ... (I thought
about
adding checking myself ...). What we'd want to assert is that before
any new DEF is registered (which may re-allocate an SSA name) that
no uses with SSA_NAME_IN_FREELIST appear. Thus, a light verification
pass would be necessary at the beginning of update_ssa
(which I queued onto my TODO list ...). We'd want that anyway to for
example catch the case where a non-virtual operand is partially renamed.
>>>
>>> Richard,
>>>
>>> while developing a patch, I ran into the same 'no immediate_use list'
>>> verification error again, caused by an unlinked use containing an
>>> ssa-name.
>>>
>>> The verification error was caused by an error in my patch, but triggered
>>> by
>>> chance, by an unrelated change in the patch.
>>>
>>> I've tried to implement the 'light verification pass' you describe above,
>>> and
>>> I've checked that the error in my patch is found, also when I remove the
>>> trigger
>>> for the verification error from my patch.
>>>
>>> Bootstrapped and reg-tested on x86_64 (with the ENABLE_CHECKING guarding
>>> removed, in order to ensure the code is active).
>>>
>>> OK for trunk?
>>
>>
>> Ok with changing the gcc_assert to
>>
>>if (SSA_NAME_IN_FREE_LIST (use))
>> {
>> error ("statement uses released SSA name");
>> debug_gimple_stmt (stmt);
>> err = true;
>> }
>>
>> and after checking all stmts
>>
>>if (err)
>> internal_error ("cannot update SSA form");
>>
>> you might want to push/pop TV_TREE_STMT_VERIFY around all this
>> as well.
>>
>
> Richard,
>
> I've implemented the changes listed above, and also made the message a bit
> more verbose:
> ...
> kernels-2.c: In function ‘main’:
> kernels-2.c:41:5: error: statement uses released SSA name
> for (COUNTERTYPE ii = 0; ii < N; ii++)
> ^
> # .MEM_57 = VDEF <.MEM_79>
> .omp_data_arr.10 ={v} {CLOBBER};
> The use of .MEM_79 should have been replaced or marked for renaming
^^^ or marked for renaming is not correct, only replacing is
> kernels-2.c:41:5: internal compiler error: cannot update SSA from
> ...
>
> I've added mentioning the specific use that has the problem, since it will
> not always be evident which is the one with the problem.
>
> OK for trunk?
Ok with ajdusting the message.
Thanks
RIchard.
> If that's too verbose I can also implement instead:
> ...
> kernels-2.c:41:5: error: statement uses released SSA name .MEM_79
> ...
>
> Thanks,
> - Tom
>
Re: Check that unlinked uses do not contain ssa-names when renaming.
On 16-10-14 10:14, Richard Biener wrote:
On Thu, Oct 16, 2014 at 9:20 AM, Tom de Vries wrote:
On 08/10/12 11:24, Richard Guenther wrote:
On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries wrote:
Richard,
attached patch checks that unlinked uses do not contain ssa-names when renaming.
This assert triggers when compiling (without the fix) the PR54735 example.
AFAIU, it was due to chance that we caught the PR54735 bug by hitting the
verification failure, because the new vdef introduced by renaming happened to be
the same name as the ssa name referenced in the invalid unlinked use (in terms
of maybe_replace_use: rdef == use).
The assert from this patch catches all cases that an unlinked use contains an
ssa-name.
Bootstrapped and reg-tested on x86_64 (Ada inclusive).
OK for trunk?
I don't think that is exactly what we should assert here ... (I thought about
adding checking myself ...). What we'd want to assert is that before
any new DEF is registered (which may re-allocate an SSA name) that
no uses with SSA_NAME_IN_FREELIST appear. Thus, a light verification
pass would be necessary at the beginning of update_ssa
(which I queued onto my TODO list ...). We'd want that anyway to for
example catch the case where a non-virtual operand is partially renamed.
Richard,
while developing a patch, I ran into the same 'no immediate_use list'
verification error again, caused by an unlinked use containing an ssa-name.
The verification error was caused by an error in my patch, but triggered by
chance, by an unrelated change in the patch.
I've tried to implement the 'light verification pass' you describe above, and
I've checked that the error in my patch is found, also when I remove the trigger
for the verification error from my patch.
Bootstrapped and reg-tested on x86_64 (with the ENABLE_CHECKING guarding
removed, in order to ensure the code is active).
OK for trunk?
Ok with changing the gcc_assert to
if (SSA_NAME_IN_FREE_LIST (use))
{
error ("statement uses released SSA name");
debug_gimple_stmt (stmt);
err = true;
}
and after checking all stmts
if (err)
internal_error ("cannot update SSA form");
you might want to push/pop TV_TREE_STMT_VERIFY around all this
as well.
Richard,
I've implemented the changes listed above, and also made the message a bit more
verbose:
...
kernels-2.c: In function ‘main’:
kernels-2.c:41:5: error: statement uses released SSA name
for (COUNTERTYPE ii = 0; ii < N; ii++)
^
# .MEM_57 = VDEF <.MEM_79>
.omp_data_arr.10 ={v} {CLOBBER};
The use of .MEM_79 should have been replaced or marked for renaming
kernels-2.c:41:5: internal compiler error: cannot update SSA from
...
I've added mentioning the specific use that has the problem, since it will not
always be evident which is the one with the problem.
OK for trunk?
If that's too verbose I can also implement instead:
...
kernels-2.c:41:5: error: statement uses released SSA name .MEM_79
...
Thanks,
- Tom
2014-10-16 Tom de Vries
* tree-into-ssa.c (update_ssa): Assert that there's no ssa use operand
with SSA_NAME_IN_FREELIST.
diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
index 01203de..dcfba3c 100644
--- a/gcc/tree-into-ssa.c
+++ b/gcc/tree-into-ssa.c
@@ -3161,6 +3161,47 @@ update_ssa (unsigned update_flags)
if (!need_ssa_update_p (cfun))
return;
+#ifdef ENABLE_CHECKING
+ timevar_push (TV_TREE_STMT_VERIFY);
+
+ bool err = false;
+
+ FOR_EACH_BB_FN (bb, cfun)
+{
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+ {
+ gimple stmt = gsi_stmt (gsi);
+
+ ssa_op_iter i;
+ use_operand_p use_p;
+ FOR_EACH_SSA_USE_OPERAND (use_p, stmt, i, SSA_OP_ALL_USES)
+ {
+ tree use = USE_FROM_PTR (use_p);
+ if (TREE_CODE (use) != SSA_NAME)
+ continue;
+
+ if (SSA_NAME_IN_FREE_LIST (use))
+ {
+ error ("statement uses released SSA name:");
+ debug_gimple_stmt (stmt);
+ fprintf (stderr, "The use of ");
+ print_generic_expr (stderr, use, 0);
+ fprintf (stderr,
+ " should have been replaced or marked for renaming"
+ "\n");
+ err = true;
+ }
+ }
+ }
+}
+
+ if (err)
+internal_error ("cannot update SSA form");
+
+ timevar_pop (TV_TREE_STMT_VERIFY);
+#endif
+
timevar_push (TV_TREE_SSA_INCREMENTAL);
if (dump_file && (dump_flags & TDF_DETAILS))
--
1.9.1
Re: Check that unlinked uses do not contain ssa-names when renaming.
On Thu, Oct 16, 2014 at 9:20 AM, Tom de Vries wrote:
> On 08/10/12 11:24, Richard Guenther wrote:
>> On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries wrote:
>>> Richard,
>>>
>>> attached patch checks that unlinked uses do not contain ssa-names when
>>> renaming.
>>>
>>> This assert triggers when compiling (without the fix) the PR54735 example.
>>>
>>> AFAIU, it was due to chance that we caught the PR54735 bug by hitting the
>>> verification failure, because the new vdef introduced by renaming happened
>>> to be
>>> the same name as the ssa name referenced in the invalid unlinked use (in
>>> terms
>>> of maybe_replace_use: rdef == use).
>>>
>>> The assert from this patch catches all cases that an unlinked use contains
>>> an
>>> ssa-name.
>>>
>>> Bootstrapped and reg-tested on x86_64 (Ada inclusive).
>>>
>>> OK for trunk?
>>
>> I don't think that is exactly what we should assert here ... (I thought about
>> adding checking myself ...). What we'd want to assert is that before
>> any new DEF is registered (which may re-allocate an SSA name) that
>> no uses with SSA_NAME_IN_FREELIST appear. Thus, a light verification
>> pass would be necessary at the beginning of update_ssa
>> (which I queued onto my TODO list ...). We'd want that anyway to for
>> example catch the case where a non-virtual operand is partially renamed.
>>
>
> Richard,
>
> while developing a patch, I ran into the same 'no immediate_use list'
> verification error again, caused by an unlinked use containing an ssa-name.
>
> The verification error was caused by an error in my patch, but triggered by
> chance, by an unrelated change in the patch.
>
> I've tried to implement the 'light verification pass' you describe above, and
> I've checked that the error in my patch is found, also when I remove the
> trigger
> for the verification error from my patch.
>
> Bootstrapped and reg-tested on x86_64 (with the ENABLE_CHECKING guarding
> removed, in order to ensure the code is active).
>
> OK for trunk?
Ok with changing the gcc_assert to
if (SSA_NAME_IN_FREE_LIST (use))
{
error ("statement uses released SSA name");
debug_gimple_stmt (stmt);
err = true;
}
and after checking all stmts
if (err)
internal_error ("cannot update SSA form");
you might want to push/pop TV_TREE_STMT_VERIFY around all this
as well.
Thanks,
Richard.
> Thanks,
> - Tom
>
>
Re: Check that unlinked uses do not contain ssa-names when renaming.
On 08/10/12 11:24, Richard Guenther wrote:
> On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries wrote:
>> Richard,
>>
>> attached patch checks that unlinked uses do not contain ssa-names when
>> renaming.
>>
>> This assert triggers when compiling (without the fix) the PR54735 example.
>>
>> AFAIU, it was due to chance that we caught the PR54735 bug by hitting the
>> verification failure, because the new vdef introduced by renaming happened
>> to be
>> the same name as the ssa name referenced in the invalid unlinked use (in
>> terms
>> of maybe_replace_use: rdef == use).
>>
>> The assert from this patch catches all cases that an unlinked use contains an
>> ssa-name.
>>
>> Bootstrapped and reg-tested on x86_64 (Ada inclusive).
>>
>> OK for trunk?
>
> I don't think that is exactly what we should assert here ... (I thought about
> adding checking myself ...). What we'd want to assert is that before
> any new DEF is registered (which may re-allocate an SSA name) that
> no uses with SSA_NAME_IN_FREELIST appear. Thus, a light verification
> pass would be necessary at the beginning of update_ssa
> (which I queued onto my TODO list ...). We'd want that anyway to for
> example catch the case where a non-virtual operand is partially renamed.
>
Richard,
while developing a patch, I ran into the same 'no immediate_use list'
verification error again, caused by an unlinked use containing an ssa-name.
The verification error was caused by an error in my patch, but triggered by
chance, by an unrelated change in the patch.
I've tried to implement the 'light verification pass' you describe above, and
I've checked that the error in my patch is found, also when I remove the trigger
for the verification error from my patch.
Bootstrapped and reg-tested on x86_64 (with the ENABLE_CHECKING guarding
removed, in order to ensure the code is active).
OK for trunk?
Thanks,
- Tom
2014-10-16 Tom de Vries
* tree-into-ssa.c (update_ssa): Assert that there's no ssa use operand
with SSA_NAME_IN_FREELIST.
diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
index 01203de..227d5bb 100644
--- a/gcc/tree-into-ssa.c
+++ b/gcc/tree-into-ssa.c
update_ssa (unsigned update_flags)
timevar_push (TV_TREE_SSA_INCREMENTAL);
+#ifdef ENABLE_CHECKING
+ FOR_EACH_BB_FN (bb, cfun)
+{
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+ {
+ gimple stmt = gsi_stmt (gsi);
+
+ ssa_op_iter i;
+ use_operand_p use_p;
+ FOR_EACH_SSA_USE_OPERAND (use_p, stmt, i, SSA_OP_ALL_USES)
+ {
+ tree use = USE_FROM_PTR (use_p);
+ if (TREE_CODE (use) != SSA_NAME)
+ continue;
+
+ gcc_assert (!SSA_NAME_IN_FREE_LIST (use));
+ }
+ }
+}
+#endif
+
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, "\nUpdating SSA:\n");
--
1.9.1
Re: Check that unlinked uses do not contain ssa-names when renaming.
On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries wrote: > Richard, > > attached patch checks that unlinked uses do not contain ssa-names when > renaming. > > This assert triggers when compiling (without the fix) the PR54735 example. > > AFAIU, it was due to chance that we caught the PR54735 bug by hitting the > verification failure, because the new vdef introduced by renaming happened to > be > the same name as the ssa name referenced in the invalid unlinked use (in terms > of maybe_replace_use: rdef == use). > > The assert from this patch catches all cases that an unlinked use contains an > ssa-name. > > Bootstrapped and reg-tested on x86_64 (Ada inclusive). > > OK for trunk? I don't think that is exactly what we should assert here ... (I thought about adding checking myself ...). What we'd want to assert is that before any new DEF is registered (which may re-allocate an SSA name) that no uses with SSA_NAME_IN_FREELIST appear. Thus, a light verification pass would be necessary at the beginning of update_ssa (which I queued onto my TODO list ...). We'd want that anyway to for example catch the case where a non-virtual operand is partially renamed. Thanks, Richard. > Thanks, > - Tom > > 2012-10-07 Tom de Vries > > * tree-into-ssa.c (maybe_replace_use): Add assert.
Check that unlinked uses do not contain ssa-names when renaming.
Richard, attached patch checks that unlinked uses do not contain ssa-names when renaming. This assert triggers when compiling (without the fix) the PR54735 example. AFAIU, it was due to chance that we caught the PR54735 bug by hitting the verification failure, because the new vdef introduced by renaming happened to be the same name as the ssa name referenced in the invalid unlinked use (in terms of maybe_replace_use: rdef == use). The assert from this patch catches all cases that an unlinked use contains an ssa-name. Bootstrapped and reg-tested on x86_64 (Ada inclusive). OK for trunk? Thanks, - Tom 2012-10-07 Tom de Vries * tree-into-ssa.c (maybe_replace_use): Add assert. Index: gcc/tree-into-ssa.c === --- gcc/tree-into-ssa.c (revision 192023) +++ gcc/tree-into-ssa.c (working copy) @@ -1773,6 +1773,9 @@ rdef = get_reaching_def (sym); else if (is_old_name (use)) rdef = get_reaching_def (use); + + if (use_p->prev == NULL && use_p->next == NULL) +gcc_assert (TREE_CODE (use) != SSA_NAME); if (rdef && rdef != use) SET_USE (use_p, rdef);
