Re: Check that unlinked uses do not contain ssa-names when renaming.

[Richard Biener <richard dot guenther at gmail dot com>]

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
>