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. > >> + 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)
