Sorry to be awkward, but I'd like to revert the rtlanal.cc and
config/mips/mips.md parts of r16-7265-ga9e48eca3a6eef. I think
the expr.cc part of that patch is enough to fix the bug. The other
parts seem unnecessary and are likely to regress code quality on MIPS
compared to previous releases. (See the testing below for examples.)
The rtlanal.cc part added the following code to truncated_to_mode:
/* 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)
return false;
I think this has two problems. The first is that mode_rep_extended
describes a canonical form that is obtained by correctly honouring
TARGET_TRULY_NOOP_TRUNCATION. It is not an independent restriction
on what RTL optimisers can do. If we need to disable an optimisation
on MIPS-like targets, the restrictions should be based on
TARGET_TRULY_NOOP_TRUNCATION instead.
The second problem is that, although the comment treats MIPS-like
DI->SI truncation as a special case, truncated_to_mode is specifically
written for such cases. The comment above the function 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. */
Thus we're already in the realm of MIPS-like truncations that need
TRUNCATE rather than SUBREG (and that in turn guarantee sign-extension
in some cases). It's the caller that checks for that condition:
&& (TRULY_NOOP_TRUNCATION_MODES_P (mode, GET_MODE (op))
|| truncated_to_mode (mode, op)))
So I think the patch has the effect of disabling exactly the kind of
optimisation that truncated_to_mode is supposed to provide.
truncated_to_mode makes an implicit assumption that sign-extension is
enough to allow a SUBREG to be used in place of a TRUNCATE. This is
true for MIPS and was true for the old SH64 port. I don't know whether
it's true for gcn and nvptx, although I assume that it must be, since
no-one seems to have complained. However, it would not be true for a
port that required zero rather than sign extension (which AFAIK we've
never had).
It's probably worth noting that this assumption is in the opposite
direction from what mode_rep_extended describes. mode_rep_extended
says that "proper" truncation leads to a guarantee of sign extension.
truncated_for_mode assumes that sign extension avoids the need for
"proper" truncation. On MIPS, the former is only true for truncation
from 64 bits to 32 bits, whereas the latter is true for all cases (such
as 64 bits to 16 bits).
And that feeds into the mips.md change in r16-7265-ga9e48eca3a6eef.
The change was:
(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"
The old :SHORT pattern existed because QI and HI values are only
guaranteed to be sign-extensions of bit 31 of the register, not bits
7 or 15 (respectively). Thus we have the worst of both worlds:
(1) truncation from DI is not a nop. It requires a left shift by
at least 32 bits and a right shift by the same amount.
(2) sign extension to DI is not a nop. It requires a left shift and
a right shift in the normal way (by 56 bits for QI and 48 bits
for HI).
So a separate truncation and extension would yield four shifts.
The pattern above exists to reduce this to two shifts, since (2)
subsumes (1).
But the :SI case is different:
(1) truncation from DI is not a nop. It requires a left shift by 32
and a right shift by 32, as above.
(2) sign extension from SI to DI is a nop.
(2) is implemented by:
;; When TARGET_64BIT, all SImode integer and accumulator registers
;; should already be in sign-extended form (see TARGET_TRULY_NOOP_TRUNCATION
;; and truncdisi2). We can therefore get rid of register->register
;; instructions if we constrain the source to be in the same register as
;; the destination.
;;
;; Only the pre-reload scheduler sees the type of the register alternatives;
;; we split them into nothing before the post-reload scheduler runs.
;; These alternatives therefore have type "move" in order to reflect
;; what happens if the two pre-reload operands cannot be tied, and are
;; instead allocated two separate GPRs. We don't distinguish between
;; the GPR and LO cases because we don't usually know during pre-reload
;; scheduling whether an operand will be LO or not.
(define_insn_and_split "extendsidi2"
[(set (match_operand:DI 0 "register_operand" "=d,l,d")
(sign_extend:DI (match_operand:SI 1 "nonimmediate_operand" "0,0,m")))]
"TARGET_64BIT"
"@
#
#
lw\t%0,%1"
"&& reload_completed && register_operand (operands[1], VOIDmode)"
[(const_int 0)]
{
emit_note (NOTE_INSN_DELETED);
DONE;
}
[(set_attr "move_type" "move,move,load")
(set_attr "mode" "DI")])
So extending the first pattern above from :SHORT to :SUBDI is not really
an optimisation, in the sense that it doesn't add new information.
Not providing the combination allows the truncation or sign-extension
to be optimised with surrounding code.
I suppose the argument in favour of going from :SHORT to :SUBDI is
that it might avoid a move in some cases. But (a) I think that would
need to be measured further, (b) it might instead mean that the
extendsidi2 pattern needs to be tweaked for modern RA choices,
and (c) it doesn't really feel like stage 4 material.
I can understand where the changes came from. The output of combine
was clearly wrong before r16-7265-ga9e48eca3a6eef. And what combine
did looked bad. But I don't think combine itself did anything wrong.
IMO, all it did was expose the problems in the existing RTL. Expand
dropped a necessary sign-extension and the rest flowed from there.
In particular, the old decisions based on truncated_to_mode seemed
correct. The thing that the truncated_to_mode patch changed was the
assumption that a 64-bit register containing a "u16 lower" parameter
could be truncated with a SUBREG. And that's true, since it's
guaranteed by the ABI. The parameter is zero-extended from bit 16
and so the register contains a sign extension of bit 16 (i.e. 0).
And that was the information that truncated_to_mode was using.
I tested the patch on mips64-linux-gnu (all 3 ABIs). The patch fixes
regressions in:
- gcc.target/mips/octeon-exts-7.c (n32 & 64)
- gcc.target/mips/truncate-1.c (n32 & 64)
- gcc.target/mips/truncate-2.c (n32)
- gcc.target/mips/truncate-6.c (64)
OK to install?
Thanks,
Richard
gcc/
Revert:
2026-02-03 Roger Sayle <[email protected]>
PR middle-end/118608
* rtlanal.cc (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/config/mips/mips.md | 2 +-
gcc/rtlanal.cc | 11 -----------
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index 6a1b63b00d2..85e7d67901f 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:SUBDI (match_operand:DI 1 "register_operand" "d"))))]
+ (truncate:SHORT (match_operand:DI 1 "register_operand" "d"))))]
"TARGET_64BIT && !TARGET_MIPS16 && !ISA_HAS_EXTS"
"#"
"&& reload_completed"
diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
index c5062ab7715..88561a54e5a 100644
--- a/gcc/rtlanal.cc
+++ b/gcc/rtlanal.cc
@@ -6201,17 +6201,6 @@ 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)
- 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]
--
2.53.0
