Re: PR 61628: Invalid sharing of DECL_INCOMING_RTL

[Richard Sandiford <rdsandiford at googlemail dot com>]

Ping.  Original message was here:

  https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01267.html

Richard Sandiford <rdsandiford@googlemail.com> writes:
> My patch to reduce the amount of rtx garbage created:
>
> 2014-05-17  Richard Sandiford  <rdsandiford@googlemail.com>
>
> 	* emit-rtl.h (replace_equiv_address, replace_equiv_address_nv): Add an
> 	inplace argument.  Store the new address in the original MEM when true.
> 	* emit-rtl.c (change_address_1): Likewise.
> 	(adjust_address_1, adjust_automodify_address_1, offset_address):
> 	Update accordingly.
> 	* rtl.h (plus_constant): Add an inplace argument.
> 	* explow.c (plus_constant): Likewise.  Try to reuse the original PLUS
> 	when true.  Avoid generating (plus X (const_int 0)).
> 	* function.c (instantiate_virtual_regs_in_rtx): Adjust the PLUS
> 	in-place.  Pass true to plus_constant.
> 	(instantiate_virtual_regs_in_insn): Pass true to replace_equiv_address.
>
> exposed a case where a DECL_INCOMING_RTL MEM rtx was being reused in insn
> patterns without being copied.  This meant that instantiating virtual
> registers changed the DECL_INCOMING_RTL too.
>
> The patch fixes this by adding the missing copy_rtxes.  However,
> validize_mem has a bit of an awkward interface as far as sharing goes.
> If the MEM is already valid, validize_mem returns the original rtx,
> but if the MEM is not valid it creates a new one.  This means that if
> you copy first you create garbage rtl if the MEM was invalid, whereas if
> you don't copy first you get invalid sharing if the MEM was valid.
>
> Obviously we need to err on the side of copying first, to avoid the
> invalid sharing.  The patch therefore changes the interface so that
> validize_mem can modify the MEM in-place.
>
> I went through all calls to validize_mem to try to find cases where
> this might cause a problem.  The patch fixes up the ones I could see.
> Most callers already copy first, so as well fixing the bug, this seems
> to reduce the amount of garbage created.
>
> Tested on x86_64-linux-gnu, sparc-sun-solaris2.1? and
> powerpc64-unknown-linux-gnu.  OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> 	PR middle-end/61268
> 	* function.c (assign_parm_setup_reg): Prevent invalid sharing of
> 	DECL_INCOMING_RTL and entry_parm.
> 	(get_arg_pointer_save_area): Likewise arg_pointer_save_area.
> 	* calls.c (load_register_parameters): Likewise argument values.
> 	(emit_library_call_value_1, store_one_arg): Likewise argument
> 	save areas.
> 	* config/i386/i386.c (assign_386_stack_local): Likewise the local
> 	stack slot.
> 	* explow.c (validize_mem): Modify the argument in-place.
>
> Index: gcc/function.c
> ===================================================================
> --- gcc/function.c	2014-07-11 11:55:10.495121493 +0100
> +++ gcc/function.c	2014-07-18 08:57:07.047215306 +0100
> @@ -2662,13 +2662,14 @@ assign_parm_adjust_entry_rtl (struct ass
>        /* Handle calls that pass values in multiple non-contiguous
>  	 locations.  The Irix 6 ABI has examples of this.  */
>        if (GET_CODE (entry_parm) == PARALLEL)
> -	emit_group_store (validize_mem (stack_parm), entry_parm,
> +	emit_group_store (validize_mem (copy_rtx (stack_parm)), entry_parm,
>  			  data->passed_type,
>  			  int_size_in_bytes (data->passed_type));
>        else
>  	{
>  	  gcc_assert (data->partial % UNITS_PER_WORD == 0);
> -	  move_block_from_reg (REGNO (entry_parm), validize_mem (stack_parm),
> +	  move_block_from_reg (REGNO (entry_parm),
> +			       validize_mem (copy_rtx (stack_parm)),
>  			       data->partial / UNITS_PER_WORD);
>  	}
>  
> @@ -2837,7 +2838,7 @@ assign_parm_setup_block (struct assign_p
>        else
>  	gcc_assert (!size || !(PARM_BOUNDARY % BITS_PER_WORD));
>  
> -      mem = validize_mem (stack_parm);
> +      mem = validize_mem (copy_rtx (stack_parm));
>  
>        /* Handle values in multiple non-contiguous locations.  */
>        if (GET_CODE (entry_parm) == PARALLEL)
> @@ -2972,7 +2973,7 @@ assign_parm_setup_reg (struct assign_par
>       assign_parm_find_data_types and expand_expr_real_1.  */
>  
>    equiv_stack_parm = data->stack_parm;
> -  validated_mem = validize_mem (data->entry_parm);
> +  validated_mem = validize_mem (copy_rtx (data->entry_parm));
>  
>    need_conversion = (data->nominal_mode != data->passed_mode
>  		     || promoted_nominal_mode != data->promoted_mode);
> @@ -3228,7 +3229,7 @@ assign_parm_setup_stack (struct assign_p
>        /* Conversion is required.  */
>        rtx tempreg = gen_reg_rtx (GET_MODE (data->entry_parm));
>  
> -      emit_move_insn (tempreg, validize_mem (data->entry_parm));
> +      emit_move_insn (tempreg, validize_mem (copy_rtx (data->entry_parm)));
>  
>        push_to_sequence2 (all->first_conversion_insn, all->last_conversion_insn);
>        to_conversion = true;
> @@ -3265,8 +3266,8 @@ assign_parm_setup_stack (struct assign_p
>  	  set_mem_attributes (data->stack_parm, parm, 1);
>  	}
>  
> -      dest = validize_mem (data->stack_parm);
> -      src = validize_mem (data->entry_parm);
> +      dest = validize_mem (copy_rtx (data->stack_parm));
> +      src = validize_mem (copy_rtx (data->entry_parm));
>  
>        if (MEM_P (src))
>  	{
> @@ -5261,7 +5262,7 @@ get_arg_pointer_save_area (void)
>  	 generated stack slot may not be a valid memory address, so we
>  	 have to check it and fix it if necessary.  */
>        start_sequence ();
> -      emit_move_insn (validize_mem (ret),
> +      emit_move_insn (validize_mem (copy_rtx (ret)),
>                        crtl->args.internal_arg_pointer);
>        seq = get_insns ();
>        end_sequence ();
> Index: gcc/calls.c
> ===================================================================
> --- gcc/calls.c	2014-06-22 10:46:24.659598553 +0100
> +++ gcc/calls.c	2014-07-18 08:57:07.123215990 +0100
> @@ -1937,7 +1937,7 @@ load_register_parameters (struct arg_dat
>  
>  	  else if (partial == 0 || args[i].pass_on_stack)
>  	    {
> -	      rtx mem = validize_mem (args[i].value);
> +	      rtx mem = validize_mem (copy_rtx (args[i].value));
>  
>  	      /* Check for overlap with already clobbered argument area,
>  	         providing that this has non-zero size.  */
> @@ -4014,7 +4014,8 @@ emit_library_call_value_1 (int retval, r
>  					     argvec[argnum].locate.size.constant
>  					     );
>  
> -		      emit_block_move (validize_mem (argvec[argnum].save_area),
> +		      emit_block_move (validize_mem
> +				         (copy_rtx (argvec[argnum].save_area)),
>  				       stack_area,
>  				       GEN_INT (argvec[argnum].locate.size.constant),
>  				       BLOCK_OP_CALL_PARM);
> @@ -4289,7 +4290,8 @@ emit_library_call_value_1 (int retval, r
>  
>  	    if (save_mode == BLKmode)
>  	      emit_block_move (stack_area,
> -			       validize_mem (argvec[count].save_area),
> +			       validize_mem
> +			         (copy_rtx (argvec[count].save_area)),
>  			       GEN_INT (argvec[count].locate.size.constant),
>  			       BLOCK_OP_CALL_PARM);
>  	    else
> @@ -4433,7 +4435,8 @@ store_one_arg (struct arg_data *arg, rtx
>  		  arg->save_area
>  		    = assign_temp (TREE_TYPE (arg->tree_value), 1, 1);
>  		  preserve_temp_slots (arg->save_area);
> -		  emit_block_move (validize_mem (arg->save_area), stack_area,
> +		  emit_block_move (validize_mem (copy_rtx (arg->save_area)),
> +				   stack_area,
>  				   GEN_INT (arg->locate.size.constant),
>  				   BLOCK_OP_CALL_PARM);
>  		}
> Index: gcc/config/i386/i386.c
> ===================================================================
> --- gcc/config/i386/i386.c	2014-07-15 20:49:20.901473328 +0100
> +++ gcc/config/i386/i386.c	2014-07-18 08:57:07.110215873 +0100
> @@ -25073,7 +25073,7 @@ assign_386_stack_local (enum machine_mod
>  
>    s->next = ix86_stack_locals;
>    ix86_stack_locals = s;
> -  return validize_mem (s->rtl);
> +  return validize_mem (copy_rtx (s->rtl));
>  }
>  
>  static void
> Index: gcc/explow.c
> ===================================================================
> --- gcc/explow.c	2014-06-22 10:46:24.659598553 +0100
> +++ gcc/explow.c	2014-07-18 08:57:07.122215981 +0100
> @@ -518,8 +518,9 @@ memory_address_addr_space (enum machine_
>    return x;
>  }
>  
> -/* Convert a mem ref into one with a valid memory address.
> -   Pass through anything else unchanged.  */
> +/* If REF is a MEM with an invalid address, change it into a valid address.
> +   Pass through anything else unchanged.  REF must be an unshared rtx and
> +   the function may modify it in-place.  */
>  
>  rtx
>  validize_mem (rtx ref)
> @@ -531,8 +532,7 @@ validize_mem (rtx ref)
>  				   MEM_ADDR_SPACE (ref)))
>      return ref;
>  
> -  /* Don't alter REF itself, since that is probably a stack slot.  */
> -  return replace_equiv_address (ref, XEXP (ref, 0));
> +  return replace_equiv_address (ref, XEXP (ref, 0), true);
>  }
>  
>  /* If X is a memory reference to a member of an object block, try rewriting