Andrew Stubbs <[email protected]> writes: > On 12/10/2026 13:37, Richard Sandiford wrote: >> Andrew Stubbs <[email protected]> writes: >>> On 12/10/2026 13:15, Richard Sandiford wrote: >>>> Andrew Stubbs <[email protected]> writes: >>>>> On 09/10/2026 16:22, Richard Sandiford wrote: >>>>>> Andrew Stubbs <[email protected]> writes: >>>>>>> On 09/10/2026 12:15, Richard Sandiford wrote: >>>>>>>> Sorry, a couple of other things: >>>>>>>> >>>>>>>>>> + >>>>>>>>>> + rtx new_addr >>>>>>>>>> + = (REG_P (addr) >>>>>>>>>> + ? simplify_gen_subreg (addr_elt_mode, addr, addrmode, >>>>>>>>>> + addr_byte) >>>>>>>>>> + : simplify_subreg (addr_elt_mode, addr, addrmode, addr_byte)); >>>>>>>> >>>>>>>> I think we should use simplify_gen_subreg unconditionally unless >>>>>>>> there's >>>>>>>> a specific reason not to. This is a simplification regardless of what >>>>>>>> the address is. >>>>>>> >>>>>>> I tried this and got an ICE. AFAICT, simplify_gen_subreg does the wrong >>>>>>> thing (in this situation) when the address has an offset: >>>>>>> >>>>>>> (mem/i:V8QI (plus:V8DI (reg:V8DI 720) >>>>>>> (vec_duplicate:V8DI (const_int 0)))) >>>>>>> >>>>>>> It tries to wrap the plus in a subreg, which is not valid. So, I'm going >>>>>>> to leave this bit as-is, unless you have a better suggestion? >>>>>> >>>>>> Why's it not valid though? >>>>> gcn_addr_space_legitimate_address_p rejects it because there's no >>>>> instruction that can do that in a load/store instruction. >>>> >>>> Yeah, I agree that it should do that. >>>> >>>>> Is that not how that's supposed to work? >>>> >>>> Yeah, gcn_addr_space_legitimate_address_p should describe the >>>> target restrictions. >>>> >>>> But simplification routines aren't required or expected to restrict >>>> themselves to things that the target supports directly. It's up to the >>>> caller of the simplification routines to test whether the target supports >>>> a particular operation. E.g. elsewhere we have: >>>> >>>> /* If we have a SUBREG of a register that we are replacing and we are >>>> replacing it with a MEM, make a new MEM and try replacing the >>>> SUBREG with it. Don't do this if the MEM has a mode-dependent >>>> address >>>> or if we would be widening it. */ >>>> >>>> if (MEM_P (op) >>>> && ! mode_dependent_address_p (XEXP (op, 0), MEM_ADDR_SPACE (op)) >>>> /* Allow splitting of volatile memory references in case we don't >>>> have instruction to move the whole thing. */ >>>> && (! MEM_VOLATILE_P (op) >>>> || ! have_insn_for (SET, innermode)) >>>> && !(STRICT_ALIGNMENT && MEM_ALIGN (op) < GET_MODE_ALIGNMENT >>>> (outermode)) >>>> && known_le (outersize, innersize)) >>>> return adjust_address_nv (op, outermode, byte); >>>> >>>> The mode_dependent_address_p check admittedly muddies the waters, but the >>>> use of adjust_address_nv means that the new MEM isn't necessarily valid. >>>> >>>>> There's also a chance to fix it in gcn_addr_space_legitimize_address, so >>>>> I'm trying to figure out if that's something it should be doing. >>>>> >>>>> The testcase is trying to convert >>>>> >>>>> (mem/i:V8QI (plus:V8DI (reg:V8DI 720) >>>>> (vec_duplicate:V8DI (const_int 0)))) >>>>> >>>>> into >>>>> >>>>> (mem:QI (subreg:DI (plus:V8DI (reg:V8DI 720) >>>>> (vec_duplicate:V8DI (const_int 0))) >>>>> 0)) >>>>> >>>>> Which is because CSE is trying to split this >>>>> >>>>> (set (mem/i:V8QI (plus:V8DI (reg:V8DI 720) >>>>> (vec_duplicate:V8DI (const_int 0)))) >>>>> (const_vector:V8QI [(const_int 0 [0]) repeated x8])) >>>>> >>>>> into 8 separate operations. The comment says "These are templates and >>>>> don't actually get emitted but are used to tell CSE how to get to a >>>>> particular constant." >>>> >>>> That too sounds ok. But CSE should only use the registered MEMs in an >>>> insn if the insn passes a recog test. >>>> >>>> Do you have a backtrace for the ICE? >>> Here's what I have, slightly cleaned up for email: >>> >>> internal_error(char const*, ...) >>> .../gcc/diagnostic-global-context.cc:787 >>> fancy_abort(char const*, int, char const*) >>> .../gcc/diagnostics/context.cc:1813 >>> df_insn_delete(rtx_insn*) >>> .../gcc/df-scan.cc:948 >>> delete_insn(rtx_insn*) >>> .../gcc/cfgrtl.cc:179 >>> delete_insn_and_edges(rtx_insn*) >>> .../gcc/cfgrtl.cc:251 >>> delete_trivially_dead_insns(rtx_insn*, int) >>> .../gcc/cse.cc:7450 >>> rest_of_handle_cse_after_global_opts >>> .../gcc/cse.cc:8036 >>> execute >>> ...gcc/cse.cc:8085 >>> >>> However, I think the delete functions are fallout from change_address et >>> al not managing to legitimize the subreg address. >> >> Hmm, yeah, that's not the kind of backtrace I expected. >> >> Is there a testcase I can use that would show the problem, after applying >> the patch series? > I've attached my WIP patchset. I think your first comments were folded > into the first 3, and the "and another thing" comments are in the > "fixup" patches. > > To reproduce the issue don't apply the subreg fixup in the final patch. > You should see that lib2-divmod-di.c in libgcc fails to build.
Thanks. I've not got to the stage of being able to build libgcc yet, but looking at the series: I forgot that change_address is inherently a validating function, which means that it can emit fix-up instructions (or ICE if called after RA). Here we want to suppress that and use a non-validating form. Does the attached patch help? I still think it makes sense to use change_address* directly in this case, since the more specific adjustment functions kind-of assume a contiguous access. Thanks, Richard
>From 849d2384bc7f2fcf77f247db97a7a1da7c26502c Mon Sep 17 00:00:00 2001 From: Richard Sandiford <[email protected]> Date: Mon, 12 Oct 2026 17:42:02 +0100 Subject: [PATCH] Add change_address_nv --- gcc/emit-rtl.cc | 27 ++++++++++++++++++++++----- gcc/emit-rtl.h | 1 + gcc/simplify-rtx.cc | 2 +- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc index a88ffbbc2cb..cd0e54a2ad5 100644 --- a/gcc/emit-rtl.cc +++ b/gcc/emit-rtl.cc @@ -2347,13 +2347,12 @@ change_address_1 (rtx memref, machine_mode mode, rtx addr, int validate, return new_rtx; } -/* Like change_address_1 with VALIDATE nonzero, but we are not saying in what - way we are changing MEMREF, so we only preserve the alias set. */ +/* Version of change_address that takes a validate parameter. */ -rtx -change_address (rtx memref, machine_mode mode, rtx addr) +static rtx +change_address (rtx memref, machine_mode mode, rtx addr, int validate) { - rtx new_rtx = change_address_1 (memref, mode, addr, 1, false); + rtx new_rtx = change_address_1 (memref, mode, addr, validate, false); machine_mode mmode = GET_MODE (new_rtx); class mem_attrs *defattrs; @@ -2379,6 +2378,24 @@ change_address (rtx memref, machine_mode mode, rtx addr) return new_rtx; } +/* Like change_address_1 with VALIDATE nonzero, but we are not saying in what + way we are changing MEMREF, so we only preserve the alias set. */ + +rtx +change_address (rtx memref, machine_mode mode, rtx addr) +{ + return change_address (memref, mode, addr, true); +} + +/* Like change_address_1 with VALIDATE zero, but we are not saying in what + way we are changing MEMREF, so we only preserve the alias set. */ + +rtx +change_address_nv (rtx memref, machine_mode mode, rtx addr) +{ + return change_address (memref, mode, addr, false); +} + /* Return a memory reference like MEMREF, but with its mode changed to MODE and its address offset by OFFSET bytes. If VALIDATE is nonzero, the memory address is forced to be valid. diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h index 5fae3264cb5..c88d1962076 100644 --- a/gcc/emit-rtl.h +++ b/gcc/emit-rtl.h @@ -482,6 +482,7 @@ extern void set_decl_incoming_rtl (tree, rtx, bool); (VOIDmode means don't change the mode. NULL for ADDR means don't change the address.) */ extern rtx change_address (rtx, machine_mode, rtx); +extern rtx change_address_nv (rtx, machine_mode, rtx); /* Return a memory reference like MEMREF, but with its mode changed to MODE and its address offset by OFFSET bytes. */ diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc index 934324629d2..103ee218d4f 100644 --- a/gcc/simplify-rtx.cc +++ b/gcc/simplify-rtx.cc @@ -8863,7 +8863,7 @@ simplify_context::simplify_subreg (machine_mode outermode, rtx op, rtx new_addr = simplify_gen_subreg (addr_elt_mode, addr, addrmode, addr_byte); if (new_addr) - return change_address (op, outermode, new_addr); + return change_address_nv (op, outermode, new_addr); } } -- 2.55.0
