| Message ID | 543FB5EF.9040200@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 16, 2014 at 2:11 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > On 16-10-14 10:14, Richard Biener wrote: >> >> On Thu, Oct 16, 2014 at 9:20 AM, Tom de Vries <Tom_deVries@mentor.com> >> wrote: >>> >>> On 08/10/12 11:24, Richard Guenther wrote: >>>> >>>> On Sun, Oct 7, 2012 at 12:44 PM, Tom de Vries <Tom_deVries@mentor.com> >>>> 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 >
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
On Thu, Oct 16, 2014 at 3:57 PM, Tom de Vries <Tom_deVries@mentor.com> 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 >
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
2014-10-16 Tom de Vries <tom@codesourcery.com> * 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