Re: [VTA, PR49310] O(n+m)-ish emit_notes

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

First of all, I'm worried about generated debug info quality.

obj81{4,5,6,7}/gcc/cc1plus below are x86_64/i686 unpatched trunk
stage3 compilers (with var-tracking.c patched afterwards and make cc1plus
in the stage3 gcc directory, so it is the same code, just built with
different compiler) and x86_64/i686 patched trunk stage3, all of
them --enable-checking=yes,rtl.

What worries me is that we suddenly emit far less than half of the
previous call_site_value parameters:
for i in 81{4,5,6,7}; do readelf -wio obj$i/gcc/cc1plus | grep 'DW_AT_GNU_call_site_value' | wc -l; done
945514
901463
398378
211542
Those need to be emitted uncached, while it seems you are now caching them
too.

Another thing, is it really necessary to add the cselib locations to the
hash tables, and if yes, can't it be postponed to start of vt_emit* phase
as opposed to making all the hash table data larger already during the
second phase which is for many testcases very memory hungry?
And, do you do anything special for ENTRY_VALUEs?  The current code
is trying hard to emit something non-ENTRY_VALUE if at all possible,
those really should be the last options if nothing else can be used.
Looking at the counts
for i in 81{4,5,6,7}; do readelf -wio obj$i/gcc/cc1plus | grep 'DW_OP_GNU_entry_value' | wc -l; done
121713
36158
116585
37687
suggests that the entry_value count increased for i686 and somewhat
decreased for x86_64 (but wonder if the decreasing isn't because of less
than half of DW_AT_GNU_call_site_value which often use entry_value).

Attached below are
for i in 81{4,5,6,7}; do locstats --tabulate=0.0:5,99:1 obj$i/gcc/cc1plus 2>/dev/null > $i; done
numbers, from quick look the number of vars with 0.0% coverage went slightly
down, but the cumulative numbers for all other percentages are worse with
the patch (i.e. fewer variables with 100% coverage, etc.), though not by a
huge margin.

And just a few comments so far:

On Mon, Sep 19, 2011 at 06:17:08PM -0300, Alexandre Oliva wrote:
> +/* A vector of loc_exp_dep holds the active dependencies of a one-part
> +   DV on VALUEs, i.e., the VALUEs expanded so as to form the current
> +   location of DV.  Each entry is also part of VALUE' s linked-list of
> +   backlinks back to DV.  */
> +typedef struct loc_exp_dep_s
> +{
> +  /* The dependent DV.  */
> +  decl_or_value dv;
> +  /* The dependency VALUE or DECL_DEBUG.  */

DEBUG_EXPR.

> +/* A narrower type used to hold a onepart_enum while saving
> +   memory.  */
> +typedef char onepart_enum_t;
> +
>  /* Structure describing where the variable is located.  */
>  typedef struct variable_def
>  {
> @@ -330,10 +397,8 @@ typedef struct variable_def
>    /* Number of variable parts.  */
>    char n_var_parts;
>  
> -  /* True if this variable changed (any of its) cur_loc fields
> -     during the current emit_notes_for_changes resp.
> -     emit_notes_for_differences call.  */
> -  bool cur_loc_changed;
> +  /* What type of DV this is, according to enum onepart_enum.  */
> +  onepart_enum_t onepart;

Can't you use ENUM_BITFIELD (onepart_enum) onepart : 8; instead?

> @@ -420,7 +515,6 @@ static void stack_adjust_offset_pre_post
>  static void insn_stack_adjust_offset_pre_post (rtx, HOST_WIDE_INT *,
>  					       HOST_WIDE_INT *);
>  static bool vt_stack_adjustments (void);
> -static void note_register_arguments (rtx);
>  static hashval_t variable_htab_hash (const void *);
>  static int variable_htab_eq (const void *, const void *);
>  static void variable_htab_free (void *);
> @@ -672,15 +766,11 @@ vt_stack_adjustments (void)
>  	    for (insn = BB_HEAD (dest);
>  		 insn != NEXT_INSN (BB_END (dest));
>  		 insn = NEXT_INSN (insn))
> -	      {
> -		if (INSN_P (insn))
> -		  {
> -		    insn_stack_adjust_offset_pre_post (insn, &pre, &post);
> -		    offset += pre + post;
> -		  }
> -		if (CALL_P (insn))
> -		  note_register_arguments (insn);
> -	      }
> +	      if (INSN_P (insn))
> +		{
> +		  insn_stack_adjust_offset_pre_post (insn, &pre, &post);
> +		  offset += pre + post;
> +		}
>  
>  	  VTI (dest)->out.stack_adjust = offset;
>  
> @@ -5020,9 +5026,6 @@ log_op_type (rtx x, basic_block bb, rtx 
>  /* All preserved VALUEs.  */
>  static VEC (rtx, heap) *preserved_values;
>  
> -/* Registers used in the current function for passing parameters.  */
> -static HARD_REG_SET argument_reg_set;
> -
>  /* Ensure VAL is preserved and remember it in a vector for vt_emit_notes.  */
>  
>  static void
> @@ -5382,8 +5396,6 @@ add_stores (rtx loc, const_rtx expr, voi
>  	  mo.u.loc = loc;
>  	  if (GET_CODE (expr) == SET
>  	      && SET_DEST (expr) == loc
> -	      && REGNO (loc) < FIRST_PSEUDO_REGISTER
> -	      && TEST_HARD_REG_BIT (argument_reg_set, REGNO (loc))
>  	      && find_use_val (loc, mode, cui)
>  	      && GET_CODE (SET_SRC (expr)) != ASM_OPERANDS)
>  	    {

Why the above?  Isn't it too expensive to enter it for all the registers,
even when they aren't going to be used for argument passing nor needed
explicitly for anything debug info related?

> @@ -5893,7 +5905,7 @@ prepare_call_arguments (basic_block bb, 
>  	      tree dtemp = VEC_index (tree, *debug_args, ix + 1);
>  	      enum machine_mode mode = DECL_MODE (dtemp);
>  	      item = gen_rtx_DEBUG_PARAMETER_REF (mode, param);
> -	      item = gen_rtx_CONCAT (mode, item, DECL_RTL (dtemp));
> +	      item = gen_rtx_CONCAT (mode, item, DECL_RTL_IF_SET (dtemp));

Generating CONCAT with NULL second argument?  Ugh.  Plus, when will
that happen?

	Jakub

Attachment: 814
Description: Text document

Attachment: 815
Description: Text document

Attachment: 816
Description: Text document

Attachment: 817
Description: Text document