"Roger Sayle" <[email protected]> writes:
> Here is a refreshed version of my patch from February 2025, for resolving
> PR middle-end/118608, a wrong code on valid regression where the middle-end
> is failing to keep values suitably sign-extended (MIPS64 is a rare
> targetm.mode_rep_extended target, as well as being BYTES_BIG_ENDIAN).
>
> This fix requires three independent tweaks, one in each source file.
> The first tweak is that the logic in my triggering patch for determining
> whether store_field updates the most significant bit needs to be updated
> to handle BYTES_BIG_ENDIAN. Of the two insertions in the bugzilla test
> case, we were generating the sign extension after the wrong one.
> The second tweak was that this explicit sign-extension was then being
> eliminated during combine by simplify-rtx that believed the explicit
> TRUNCATE wasn't required. This patch updates truncated_to_mode to
> understand that on mode_rep_extended targets, TRUNCATE is used instead
> of SUBREG because it isn't a no-op. Finally, the third tweak is
> that the MIPS backend requires a small change to recognize (and split)
> *extenddi_truncatesi when TARGET_64BIT and !ISA_HAS_EXTS.
>
> On mips64-elf with -mabi=64 the following are now generated for
> prepareNeedle:
>
> -O2
> sll $5,$5,16
> jr $31
> or $2,$5,$4
>
> -Os
> dsll $5,$5,16
> or $2,$4,$5
> dsll $2,$2,32
> jr $31
> dsra $2,$2,32
>
> -O2 -march=octeon2
> move $2,$0
> ins $2,$5,16,16
> jr $31
> ins $2,$4,0,16
>
> -Os -march=octeon2
> move $2,$0
> dins $2,$4,0,16
> dins $2,$5,16,16
> jr $31
> sll $2,$2,0
>
>
> This patch has been tested by bootstrapping x86_64-pc-linux-gnu, to
> reconfirm
> that the middle-end changes to do harm, and by building cc1 for mips64-elf
> to confirm that the backend still builds. Very many thanks to Mateusz
> Marciniec for confirming that this patch resolves (or used to resolve) the
> issue on mips64 hardware. Ok for mainline?
>
>
> 2026-01-29 Roger Sayle <[email protected]>
>
> gcc/ChangeLog
> PR middle-end/118608
> * expr.cc (store_field_updates_msb_p): New helper function that
> now also handles BYTES_BIG_ENDIAN targets.
> (expand_assignment): Use the above function when deciding to emit
> a required sign/zero extension.
> * rtlanal.c (truncated_to_mode): Call targetm.mode_rep_extended
> to check whether an explicit TRUNCATE is required (i.e. performs
> an extension) on this target.
> * config/mips/mips.md (*extenddi_truncate<mode>): Handle all
> SUBDI modes, not just SHORT modes.
>
> gcc/testsuite/ChangeLog
> PR middle-end/118608
> * gcc.target/mips/pr118608-1.c: New test case.
> * gcc.target/mips/pr118608-2.c: Likewise.
> * gcc.target/mips/pr118608-3.c: Likewise.
> * gcc.target/mips/pr118608-4.c: Likewise.
>
>
> Thanks again, and sorry for the inconvenience.
> Roger
> --
>
> diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
> index 85e7d67901f..6a1b63b00d2 100644
> --- a/gcc/config/mips/mips.md
> +++ b/gcc/config/mips/mips.md
> @@ -3952,7 +3952,7 @@
> (define_insn_and_split "*extenddi_truncate<mode>"
> [(set (match_operand:DI 0 "register_operand" "=d")
> (sign_extend:DI
> - (truncate:SHORT (match_operand:DI 1 "register_operand" "d"))))]
> + (truncate:SUBDI (match_operand:DI 1 "register_operand" "d"))))]
> "TARGET_64BIT && !TARGET_MIPS16 && !ISA_HAS_EXTS"
> "#"
> "&& reload_completed"
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index b6d593d09a2..d7b0bfccb7d 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -5968,6 +5968,18 @@ mem_ref_refers_to_non_mem_p (tree ref)
> return non_mem_decl_p (base);
> }
>
> +/* Helper function of expand_assignment. Check if storing field of
> + size BITSIZE at position BITPOS overlaps with the most significant
> + bit of TO_RTX, known to be SUBREG_PROMOTED_VAR_P.
> + Updating this field requires an explicit extension. */
> +static bool
> +store_field_updates_msb_p (poly_int64 bitpos, poly_int64 bitsize, rtx to_rtx)
> +{
> + poly_int64 to_size = GET_MODE_SIZE (GET_MODE (to_rtx));
> + poly_int64 bitnum = BYTES_BIG_ENDIAN ? to_size - bitsize - bitpos : bitpos;
> + return maybe_eq (bitnum + bitsize, to_size);
> +}
> +
> /* Expand an assignment that stores the value of FROM into TO. If
> NONTEMPORAL
> is true, try generating a nontemporal store. */
>
> @@ -6314,8 +6326,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
> && known_eq (bitsize, GET_MODE_BITSIZE (GET_MODE (to_rtx))))
> result = store_expr (from, to_rtx, 0, nontemporal, false);
> /* Check if the field overlaps the MSB, requiring extension. */
> - else if (maybe_eq (bitpos + bitsize,
> - GET_MODE_BITSIZE (GET_MODE (to_rtx))))
> + else if (store_field_updates_msb_p (bitpos, bitsize, to_rtx))
> {
> scalar_int_mode imode = subreg_unpromoted_mode (to_rtx);
> scalar_int_mode omode = subreg_promoted_mode (to_rtx);
> diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
> index 27349a0a74f..37df8d33e89 100644
> --- a/gcc/rtlanal.cc
> +++ b/gcc/rtlanal.cc
> @@ -6200,6 +6200,17 @@ truncated_to_mode (machine_mode mode, const_rtx x)
> if (REG_P (x) && rtl_hooks.reg_truncated_to_mode (mode, x))
> return true;
>
> + /* This explicit TRUNCATE may be needed on targets that require
> + MODE to be suitably extended when stored in X. Targets such as
> + mips64 use (sign_extend:DI (truncate:SI (reg:DI x))) to perform
> + an explicit extension, avoiding use of (subreg:SI (reg:DI x))
> + which is assumed to already be extended. */
> + scalar_int_mode imode, omode;
> + if (is_a <scalar_int_mode> (mode, &imode)
> + && is_a <scalar_int_mode> (GET_MODE (x), &omode)
> + && targetm.mode_rep_extended (imode, omode) != UNKNOWN)
Sorry for the very late review, but: correctness tests like this should
be using TRULY_NOOP_TRUNCATION rather than targetm.mode_rep_extended.
TRULY_NOOP_TRUNCATION says whether (truncate ...) can be converted
to a subreg or (in the negative) whether (truncate ...) needs to be kept.
targetm.mode_rep_extended instead says whether we can optimise away certain
extensions, so should only be used for optimisation decisions.
But I don't understand the patch. The comment above truncated_to_mode says:
/* Suppose that truncation from the machine mode of X to MODE is not a
no-op. See if there is anything special about X so that we can
assume it already contains a truncated value of MODE. */
So it's already a given that we're dealing with a MIPS-like target.
The caller bypasses the functions for "normal" targets with:
&& (TRULY_NOOP_TRUNCATION_MODES_P (mode, GET_MODE (op))
|| truncated_to_mode (mode, op)))
Thanks,
Richard
> + return false;
> +
> /* See if we already satisfy the requirements of MODE. If yes we
> can just switch to MODE. */
> if (num_sign_bit_copies_in_rep[GET_MODE (x)][mode]
> diff --git a/gcc/testsuite/gcc.target/mips/pr118608-1.c
> b/gcc/testsuite/gcc.target/mips/pr118608-1.c
> new file mode 100644
> index 00000000000..e7384d7137d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr118608-1.c
> @@ -0,0 +1,45 @@
> +/* { dg-do run } */
> +/* { dg-options "-mabi=64 -O2" } */
> +
> +#define COUNT 10
> +
> +typedef unsigned short u16;
> +typedef unsigned int u32;
> +
> +typedef struct NeedleAddress
> +{
> + u16 nId;
> + u16 mId;
> +} NeedleAddress;
> +
> +u32 __attribute__ ((noinline)) prepareNeedle(const u16 upper, const u16
> lower)
> +{
> + u32 needleAddress = 0;
> + NeedleAddress *const addr = (NeedleAddress*)(&needleAddress);
> + addr->mId = upper;
> + addr->nId = lower;
> + return needleAddress;
> +}
> +
> +const u32* __attribute__ ((noinline)) findNeedle(const u32 needle, const
> u32* begin, const u32* end)
> +{
> + while ( begin != end && needle != *begin )
> + {
> + ++begin;
> + }
> + return begin;
> +}
> +
> +int main()
> +{
> + u32 needle = prepareNeedle(0xDCBA, 0xABCD);
> +
> + u32 haystack[COUNT] = {};
> + for (int i = 0; i < COUNT; i++)
> + haystack[i] = needle;
> +
> + const u32* result = findNeedle(needle, haystack, haystack + COUNT);
> + if (result == haystack + COUNT)
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/mips/pr118608-2.c
> b/gcc/testsuite/gcc.target/mips/pr118608-2.c
> new file mode 100644
> index 00000000000..a5bcfda6ca1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr118608-2.c
> @@ -0,0 +1,45 @@
> +/* { dg-do run } */
> +/* { dg-options "-mabi=64 -Os" } */
> +
> +#define COUNT 10
> +
> +typedef unsigned short u16;
> +typedef unsigned int u32;
> +
> +typedef struct NeedleAddress
> +{
> + u16 nId;
> + u16 mId;
> +} NeedleAddress;
> +
> +u32 __attribute__ ((noinline)) prepareNeedle(const u16 upper, const u16
> lower)
> +{
> + u32 needleAddress = 0;
> + NeedleAddress *const addr = (NeedleAddress*)(&needleAddress);
> + addr->mId = upper;
> + addr->nId = lower;
> + return needleAddress;
> +}
> +
> +const u32* __attribute__ ((noinline)) findNeedle(const u32 needle, const
> u32* begin, const u32* end)
> +{
> + while ( begin != end && needle != *begin )
> + {
> + ++begin;
> + }
> + return begin;
> +}
> +
> +int main()
> +{
> + u32 needle = prepareNeedle(0xDCBA, 0xABCD);
> +
> + u32 haystack[COUNT] = {};
> + for (int i = 0; i < COUNT; i++)
> + haystack[i] = needle;
> +
> + const u32* result = findNeedle(needle, haystack, haystack + COUNT);
> + if (result == haystack + COUNT)
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/mips/pr118608-3.c
> b/gcc/testsuite/gcc.target/mips/pr118608-3.c
> new file mode 100644
> index 00000000000..f017e4e858c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr118608-3.c
> @@ -0,0 +1,45 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mabi=64 -O2 -march=octeon2" } */
> +
> +#define COUNT 10
> +
> +typedef unsigned short u16;
> +typedef unsigned int u32;
> +
> +typedef struct NeedleAddress
> +{
> + u16 nId;
> + u16 mId;
> +} NeedleAddress;
> +
> +u32 __attribute__ ((noinline)) prepareNeedle(const u16 upper, const u16
> lower)
> +{
> + u32 needleAddress = 0;
> + NeedleAddress *const addr = (NeedleAddress*)(&needleAddress);
> + addr->mId = upper;
> + addr->nId = lower;
> + return needleAddress;
> +}
> +
> +const u32* __attribute__ ((noinline)) findNeedle(const u32 needle, const
> u32* begin, const u32* end)
> +{
> + while ( begin != end && needle != *begin )
> + {
> + ++begin;
> + }
> + return begin;
> +}
> +
> +int main()
> +{
> + u32 needle = prepareNeedle(0xDCBA, 0xABCD);
> +
> + u32 haystack[COUNT] = {};
> + for (int i = 0; i < COUNT; i++)
> + haystack[i] = needle;
> +
> + const u32* result = findNeedle(needle, haystack, haystack + COUNT);
> + if (result == haystack + COUNT)
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/mips/pr118608-4.c
> b/gcc/testsuite/gcc.target/mips/pr118608-4.c
> new file mode 100644
> index 00000000000..4c96e378e0f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr118608-4.c
> @@ -0,0 +1,45 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mabi=64 -Os -march=octeon2" } */
> +
> +#define COUNT 10
> +
> +typedef unsigned short u16;
> +typedef unsigned int u32;
> +
> +typedef struct NeedleAddress
> +{
> + u16 nId;
> + u16 mId;
> +} NeedleAddress;
> +
> +u32 __attribute__ ((noinline)) prepareNeedle(const u16 upper, const u16
> lower)
> +{
> + u32 needleAddress = 0;
> + NeedleAddress *const addr = (NeedleAddress*)(&needleAddress);
> + addr->mId = upper;
> + addr->nId = lower;
> + return needleAddress;
> +}
> +
> +const u32* __attribute__ ((noinline)) findNeedle(const u32 needle, const
> u32* begin, const u32* end)
> +{
> + while ( begin != end && needle != *begin )
> + {
> + ++begin;
> + }
> + return begin;
> +}
> +
> +int main()
> +{
> + u32 needle = prepareNeedle(0xDCBA, 0xABCD);
> +
> + u32 haystack[COUNT] = {};
> + for (int i = 0; i < COUNT; i++)
> + haystack[i] = needle;
> +
> + const u32* result = findNeedle(needle, haystack, haystack + COUNT);
> + if (result == haystack + COUNT)
> + __builtin_abort ();
> + return 0;
> +}
