> On 29 Jun 2026, at 09:41, Kyrylo Tkachov <[email protected]> wrote: > > Hi Richard, > >> On 18 Jun 2026, at 08:24, Richard Biener <[email protected]> wrote: >> >> On Wed, Jun 17, 2026 at 4:44 PM <[email protected]> wrote: >>> >>> From: Kyrylo Tkachov <[email protected]> >>> >>> A gimple register that cannot be kept in a hard register (for example an >>> oversized vector such as V16DI on AArch64, which has no register mode) may >>> have >>> several simultaneously live SSA partitions. When such partitions are >>> spilled, >>> each is given its own distinct stack slot, yet set_rtl, via >>> set_mem_attributes, >>> attaches the base VAR_DECL of the SSA name as the MEM_EXPR of every slot, at >>> offset 0 and with the size of the whole variable. The distinct slots >>> therefore >>> all claim to be the same object. >> >> Ugh. So do those SSA variables have BLKmode? IMO the error then is really >> out-of-SSA which should have assigned distinct decls to overlapping >> partitions. > > Yes, they are BLKmode. I did look into fixing it in out-of-ssa as well by > splitting overlapping partition decls in remove_ssa_form. It seems to > bootstrap and test okay so I can propose it, but I wonder if this patch is > more appropriate for backports? > >> >> That said, it's bad that we have to drop MEM_EXPRs but until we improve >> the above it looks like an OK fix, but ... >> >>> >>> This misleads MEM_EXPR based disambiguation. In particular the load/store >>> pair >>> fusion pass groups candidate accesses by (decl, offset) >>> (track_via_mem_expr), >>> so a store to one slot at offset K and a store to a different slot at offset >>> K +/- access_size are treated as adjacent and fused, redirecting an access >>> to >>> the wrong slot and producing wrong code. This shows up as a miscompile at >>> -O2/-O3 on AArch64 (PR121957, where late-combine first reshapes the >>> addressing >>> into the affected form, and PR123625). Both the early (pre-RA) and late >>> (post-RA) copies of the pass are affected. >>> >>> A stack slot assigned to an SSA partition represents a single coalesced SSA >>> live-range, not the storage of the user variable, so attaching the variable >>> as >>> its MEM_EXPR is not meaningful, and is positively wrong once the variable >>> occupies more than one slot. Fix this in pass_expand::execute: after every >>> SSA >>> partition has been given its final rtl, drop the MEM_EXPR of the stack >>> slots of >>> any decl that lives in more than one place. Such a decl is exactly the one >>> whose DECL_RTL set_rtl marked with pc_rtx, so we only touch the slots that >>> actually share a misleading MEM_EXPR; single-slot objects are left >>> unchanged. >>> The distinct RTL base addresses of the slots continue to disambiguate them, >>> and >>> in-slot accesses, which share a base register, are still paired normally, so >>> ldp/stp formation for ordinary code is unaffected. >>> >>> The clear is done after the partition-adjustment loop rather than in set_rtl >>> itself: set_rtl is reached more than once per slot (notably from >>> adjust_one_expanded_partition_var), and clearing the MEM_EXPR mid-way would >>> make >>> a later set_rtl re-run set_mem_attributes after DECL_RTL has been set, >>> tripping >>> the assertion in set_mem_attributes_minus_bitpos. >>> >>> This is a broader fix that makes >>> https://gcc.gnu.org/pipermail/gcc-patches/2026-June/720419.html >>> redundant, so I'm proposing we drop that patch and go with this. >>> >>> Bootstrapped and tested on aarch64-none-linux-gnu and x86_64-linux. >>> >>> Ok for trunk? >>> Thanks, >>> Kyrill >>> >>> Signed-off-by: Kyrylo Tkachov <[email protected]> >>> >>> gcc/ChangeLog: >>> >>> PR target/123625 >>> PR target/121957 >>> * cfgexpand.cc (pass_expand::execute): After expanding SSA >>> partitions, drop the MEM_EXPR of the stack slots of any decl that >>> occupies more than one partition. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR target/123625 >>> PR target/121957 >>> * gcc.c-torture/execute/pr123625.c: New test. >>> * gcc.c-torture/execute/pr121957.c: New test. >>> >>> Signed-off-by: Kyrylo Tkachov <[email protected]> >>> --- >>> gcc/cfgexpand.cc | 26 ++++++++++ >>> .../gcc.c-torture/execute/pr121957.c | 27 +++++++++++ >>> .../gcc.c-torture/execute/pr123625.c | 48 +++++++++++++++++++ >>> 3 files changed, 101 insertions(+) >>> create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr121957.c >>> create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr123625.c >>> >>> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc >>> index 05efcea13ba..76c2dbe7a29 100644 >>> --- a/gcc/cfgexpand.cc >>> +++ b/gcc/cfgexpand.cc >>> @@ -7167,6 +7167,32 @@ pass_expand::execute (function *fun) >>> adjust_one_expanded_partition_var (name); >>> } >>> >>> + /* A gimple register that cannot live in a hard register (e.g. an >>> oversized >>> + vector with no register mode) may be split into several simultaneously >>> + live SSA partitions, each given its own distinct stack slot. >>> set_rtl, via >>> + set_mem_attributes, attaches the base decl as the MEM_EXPR of every >>> such >>> + slot at offset 0, so the distinct slots all claim to be the same >>> object. >>> + That misleads MEM_EXPR-based disambiguation: the load/store >>> pair-fusion >>> + pass keys candidate accesses on (decl, offset) and would treat >>> accesses to >>> + different slots as adjacent, fusing them and corrupting one slot (see >>> + PR123625, PR121957). A decl that lives in more than one place is >>> exactly >>> + the one whose DECL_RTL set_rtl marked with pc_rtx; drop the bogus >>> MEM_EXPR >>> + on its stack slots. The slots' distinct RTL base addresses continue >>> to >>> + disambiguate them correctly. This must run after the loop above, >>> once all >>> + set_rtl calls (which would otherwise re-attach the MEM_EXPR) are >>> done. */ >>> + FOR_EACH_SSA_NAME (i, name, cfun) >>> + { >>> + if (!SSA_NAME_DEF_STMT (name)) >> >> this should not happen ... > > There are comments elsewhere in that function that > update_alias_info_with_stack_vars can create SSA names that are not in SA.map > and on which var_to_partition must not be called. But I can change the loop > to go over num_var_partitions and use partition_to_var.
I’ve adjusted this patch and resent it at: https://gcc.gnu.org/pipermail/gcc-patches/2026-June/721973.html Thanks, Kyrill > >> >>> + continue; >>> + int part = var_to_partition (SA.map, name); >>> + if (part == NO_PARTITION) >>> + continue; >>> + tree var = SSA_NAME_VAR (name); >>> + rtx x = SA.partition_to_pseudo[part]; >>> + if (var && DECL_RTL_IF_SET (var) == pc_rtx && x && MEM_P (x)) >>> + set_mem_expr (x, NULL_TREE); >> >> how do we expand anonymous SSA vars (!var) here? You could try with a >> GIMPLE testcase, dropping the decl. > > They never receive a decl MEM_EXPR to begin with. For an SSA name without > SSA_NAME_VAR, set_rtl passes its type rather than a decl to > set_mem_attributes, and set_mem_attributes records no MEM_EXPR for a bare > type. > > >> >> Can we avoid setting DECL_RTL in cfgexpand.cc:set_rtl? It seems all >> information is already there? > > The misleading MEM_EXPR comes from the set_mem_attributes call in set_rtl, > not from the DECL_RTL recording. > Thanks, > Kyrill > >> >>> + } >>> + >>> /* Clean up RTL of variables that straddle across multiple >>> partitions, and check that the rtl of any PARM_DECLs that are not >>> cleaned up is that of their default defs. */ >>> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr121957.c >>> b/gcc/testsuite/gcc.c-torture/execute/pr121957.c >>> new file mode 100644 >>> index 00000000000..cfe5fa468aa >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.c-torture/execute/pr121957.c >>> @@ -0,0 +1,27 @@ >>> +/* PR target/121957: AArch64 wrong code at -O2/-O3. An oversized vector >>> + (V16DI, 128 bytes, with no register mode) is expanded into two distinct >>> + stack slots that share the same MEM_EXPR. The load/store pair-fusion >>> pass >>> + then discovers "adjacent" store pairs across the two slots via the >>> shared >>> + MEM_EXPR and fuses them, redirecting a store to the wrong slot and >>> leaving >>> + part of a slot uninitialised. Self-checking: aborts if the result is >>> + wrong. */ >>> + >>> +typedef long __attribute__((vector_size (16 * sizeof (long)))) v16di; >>> + >>> +int >>> +main (void) >>> +{ >>> + v16di v = {}; >>> + asm goto ("" : : : : L1); >>> +L2: >>> + asm goto ("" : : : : L1); >>> +L0: >>> + asm goto ("" : : : : L2); >>> + v = (v16di){ -1 }; >>> + asm goto ("" : : : : L0); >>> +L1: >>> + asm goto ("" : : : : L0); >>> + if (v[3]) >>> + __builtin_abort (); >>> + return 0; >>> +} >>> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr123625.c >>> b/gcc/testsuite/gcc.c-torture/execute/pr123625.c >>> new file mode 100644 >>> index 00000000000..0dae287d227 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.c-torture/execute/pr123625.c >>> @@ -0,0 +1,48 @@ >>> +/* PR target/123625: AArch64 wrong code at -O3. An oversized vector >>> + (V16DI, 128 bytes, no register mode) is expanded into two distinct >>> + stack slots that shared the same MEM_EXPR; the load/store pair fusion >>> + pass then fused stores belonging to different slots, leaving part of a >>> + slot uninitialized. Self-checking: the checksum is target-independent. >>> */ >>> + >>> +#include <stdint.h> >>> + >>> +#define BS_VEC(type, num) type __attribute__((vector_size(num * >>> sizeof(type)))) >>> +uint64_t BS_CHECKSUM, g_284; >>> +struct U0 >>> +{ >>> + int16_t f0; >>> + int64_t f2; >>> +} g_205; >>> +int16_t g_8, func_2_BS_COND_1; >>> +uint8_t g_9[2][1]; >>> +volatile struct U0 g_121[1]; >>> +int64_t *g_565 = &g_284; >>> + >>> +int >>> +main (void) >>> +{ >>> + BS_VEC (int64_t, 16) BS_VAR_3 = { 1, 8096386231136, 9039249955151 }; >>> + uint64_t LOCAL_CHECKSUM = 0; >>> + switch (func_2_BS_COND_1) >>> + { >>> + case 2: goto BS_LABEL_0; >>> + case 4: goto BS_LABEL_0; >>> + } >>> + for (g_8 = 0; g_8 <= 0; g_8 += 1) >>> + for (g_205.f2 = 0; g_205.f2 <= 1; g_205.f2 += 1) >>> + { >>> + if (g_9[g_205.f2][0]) >>> + BS_LABEL_0: >>> + for (;;) >>> + ; >>> + for (uint32_t BS_TEMP_371 = 0; BS_TEMP_371 < 16; BS_TEMP_371++) >>> + LOCAL_CHECKSUM ^= BS_VAR_3[BS_TEMP_371] + 9 >>> + + (LOCAL_CHECKSUM << 6) + LOCAL_CHECKSUM >>> + >> (*g_565 |= g_121[0].f0); >>> + BS_VAR_3 = (BS_VEC (int64_t, 16)){}; >>> + } >>> + BS_CHECKSUM = LOCAL_CHECKSUM; >>> + if (BS_CHECKSUM != 0x7237237237237237ULL) >>> + __builtin_abort (); >>> + return 0; >>> +} >>> -- >>> 2.50.1 (Apple Git-155)
