Richard Sandiford <[email protected]> writes:
> "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)))
To follow up: are the config/mips and rtlanal.cc changes needed
after the expr.cc change? Or were they trying to fix an issue that
you were seeing when the expr.cc change wasn't applied?
When I try locally, the expr.cc change seems to fix the issue for me.
Before that part of the patch, expand generated:
(insn 10 7 11 2 (set (zero_extract:DI (reg/v:DI 194 [ needleAddress+-4 ])
(const_int 16 [0x10])
(const_int 0 [0]))
(reg/v:DI 196 [ upper+-6 ])) "/tmp/foo.c":19:15 -1
(nil))
(insn 11 10 12 2 (set (reg:SI 198)
(truncate:SI (reg/v:DI 194 [ needleAddress+-4 ]))) "/tmp/foo.c":19:15 -1
(nil))
(insn 12 11 13 2 (set (reg/v:DI 194 [ needleAddress+-4 ])
(sign_extend:DI (reg:SI 198))) "/tmp/foo.c":19:15 -1
(nil))
(insn 13 12 14 2 (set (zero_extract:DI (reg/v:DI 194 [ needleAddress+-4 ])
(const_int 16 [0x10])
(const_int 16 [0x10]))
(reg/v:DI 197 [ lower+-6 ])) "/tmp/foo.c":20:15 -1
(nil))
[eventually return 194]
As you say, the truncation and extension were being applied to the
wrong half. This meant that things had already gone wrong by this point.
The function returns a u32, which for the MIPS 64-bit ABI must be returned
in sign-extended form. Since insn 13 changes the top bit of the u32,
it needed to be followed by a truncation to 32 bits and a sign extension
back to 64 bits.
AFAICT, combine correctly optimised this code. It was just a GIGO
situation: combine was optimising something that was already wrong.
In particular, the values returned by truncated_to_mode were correct,
including for the "u16 lower" parameter that was affected by the rtlanal.cc
part of the patch. The ABI guarantees that the upper 48 bits of $5
(the register that contains "lower") are zero, meaning that:
(a) truncation to 32 bits is a nop and
(b) a sign-extension of the low 32 bits can be optimised away.
With just the expr.cc patch, I get the code you quote above for octeon2.
If that's right, I think we should revert the config/mips and rtlanal.cc
parts.
On the expr.cc part: the test seems a bit indirect. After all the
cancellations, the BE check (rightly) reduces to "maybe_eq (bitpos, 0)",
so it might be simpler to write that directly. Not a strong opinion
though.
Thanks,
Richard
