Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek <jakub@redhat.com>]

On Tue, May 24, 2011 at 12:59:57PM +0200, Eric Botcazou wrote:
> Now on to a few questions:
> 
> > 2011-05-23  Jakub Jelinek  <jakub@redhat.com>
> >
> > 	* var-tracking.c (vt_add_function_parameter): Remap incoming
> > 	MEMs with crtl->args.internal_arg_pointer based address
> > 	if stack_realign_drap to arg_pointer_rtx.
> > 	(vt_init_cfa_base): Add equate argument, don't equate cfa_base_rtx
> > 	to hfp/sp if it is false.
> > 	(vt_initialize): Adjust vt_init_cfa_base callers, don't call
> > 	vt_init_cfa_base if frame_pointer_needed, but fp_cfa_offset is -1,
> > 	set fp_cfa_offset to -1 if fp/argp hasn't been eliminated.
> 
> Why setting CFA_BASE_RTX here?  The remapping in vt_add_function_parameter 
> doesn't seem to need it.  Is that necessary so that the processing of MEMs is 
> short-circuited elsewhere?  If so, would this be correct in the whole function 
> given that the MEMs aren't adjusted in this case (i.e. compute_cfa_pointer is 
> never called)?

That is so that the argp (or framep) will be properly handled during CSELIB,
as constant VALUE that is never changed through the function.  And
additionally to find out if we can do the replacement (we can't if
argp resp. framep isn't eliminated and could therefore occur in the real
code).

> -/* Compute a CFA-based value for the stack pointer.  */
> +/* Compute a CFA-based value for an ADJUSTMENT made to stack_pointer_rtx
> +   or hard_frame_pointer_rtx.  */

This is certainly fine.

>  static inline rtx
>  compute_cfa_pointer (HOST_WIDE_INT adjustment)
> @@ -8722,6 +8723,15 @@ vt_initialize (void)
>  
>    CLEAR_HARD_REG_SET (argument_reg_set);
>  
> +  /* In order to shield ourselves from adjustments to the stack pointer or to
> +     the hard frame pointer that aren't meant to change the base location of
> +     variables, we're going to rewrite MEMs based on them into MEMs based on
> +     the CFA by de-eliminating stack_pointer_rtx or hard_frame_pointer_rtx to
> +     the virtual CFA pointer frame_pointer_rtx resp. arg_pointer_rtx.  We can
> +     do this either when there is no frame pointer in the function and stack
> +     adjustments are consistent for all basic blocks or when there is a frame
> +     pointer and no stack realignment.  In any case, we first have to check
> +     that frame_pointer_rtx resp. arg_pointer_rtx has been eliminated.  */
>    if (!frame_pointer_needed)
>      {
>        rtx reg, elim;

This comment is not 100% accurate.  The reason we do that is to use the
much more compact DW_OP_fbreg* if possible compared to huge location lists,
especially for !frame_pointer_needed where the location list would need to
have another entry whenever sp changes.  dwarf2out.c rewrites argp/framep
if eliminated into DW_OP_fbreg (with some offset when needed).

> @@ -8664,7 +8665,7 @@ vt_init_cfa_base (void)
>  static bool
>  vt_initialize (void)
>  {
> -  basic_block bb, prologue_bb = NULL;
> +  basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR);
>    HOST_WIDE_INT fp_cfa_offset = -1;
>  
>    alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
> @@ -8764,10 +8774,11 @@ vt_initialize (void)
>  	    }
>  	  if (elim != hard_frame_pointer_rtx)
>  	    fp_cfa_offset = -1;
> -	  else
> -	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
>  	}
> +      else
> +	fp_cfa_offset = -1;
>      }
> +
>    if (frame_pointer_needed)
>      {
>        rtx insn;
> @@ -8870,7 +8881,8 @@ vt_initialize (void)
>  		  if (bb == prologue_bb
>  		      && hard_frame_pointer_adjustment == -1
>  		      && RTX_FRAME_RELATED_P (insn)
> -		      && fp_setter (insn))
> +		      && fp_setter (insn)
> +		      && fp_cfa_offset != -1)
>  		    {
>  		      vt_init_cfa_base ();
>  		      hard_frame_pointer_adjustment = fp_cfa_offset;

While I had part of this in my patch too, it is unnecessary, if prologue_bb
is set to non-NULL only when we want to use it, then we know fp_cfa_offset is not -1.
By computing prologue_bb always, it will call fp_setter unnecessarily
for every insn in the prologue bb for !frame_pointer_needed or for DRAP.
I guess adding a comment instead of this would be better.

	Jakub