On Sun, Mar 8, 2026 at 4:52 PM Richard Sandiford <[email protected]> wrote: > > 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.
Right, I thought I originally did mention the issue was in expand; oh wait that was for PR 113179 (which is related and fixed a different way too). > > 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? Ok. Thanks, Andrew > > 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 >
