Re: [PATCH v3] EXPR: Emit an truncate if 31+ bits polluted for SImode

[Jeff Law <jeffreyalaw@gmail.com>]

On 12/23/23 01:58, YunQiang Su wrote:
> On TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true platforms,
> if 31 or above bits is polluted by an bitops, we will need an
> truncate. Let's emit one, and mark let's use the same hardreg
> as in and out, the RTL may like:
> 
> (insn 21 20 24 2 (set (subreg/s/u:SI (reg/v:DI 200 [ val ]) 0)
>          (truncate:SI (reg/v:DI 200 [ val ]))) "../xx.c":7:29 -1
>       (nil))
> 
> We use /s/u flags to mark it as really needed, as in
> combine_simplify_rtx, this insn may be considered as truncated,
> so let's skip this combination.
> 
> gcc/ChangeLog:
>          PR: 104914.
>          * combine.cc (try_combine): Skip combine with truncate if
> 	dest is subreg and has /u/s flags on platforms
> 	TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true.
> 	* expr.cc (expand_assignment): Emit a truncate insn, if
> 	31+ bits is polluted for SImode.
> 
> gcc/testsuite/ChangeLog:
> 	PR: 104914.
> 	* gcc.target/mips/pr104914.c: New testcase.
I would suggest you show the RTL before/after whatever transformation 
has caused problems on your target and explain why you think the 
transformation is incorrect.

Focus on the RTL semantics as well as the target specific semantics 
because both are critically important here.

I strongly suspect you're just papering over a problem elsewhere.


> ---
>   gcc/combine.cc                           | 23 +++++++++++++++++++++-
>   gcc/expr.cc                              | 17 ++++++++++++++++
>   gcc/testsuite/gcc.target/mips/pr104914.c | 25 ++++++++++++++++++++++++
>   3 files changed, 64 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.target/mips/pr104914.c
> 
> diff --git a/gcc/combine.cc b/gcc/combine.cc
> index 1cda4dd57f2..04b9c414053 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -3294,6 +3294,28 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
>         n_occurrences = 0;		/* `subst' counts here */
>         subst_low_luid = DF_INSN_LUID (i2);
>   
> +      /* Don't try to combine a TRUNCATE INSN, if it's DEST is SUBREG and has
> +	 FLAG /s/u.  We use these 2 flags to mark this INSN as really needed:
> +	 normally, it means that the bits of 31+ of this variable is polluted
> +	 by a bitops.  The reason of existing of case (subreg:SI (reg:DI)) is
> +	 that, the same hardreg may act as src and dest.  */
> +      if (TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)
> +	  && INSN_P (i2))
> +	{
> +	  rtx i2dest_o = SET_DEST (PATTERN (i2));
> +	  rtx i2src_o = SET_SRC (PATTERN (i2));
> +	  if (GET_CODE (i2dest_o) == SUBREG
> +	      && GET_MODE (i2dest_o) == SImode
> +	      && GET_MODE (SUBREG_REG (i2dest_o)) == DImode
> +	      && SUBREG_PROMOTED_VAR_P (i2dest_o)
> +	      && SUBREG_PROMOTED_GET (i2dest_o) == SRP_SIGNED
> +	      && GET_CODE (i2src_o) == TRUNCATE
> +	      && GET_MODE (i2src_o) == SImode
> +	      && rtx_equal_p (SUBREG_REG (i2dest_o), XEXP (i2src_o, 0))
> +	      )
> +	    return 0;
> +	}
So checking SI/DI like this is just wrong.  There's nothing special 
about SI/DI.    Checking for equality between the destination and source 
also seems wrong -- if the state of the sign bit is wrong, it's wrong 
regardless of whether or not the source/destination register is the same.


>
> @@ -5326,7 +5348,6 @@ find_split_point (rtx *loc, rtx_insn *insn, bool set_src)
>   
>      UNIQUE_COPY is true if each substitution must be unique.  We do this
>      by copying if `n_occurrences' is nonzero.  */
> -
>   static rtx
>   subst (rtx x, rtx from, rtx to, bool in_dest, bool in_cond, bool unique_copy)
>   {
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 9fef2bf6585..f7236040a34 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -6284,6 +6284,23 @@ expand_assignment (tree to, tree from, bool nontemporal)
>   					nontemporal, reversep);
>   		  convert_move (SUBREG_REG (to_rtx), to_rtx1,
>   				SUBREG_PROMOTED_SIGN (to_rtx));
> +
> +		  rtx last = get_last_insn ();
> +		  if (TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)
> +		      && known_ge (bitregion_end, 31)
> +		      && SUBREG_PROMOTED_VAR_P (to_rtx)
> +		      && SUBREG_PROMOTED_SIGN (to_rtx) == SRP_SIGNED
> +		      && GET_MODE (to_rtx) == SImode
> +		      && GET_MODE (SUBREG_REG (to_rtx)) == DImode
> +		      && GET_CODE (SET_SRC (PATTERN (last))) == SIGN_EXTEND
> +		      )
> +		    {
> +		      insn_code icode = convert_optab_handler
> +						(trunc_optab, SImode, DImode);
> +		      if (icode != CODE_FOR_nothing)
> +			emit_unop_insn (icode, to_rtx,
> +					SUBREG_REG (to_rtx), TRUNCATE);
> +		    }
Similar comments about the modes apply here.

But again, my sense is there's a higher level problem here and that 
these changes are just papering over it.

Jeff