Re: [PR103302] skip multi-word pre-move clobber during lra

[Alexandre Oliva <oliva@adacore.com>]

On Mar  1, 2022, Alexandre Oliva <oliva@adacore.com> wrote:

> On Feb 23, 2022, Alexandre Oliva <oliva@adacore.com> wrote:
>> On Feb 21, 2022, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> Ok to revert commit r12-5852-g50e8b0c9bca6cdc57804f860ec5311b641753fbb

>>> OK.  Please re-open the bug as appropriate.

>> Thanks.  I've reopened it.  Here's what I'm installing.  I'm not
>> reverting the testcase, since it stopped failing even before the patch
>> was put in.

> And now here's a patch that fixes the underlying issue.

Oops, I missed the important question:


> Undo multi-word optional reloads correctly

> Unlike e.g. remove_inheritance_pseudos, undo_optional_reloads didn't
> deal with subregs, so instead of removing multi-word moves, it
> replaced the reload pseudo with the original pseudo.  Besides the
> redundant move, that retained the clobber of the dest, that starts a
> multi-word move.  After the remap, the sequence that should have
> become a no-op move starts by clobbering the original pseudo and then
> moving its pieces onto themselves.  The problem is the clobber: it
> makes earlier sets of the original pseudo to be regarded as dead: if
> the optional reload sequence was an output reload, the insn for which
> the output reload was attempted may be regarded as dead and deleted.

> I've arranged for undo_optional_reloads to accept SUBREGs and use
> get_regno, like remove_inheritance_pseudo, adjusted its insn-removal
> loop to tolerate iterating over a removed clobber, and added logic to
> catch any left-over reload clobbers that could trigger the problem.

Regstrapped on x86_64-linux-gnu, also tested on various riscv and arm
targets (with gcc-11).  Ok to install?


> for  gcc/ChangeLog

> 	* lra-constraints.cc (undo_optional_reloads): Recognize and
> 	drop insns of multi-word move sequences, tolerate removal
> 	iteration on an already-removed clobber, and refuse to
> 	substitute original pseudos into clobbers.
> ---
>  gcc/lra-constraints.cc |   37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)

> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index b2c4590153c4c..5cd89e7eeddc2 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -7261,15 +7261,17 @@ undo_optional_reloads (void)
>  	      continue;
>  	    src = SET_SRC (set);
>  	    dest = SET_DEST (set);
> -	    if (! REG_P (src) || ! REG_P (dest))
> +	    if ((! REG_P (src) && ! SUBREG_P (src))
> +		|| (! REG_P (dest) && ! SUBREG_P (dest)))
>  	      continue;
> -	    if (REGNO (dest) == regno
> +	    if (get_regno (dest) == (int) regno
>  		/* Ignore insn for optional reloads itself.  */
> -		&& REGNO (lra_reg_info[regno].restore_rtx) != REGNO (src)
> +		&& (get_regno (lra_reg_info[regno].restore_rtx)
> +		    != get_regno (src))
>  		/* Check only inheritance on last inheritance pass.  */
> -		&& (int) REGNO (src) >= new_regno_start
> +		&& get_regno (src) >= new_regno_start
>  		/* Check that the optional reload was inherited.  */
> -		&& bitmap_bit_p (&lra_inheritance_pseudos, REGNO (src)))
> +		&& bitmap_bit_p (&lra_inheritance_pseudos, get_regno (src)))
>  	      {
>  		keep_p = true;
>  		break;
> @@ -7291,18 +7293,22 @@ undo_optional_reloads (void)
>        bitmap_copy (insn_bitmap, &lra_reg_info[regno].insn_bitmap);
>        EXECUTE_IF_SET_IN_BITMAP (insn_bitmap, 0, uid, bi2)
>  	{
> +	  /* We may have already removed a clobber.  */
> +	  if (!lra_insn_recog_data[uid])
> +	    continue;
>  	  insn = lra_insn_recog_data[uid]->insn;
>  	  if ((set = single_set (insn)) != NULL_RTX)
>  	    {
>  	      src = SET_SRC (set);
>  	      dest = SET_DEST (set);
> -	      if (REG_P (src) && REG_P (dest)
> -		  && ((REGNO (src) == regno
> -		       && (REGNO (lra_reg_info[regno].restore_rtx)
> -			   == REGNO (dest)))
> -		      || (REGNO (dest) == regno
> -			  && (REGNO (lra_reg_info[regno].restore_rtx)
> -			      == REGNO (src)))))
> +	      if ((REG_P (src) || SUBREG_P (src))
> +		  && (REG_P (dest) || SUBREG_P (dest))
> +		  && ((get_regno (src) == (int) regno
> +		       && (get_regno (lra_reg_info[regno].restore_rtx)
> +			   == get_regno (dest)))
> +		      || (get_regno (dest) == (int) regno
> +			  && (get_regno (lra_reg_info[regno].restore_rtx)
> +			      == get_regno (src)))))
>  		{
>  		  if (lra_dump_file != NULL)
>  		    {
> @@ -7310,7 +7316,7 @@ undo_optional_reloads (void)
>  			       INSN_UID (insn));
>  		      dump_insn_slim (lra_dump_file, insn);
>  		    }
> -		  delete_move_and_clobber (insn, REGNO (dest));
> +		  delete_move_and_clobber (insn, get_regno (dest));
>  		  continue;
>  		}
>  	      /* We should not worry about generation memory-memory
> @@ -7319,6 +7325,11 @@ undo_optional_reloads (void)
>  		 we remove the inheritance pseudo and the optional
>  		 reload.  */
>  	    }
> +	  if (GET_CODE (PATTERN (insn)) == CLOBBER
> +	      && REG_P (SET_DEST (insn))
> +	      && get_regno (SET_DEST (insn)) == (int) regno)
> +	    /* Refuse to remap clobbers to preexisting pseudos.  */
> +	    gcc_unreachable ();
>  	  lra_substitute_pseudo_within_insn
>  	    (insn, regno, lra_reg_info[regno].restore_rtx, false);
>  	  lra_update_insn_regno_info (insn);


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>