ENTRY_VALUE fixes (PR debug/48203)

Hi!

This patch fixes a few problems:
1) on arm bootstrap fails, because expansion creates ENTRY_VALUE
   of a MEM with virtual reg address, which then is changed for
   ap + constant and dwarf2out doesn't expect that.
   As in most passes ENTRY_VALUE is treated like a black box,
   we shouldn't accept anything but hard registers or
   MEMs with hard register addresses
2) DW_OP_GNU_entry_value 2 <DW_OP_fbreg 16> is something we should
   avoid
3) the var-tracking and cselib changes were added to make sure
   var-tracking can easily map ENTRY_VALUEs created by cfgexpand
   back to the canonical VALUEs holding the parameter.  This means
   they can expand even to the original argument registers if they
   haven't been modified or at least in the part of the function
   where they haven't been modified.  E.g. on x86_64-linux
   on the attached testcase we no longer generate any DW_OP_GNU_entry_value
   ops, as the registers aren't clobbered.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2011-03-20  Jakub Jelinek  <jakub@redhat.com>

	PR debug/48203
	* cfgexpand.c (expand_debug_expr) <case SSA_NAME>: Only
	create ENTRY_VALUE if incoming or address of incoming's MEM
	is a hard REG.
	* dwarf2out.c (mem_loc_descriptor): Don't emit
	DW_OP_GNU_entry_value of DW_OP_fbreg.
	* var-tracking.c (vt_add_function_parameter): Ensure cselib_lookup
	on ENTRY_VALUE is able to find the canonical parameter VALUE.
	* cselib.c (rtx_equal_for_cselib_1) <case ENTRY_VALUE>: Use
	rtx_equal_p instead of rtx_equal_for_cselib_1 to compare
	ENTRY_VALUE_EXPs.
	(cselib_hash_rtx) <case ENTRY_VALUE>: If ENTRY_VALUE_EXP
	is a REG_P or MEM_P with REG_P address, compute hash directly
	instead of calling cselib_hash_rtx on ENTRY_VALUE_EXP.
	(preserve_only_constants): Don't clear VALUES forwaring
	ENTRY_VALUE to some other VALUE.

	* gcc.dg/pr48203.c: New test.


	Jakub

On 03/20/2011 05:57 AM, Jakub Jelinek wrote:
> 	* cfgexpand.c (expand_debug_expr) <case SSA_NAME>: Only
> 	create ENTRY_VALUE if incoming or address of incoming's MEM
> 	is a hard REG.
> 	* dwarf2out.c (mem_loc_descriptor): Don't emit
> 	DW_OP_GNU_entry_value of DW_OP_fbreg.

Ok.

> 	* var-tracking.c (vt_add_function_parameter): Ensure cselib_lookup
> 	on ENTRY_VALUE is able to find the canonical parameter VALUE.

I don't really understand what's going on here.  Whatever it is, it
could definitely use some more commentary; there's almost nothing in
the surrounding context.  I also suggest pulling some of this out into
a new function.  You've got 2 exact copies here in the patch, and 2
more that are nearly the same already in the code.

> 	* cselib.c (rtx_equal_for_cselib_1) <case ENTRY_VALUE>: Use
> 	rtx_equal_p instead of rtx_equal_for_cselib_1 to compare
> 	ENTRY_VALUE_EXPs.

Ok.

> 	(cselib_hash_rtx) <case ENTRY_VALUE>: If ENTRY_VALUE_EXP
> 	is a REG_P or MEM_P with REG_P address, compute hash directly
> 	instead of calling cselib_hash_rtx on ENTRY_VALUE_EXP.

Why?

> 	(preserve_only_constants): Don't clear VALUES forwaring
> 	ENTRY_VALUE to some other VALUE.

Ok with a comment.  I guess the reasoning being that even though this
value isn't "constant", it's function invariant?


r~

On Mon, Mar 28, 2011 at 09:58:38AM -0700, Richard Henderson wrote:
> > 	* var-tracking.c (vt_add_function_parameter): Ensure cselib_lookup
> > 	on ENTRY_VALUE is able to find the canonical parameter VALUE.
> 
> I don't really understand what's going on here.  Whatever it is, it
> could definitely use some more commentary; there's almost nothing in
> the surrounding context.  I also suggest pulling some of this out into
> a new function.  You've got 2 exact copies here in the patch, and 2
> more that are nearly the same already in the code.

As cfgexpand.c creates ENTRY_VALUEs (where the first hunk of this patch
applies), when vt_initialization uses cselib to lookup those ENTRY_VALUEs,
without the patch it will find a newly created cselib_val whose
only location is the ENTRY_VALUE.  Which means it will always use
DW_OP_GNU_entry_value in the end.  Sometimes that is the only usable choice,
but e.g. the first function in gcc.dg/pr48203.c shows we can do better,
because all the entry values are also held in the corresponding registers
(the function doesn't clobber them), so we can instead of
DW_OP_GNU_entry_value just refer to the registers itself, either in the
whole function, or at least as long as the aren't clobbered by something
else.  The extra cselib_lookup calls give us those values that hash
as those ENTRY_VALUE will hash and by telling that the value is live
in the parameter's VALUE vt_expand_loc_callback will find it.

Say vt_add_function_parameter is called for first parameter in %rdi,
cselib_lookup_from_insn gives us VALUE 2:2 for it.  We add
(entry_value:DI (reg:DI %rdi)) to list of locations for that VALUE.
The second cselib_lookup_from_insn gives us VALUE 3:217, which will
have locations (value:DI 2:2) (the one we've injected there) and
(entry_value:DI (reg:DI %rdi)).  Then when (entry_value:DI (reg:DI %rdi))
appears in some DEBUG_INSNs, it will give (value:DI 3:217)
and vt_expand_loc_callback will use the first location for it
(i.e. (value:DI 2:2) ) and either it will see the value is still live
in %rdi, some other register or memory, or, if nowhere else, in
(entry_value:DI (reg:DI %rdi)).  The 3 cselib.c changes from this
patch ensure that the hash value for (entry_value:DI (reg:DI %rdi))
will always be the same and (value:DI 3:217) will never be flushed from
the hash table, even when e.g. on next bb's boundary we flush
all registers from the hash table, or e.g. when movl $123, %edi
is seen in the insns.

I could of course create another hash table and map ENTRY_VALUEs through
that hash table back to the parameter VALUEs, but cselib already has
a hash table which we can use and furthermore the ENTRY_VALUEs could
be embedded in other RTLs.

I will look into creating helper inlines to reduce code duplication.

> > 	* cselib.c (rtx_equal_for_cselib_1) <case ENTRY_VALUE>: Use
> > 	rtx_equal_p instead of rtx_equal_for_cselib_1 to compare
> > 	ENTRY_VALUE_EXPs.
> 
> Ok.
> 
> > 	(cselib_hash_rtx) <case ENTRY_VALUE>: If ENTRY_VALUE_EXP
> > 	is a REG_P or MEM_P with REG_P address, compute hash directly
> > 	instead of calling cselib_hash_rtx on ENTRY_VALUE_EXP.
> 
> Why?

Because REG and MEM hash based on their current value in cselib:
    case MEM:
    case REG:
      e = cselib_lookup (x, GET_MODE (x), create, memmode);
      if (! e)
        return 0;

      return e->hash;
In ENTRY_VALUEs which are function invariant I want it to hash always
the same.

> > 	(preserve_only_constants): Don't clear VALUES forwaring
> > 	ENTRY_VALUE to some other VALUE.
> 
> Ok with a comment.  I guess the reasoning being that even though this
> value isn't "constant", it's function invariant?

Yeah.  Perhaps preserve_only_constants could be renamed
to preserve_only_function_invariants or similar (and likewise for
cselib_preserve_constants and CSELIB_PRESERVE_CONSTANTS).

	Jakub

On 03/28/2011 10:32 AM, Jakub Jelinek wrote:
> Say vt_add_function_parameter is called for first parameter in %rdi,
> cselib_lookup_from_insn gives us VALUE 2:2 for it.  We add
> (entry_value:DI (reg:DI %rdi)) to list of locations for that VALUE.
> The second cselib_lookup_from_insn gives us VALUE 3:217, which will
> have locations (value:DI 2:2) (the one we've injected there) and
> (entry_value:DI (reg:DI %rdi)).  Then when (entry_value:DI (reg:DI %rdi))
> appears in some DEBUG_INSNs, it will give (value:DI 3:217)
> and vt_expand_loc_callback will use the first location for it
> (i.e. (value:DI 2:2) ) and either it will see the value is still live
> in %rdi, some other register or memory, or, if nowhere else, in
> (entry_value:DI (reg:DI %rdi)).  The 3 cselib.c changes from this
> patch ensure that the hash value for (entry_value:DI (reg:DI %rdi))
> will always be the same and (value:DI 3:217) will never be flushed from
> the hash table, even when e.g. on next bb's boundary we flush
> all registers from the hash table, or e.g. when movl $123, %edi
> is seen in the insns.

Ok, thanks for the explanation.  All the changes make sense now.

> I will look into creating helper inlines to reduce code duplication.

Please.  You can do this as a follow-up if you prefer.

>>> 	(cselib_hash_rtx) <case ENTRY_VALUE>: If ENTRY_VALUE_EXP
>>> 	is a REG_P or MEM_P with REG_P address, compute hash directly
>>> 	instead of calling cselib_hash_rtx on ENTRY_VALUE_EXP.
>>
>> Why?
> 
> Because REG and MEM hash based on their current value in cselib:
>     case MEM:
>     case REG:
>       e = cselib_lookup (x, GET_MODE (x), create, memmode);
>       if (! e)
>         return 0;
> 
>       return e->hash;
> In ENTRY_VALUEs which are function invariant I want it to hash always
> the same.

Ok, with something akin to this as a comment.


r~