[2/2] df-scan: remove ad-hoc handling of global regs in asms

As discussed in the cover letter, the code removed in this patch is unnecessary,
references to global reg vars from inline asms do not work reliably, and so we
should simply require that inline asms use constraints to make such references
properly visible to the compiler.

Bootstrapped/regtested on powerpc64, will retest on ppc64le and x86 in stage 1.

        PR rtl-optimization/79985
	* df-scan.c (df_insn_refs_collect): Remove special case for
        global registers and asm statements.
---
 gcc/df-scan.c | 11 -----------
 1 file changed, 11 deletions(-)

On Mon, 23 Apr 2018, Alexander Monakov wrote:

> As discussed in the cover letter, the code removed in this patch is unnecessary,
> references to global reg vars from inline asms do not work reliably, and so we
> should simply require that inline asms use constraints to make such references
> properly visible to the compiler.
> 
> Bootstrapped/regtested on powerpc64, will retest on ppc64le and x86 in stage 1.
> 
>         PR rtl-optimization/79985
> 	* df-scan.c (df_insn_refs_collect): Remove special case for
>         global registers and asm statements.

Ping. I've retested once on ppc64le since posting.

> ---
>  gcc/df-scan.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/gcc/df-scan.c b/gcc/df-scan.c
> index 95e1e0df2d5..cbb08fc36ae 100644
> --- a/gcc/df-scan.c
> +++ b/gcc/df-scan.c
> @@ -3207,17 +3207,6 @@ df_insn_refs_collect (struct df_collection_rec *collection_rec,
>    if (CALL_P (insn_info->insn))
>      df_get_call_refs (collection_rec, bb, insn_info, flags);
>  
> -  if (asm_noperands (PATTERN (insn_info->insn)) >= 0)
> -    for (unsigned i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> -      if (global_regs[i])
> -       {
> -         /* As with calls, asm statements reference all global regs. */
> -         df_ref_record (DF_REF_BASE, collection_rec, regno_reg_rtx[i],
> -                        NULL, bb, insn_info, DF_REF_REG_USE, flags);
> -         df_ref_record (DF_REF_BASE, collection_rec, regno_reg_rtx[i],
> -                        NULL, bb, insn_info, DF_REF_REG_DEF, flags);
> -       }
> -
>    /* Record other defs.  These should be mostly for DF_REF_REGULAR, so
>       that a qsort on the defs is unnecessary in most cases.  */
>    df_defs_record (collection_rec,
>

On 05/16/2018 04:30 AM, Alexander Monakov wrote:
> 
> 
> On Mon, 23 Apr 2018, Alexander Monakov wrote:
> 
>> As discussed in the cover letter, the code removed in this patch is unnecessary,
>> references to global reg vars from inline asms do not work reliably, and so we
>> should simply require that inline asms use constraints to make such references
>> properly visible to the compiler.
>>
>> Bootstrapped/regtested on powerpc64, will retest on ppc64le and x86 in stage 1.
>>
>>         PR rtl-optimization/79985
>> 	* df-scan.c (df_insn_refs_collect): Remove special case for
>>         global registers and asm statements.
> 
> Ping. I've retested once on ppc64le since posting.
This has the potential to break existing code that we've tried to keep
working.  Worse yet, it's not code we're likely to see until gcc-9 goes
into wide deployment.  So there's certainly a risk of complaints around
this change.

I would not expect our testsuite to provide any meaningful test coverage
here.  Matz (in the discussion around pr44281 on gcc-patches) indicates
that some jits may utilize global registers.  Unfortunately, he doesn't
indicate which ones -- which would provide a pointer for deeper testing.

But even with those caveats, I think the consensus is to go forward with
the doc change.  This change naturally follows from the doc update.

So OK for the trunk.  If there's fallout in gcc-9, we'll obviously have
to deal with it.

jeff

On Tue, 22 May 2018, Jeff Law wrote:
> So OK for the trunk.  If there's fallout in gcc-9, we'll obviously have
> to deal with it.

IMHO what happened here is not healthy. Thank you for the green light.

Alexander