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
