Re: [middle-end PATCH take #2] Only call targetm.truly_noop_truncation for truncations.

[Richard Sandiford <richard.sandiford@arm.com>]

"Roger Sayle" <roger@nextmovesoftware.com> writes:
> Very many thanks (and a Happy New Year) to the pre-commit
> patch testing folks at linaro.org.   Their testing has revealed that
> although my patch is clean on x86_64, it triggers some problems
> on aarch64 and arm.  The issue (with the previous version of my
> patch) is that these platforms require a paradoxical subreg to be
> generated by the middle-end, where we were previously checking
> for truly_noop_truncation.
>
> This has been fixed (in revision 2) below.  Where previously I had:
>
> @@ -66,7 +66,9 @@ gen_lowpart_general (machine_mode mode, rtx x)
>        scalar_int_mode xmode;
>        if (is_a <scalar_int_mode> (GET_MODE (x), &xmode)
>           && GET_MODE_SIZE (xmode) <= UNITS_PER_WORD
> -         && TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode)
> +         && (known_lt (GET_MODE_SIZE (mode), GET_MODE_SIZE (xmode))
> +             ? TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode)
> +             : known_eq (GET_MODE_SIZE (mode), GET_MODE_SIZE (xmode)))
>           && !reload_completed)
>         return gen_lowpart_general (mode, force_reg (xmode, x));
>
> the correct change is:
>
>        scalar_int_mode xmode;
>        if (is_a <scalar_int_mode> (GET_MODE (x), &xmode)
>           && GET_MODE_SIZE (xmode) <= UNITS_PER_WORD
> -         && TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode)
> +         && (known_ge (GET_MODE_SIZE (mode), GET_MODE_SIZE (xmode))
> +             || TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode))
>           && !reload_completed)
>         return gen_lowpart_general (mode, force_reg (xmode, x));
>
> i.e. we only call TRULY_NOOP_TRUNCATION_MODES_P when we
> know we have a truncation, but the behaviour of non-truncations
> is preserved (no longer depends upon unspecified behaviour) and
> gen_lowpart_general is called to create the paradoxical SUBREG.
>
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures.  Ok for mainline?
>
> Hopefully this revision tests cleanly on the linaro.org CI pipeline.
>
> 2023-12-31  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         * combine.cc (make_extraction): Confirm that OUTPREC is less than
>         INPREC before calling TRULY_NOOP_TRUNCATION_MODES_P.
>         * expmed.cc (store_bit_field_using_insv): Likewise.
>         (extract_bit_field_using_extv): Likewise.
>         (extract_bit_field_as_subreg): Likewise.
>         * optabs-query.cc (get_best_extraction_insn): Likewise.
>         * optabs.cc (expand_parity): Likewise.
>         * rtlhooks.cc (gen_lowpart_general): Likewise.
>         * simplify-rtx.cc (simplify_truncation): Disallow truncations
>         to the same precision.
>         (simplify_unary_operation_1) <case TRUNCATE>: Move optimization
>         of truncations to the same mode earlier.
>
>
>> -----Original Message-----
>> From: Roger Sayle <roger@nextmovesoftware.com>
>> Sent: 28 December 2023 15:35
>> To: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
>> Cc: 'Jeff Law' <jeffreyalaw@gmail.com>
>> Subject: [middle-end PATCH] Only call targetm.truly_noop_truncation for
>> truncations.
>> 
>> 
>> The truly_noop_truncation target hook is documented, in target.def, as
> "true if it
>> is safe to convert a value of inprec bits to one of outprec bits (where
> outprec is
>> smaller than inprec) by merely operating on it as if it had only outprec
> bits", i.e.
>> the middle-end can use a SUBREG instead of a TRUNCATE.
>> 
>> What's perhaps potentially a little ambiguous in the above description is
> whether
>> it is the caller or the callee that's responsible for ensuring or checking
> whether
>> "outprec < inprec".  The name TRULY_NOOP_TRUNCATION_P, like
>> SUBREG_PROMOTED_P, may be prone to being understood as a predicate that
>> confirms that something is a no-op truncation or a promoted subreg, when
> in fact
>> the caller must first confirm this is a truncation/subreg and only then
> call the
>> "classification" macro.
>> 
>> Alas making the following minor tweak (for testing) to the i386 backend:
>> 
>> static bool
>> ix86_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec) {
>>   gcc_assert (outprec < inprec);
>>   return true;
>> }
>> 
>> #undef TARGET_TRULY_NOOP_TRUNCATION
>> #define TARGET_TRULY_NOOP_TRUNCATION ix86_truly_noop_truncation
>> 
>> reveals that there are numerous callers in middle-end that rely on the
> default
>> behaviour of silently returning true for any (invalid) input.
>> These are fixed below.
>> 
>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
>> make -k check, both with and without --target_board=unix{-m32} with no new
>> failures.  Ok for mainline?
>> 
>> 
>> 2023-12-28  Roger Sayle  <roger@nextmovesoftware.com>
>> 
>> gcc/ChangeLog
>>         * combine.cc (make_extraction): Confirm that OUTPREC is less than
>>         INPREC before calling TRULY_NOOP_TRUNCATION_MODES_P.
>>         * expmed.cc (store_bit_field_using_insv): Likewise.
>>         (extract_bit_field_using_extv): Likewise.
>>         (extract_bit_field_as_subreg): Likewise.
>>         * optabs-query.cc (get_best_extraction_insn): Likewise.
>>         * optabs.cc (expand_parity): Likewise.
>>         * rtlhooks.cc (gen_lowpart_general): Likewise.
>>         * simplify-rtx.cc (simplify_truncation): Disallow truncations
>>         to the same precision.
>>         (simplify_unary_operation_1) <case TRUNCATE>: Move optimization
>>         of truncations to the same mode earlier.
>> 
>> 
>> Thanks in advance,
>> Roger
>> --
>
> diff --git a/gcc/combine.cc b/gcc/combine.cc
> index f2c64a9..5aa2f57 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -7613,7 +7613,8 @@ make_extraction (machine_mode mode, rtx inner, HOST_WIDE_INT pos,
>  	   && (pos == 0 || REG_P (inner))
>  	   && (inner_mode == tmode
>  	       || !REG_P (inner)
> -	       || TRULY_NOOP_TRUNCATION_MODES_P (tmode, inner_mode)
> +	       || (known_lt (GET_MODE_SIZE (tmode), GET_MODE_SIZE (inner_mode))
> +		   && TRULY_NOOP_TRUNCATION_MODES_P (tmode, inner_mode))

It's not obvious that this has the effect of preserving the original
condition.  Previously TRULY_NOOP_TRUNCATION_MODES_P would return
true for anything that wasn't a truncation, so I think it was closer to:

	   && (inner_mode == tmode
	       || !REG_P (inner)
	       || maybe_ge (GET_MODE_SIZE (tmode), GET_MODE_SIZE (inner_mode))
	       || TRULY_NOOP_TRUNCATION_MODES_P (tmode, inner_mode)

In other words...

>  	       || reg_truncated_to_mode (tmode, inner))

...this should also only be tested for actual truncations.

>  	   && (! in_dest
>  	       || (REG_P (inner)
> @@ -7856,6 +7857,8 @@ make_extraction (machine_mode mode, rtx inner, HOST_WIDE_INT pos,
>        /* On the LHS, don't create paradoxical subregs implicitely truncating
>  	 the register unless TARGET_TRULY_NOOP_TRUNCATION.  */
>        if (in_dest
> +	  && known_lt (GET_MODE_SIZE (GET_MODE (inner)),
> +		       GET_MODE_SIZE (wanted_inner_mode))
>  	  && !TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (inner),
>  					     wanted_inner_mode))
>  	return NULL_RTX;
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index 05331dd..6398bf9 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -651,6 +651,7 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>       X) 0)) is (reg:N X).  */
>    if (GET_CODE (xop0) == SUBREG
>        && REG_P (SUBREG_REG (xop0))
> +      && paradoxical_subreg_p (xop0)
>        && !TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (SUBREG_REG (xop0)),
>  					 op_mode))
>      {
> @@ -1585,7 +1586,11 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,
>  	 mode.  Instead, create a temporary and use convert_move to set
>  	 the target.  */
>        if (REG_P (target)
> -	  && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)
> +	  && (known_lt (GET_MODE_SIZE (GET_MODE (target)),
> +			GET_MODE_SIZE (ext_mode))
> +	      ? TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)
> +	      : known_eq (GET_MODE_SIZE (GET_MODE (target)),
> +			  GET_MODE_SIZE (ext_mode)))
>  	  && (temp = gen_lowpart_if_possible (ext_mode, target)))
>  	{
>  	  target = temp;
> @@ -1626,7 +1631,9 @@ extract_bit_field_as_subreg (machine_mode mode, rtx op0,
>    if (multiple_p (bitnum, BITS_PER_UNIT, &bytenum)
>        && known_eq (bitsize, GET_MODE_BITSIZE (mode))
>        && lowpart_bit_field_p (bitnum, bitsize, op0_mode)
> -      && TRULY_NOOP_TRUNCATION_MODES_P (mode, op0_mode))
> +      && (known_lt (GET_MODE_SIZE (mode), GET_MODE_SIZE (op0_mode))
> +	  ? TRULY_NOOP_TRUNCATION_MODES_P (mode, op0_mode)
> +	  : known_eq (GET_MODE_SIZE (mode), GET_MODE_SIZE (op0_mode))))
>      return simplify_gen_subreg (mode, op0, op0_mode, bytenum);
>    return NULL_RTX;
>  }

I think these two should also use the known_ge formulation in the
covering letter.  Same for expand_parity.

We're then left with all the sites testing for essentially the same
thing, which is: can I change directly from mode A to mode B using
subregs?  It might be worth having a wrapper for that, rather than
requiring the known_ge in so many places.

Thanks,
Richard

> diff --git a/gcc/optabs-query.cc b/gcc/optabs-query.cc
> index 947ccef..f33253f 100644
> --- a/gcc/optabs-query.cc
> +++ b/gcc/optabs-query.cc
> @@ -213,7 +213,7 @@ get_best_extraction_insn (extraction_insn *insn,
>  	  FOR_EACH_MODE_FROM (mode_iter, mode)
>  	    {
>  	      mode = mode_iter.require ();
> -	      if (maybe_gt (GET_MODE_SIZE (mode), GET_MODE_SIZE (field_mode))
> +	      if (maybe_ge (GET_MODE_SIZE (mode), GET_MODE_SIZE (field_mode))
>  		  || TRULY_NOOP_TRUNCATION_MODES_P (insn->field_mode,
>  						    field_mode))
>  		break;
> diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> index 6a34276..fad0d59 100644
> --- a/gcc/optabs.cc
> +++ b/gcc/optabs.cc
> @@ -2954,7 +2954,11 @@ expand_parity (scalar_int_mode mode, rtx op0, rtx target)
>  	  if (temp)
>  	    {
>  	      if (mclass != MODE_INT
> -		  || !TRULY_NOOP_TRUNCATION_MODES_P (mode, wider_mode))
> +		  || (known_lt (GET_MODE_SIZE (mode),
> +				GET_MODE_SIZE (wider_mode))
> +		      ? !TRULY_NOOP_TRUNCATION_MODES_P (mode, wider_mode)
> +		      : maybe_ne (GET_MODE_SIZE (mode),
> +				  GET_MODE_SIZE (wider_mode))))
>  		return convert_to_mode (mode, temp, 0);
>  	      else
>  		return gen_lowpart (mode, temp);
> diff --git a/gcc/rtlhooks.cc b/gcc/rtlhooks.cc
> index 989d3c9..c3313fd 100644
> --- a/gcc/rtlhooks.cc
> +++ b/gcc/rtlhooks.cc
> @@ -66,7 +66,8 @@ gen_lowpart_general (machine_mode mode, rtx x)
>        scalar_int_mode xmode;
>        if (is_a <scalar_int_mode> (GET_MODE (x), &xmode)
>  	  && GET_MODE_SIZE (xmode) <= UNITS_PER_WORD
> -	  && TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode)
> +	  && (known_ge (GET_MODE_SIZE (mode), GET_MODE_SIZE (xmode))
> +	      || TRULY_NOOP_TRUNCATION_MODES_P (mode, xmode))
>  	  && !reload_completed)
>  	return gen_lowpart_general (mode, force_reg (xmode, x));
>  
> diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc
> index f3745d8..27518f5 100644
> --- a/gcc/simplify-rtx.cc
> +++ b/gcc/simplify-rtx.cc
> @@ -617,7 +617,7 @@ simplify_context::simplify_truncation (machine_mode mode, rtx op,
>    unsigned int op_precision = GET_MODE_UNIT_PRECISION (op_mode);
>    scalar_int_mode int_mode, int_op_mode, subreg_mode;
>  
> -  gcc_assert (precision <= op_precision);
> +  gcc_assert (precision < op_precision);
>  
>    /* Optimize truncations of zero and sign extended values.  */
>    if (GET_CODE (op) == ZERO_EXTEND
> @@ -1207,6 +1207,10 @@ simplify_context::simplify_unary_operation_1 (rtx_code code, machine_mode mode,
>        break;
>  
>      case TRUNCATE:
> +      /* Check for useless truncation.  */
> +      if (GET_MODE (op) == mode)
> +	return op;
> +
>        /* Don't optimize (lshiftrt (mult ...)) as it would interfere
>  	 with the umulXi3_highpart patterns.  */
>        if (GET_CODE (op) == LSHIFTRT
> @@ -1271,9 +1275,6 @@ simplify_context::simplify_unary_operation_1 (rtx_code code, machine_mode mode,
>  	    return temp;
>  	}
>  
> -      /* Check for useless truncation.  */
> -      if (GET_MODE (op) == mode)
> -	return op;
>        break;
>  
>      case FLOAT_TRUNCATE: