On Tue, 7 Jul 2026, Kyrylo Tkachov wrote: > Hi Richard, > > > On 7 Jul 2026, at 11:36, Richard Biener <[email protected]> wrote: > > > > On Tue, 30 Jun 2026, [email protected] wrote: > > > >> From: Kyrylo Tkachov <[email protected]> > >> > >> A gimple register with no register mode (e.g. an oversized vector such as > >> V16DI > >> on AArch64, which is BLKmode) may have several simultaneously live SSA > >> versions > >> that out-of-SSA cannot coalesce, so they end up in separate partitions > >> sharing > >> one base VAR_DECL. When such partitions are spilled they are given > >> distinct > >> stack slots, but set_mem_attributes gives every slot that one decl as its > >> MEM_EXPR at offset 0, so the slots all appear to be the same object. > >> > >> This misleads MEM_EXPR-based disambiguation: the load/store pair-fusion > >> pass > >> keys candidate accesses on (decl, offset) and fuses stores across the > >> slots, > >> corrupting one and producing wrong code at -O2/-O3 (PR123625, and PR121957 > >> where late-combine first reshapes the addressing into this form). > >> > >> This is an alternative fix to the expand one posted at: > >> https://gcc.gnu.org/pipermail/gcc-patches/2026-June/721973.html > >> > >> Fix it in remove_ssa_form after coalescing, give every partition > >> after the first of such a BLKmode decl its own artificial decl, so the > >> slots > >> are distinguished. The new decl carries a DECL_DEBUG_EXPR back to the user > >> variable so debug info still attributes the storage to it (as > >> create_access_replacement does in tree-sra.cc). PARM_DECLs and > >> RESULT_DECLs > >> are left alone. They require a single partition holding the canonical > >> RTL. The > >> split runs after find_replaceable_exprs so it does not perturb TER. > >> > >> Bootstrapped and tested on aarch64-none-linux-gnu and x86_64-linux. > >> Is this preferable to the expand fix? > > > > While I like this more I think the actual issue is in set_rtl.cc which > > does strange things, but notably > > > > tree next = skip ? cur : leader_merge (cur, SSAVAR (t) ? SSAVAR (t) > > : t); > > > > if (cur != next) > > { > > if (MEM_P (x)) > > set_mem_attributes (x, > > next && TREE_CODE (next) == SSA_NAME > > ? TREE_TYPE (next) > > : next, true); > > > > that is, it is responsible for turning in SSA_NAME_VAR to > > set_mem_attributes. It shouldn't do that if there are multiple > > partitions for it. In fact I'm not sure why we ever do all this > > singing and dancing. > > This area isn’t exactly my strong suit but judging by the archaeology… > This is for >=01 debug info from PR64164 copyrename removal. > At -O0 it goes through the DECL_RTL recording. > > > > > IMO a simple fix(?) would be to drop all this and do > > > > tree next = skip ? cur : leader_merge (cur, t); > > > > esp. when MEM_P (x). > > That would fix the sharing, but it degrades every named memory partition to > type-only attrs, losing MEM_EXPR, MEM_OFFSET and MEM_NOTRAP_P also in the > case where a variable has exactly one partition. > > > > There's similarly dubious code after this, > > but we already note the issue! > > > > if (TREE_CODE (t) == SSA_NAME) > > { > > int part = var_to_partition (SA.map, t); > > if (part != NO_PARTITION) > > { > > if (SA.partition_to_pseudo[part]) > > gcc_assert (SA.partition_to_pseudo[part] == x); > > else if (x != pc_rtx) > > SA.partition_to_pseudo[part] = x; > > } > > /* For the benefit of debug information at -O0 (where > > vartracking doesn't run) record the place also in the base > > DECL. For PARMs and RESULTs, do so only when setting the > > default def. */ > > if (x && x != pc_rtx && SSA_NAME_VAR (t) > > && (VAR_P (SSA_NAME_VAR (t)) > > || SSA_NAME_IS_DEFAULT_DEF (t))) > > { > > tree var = SSA_NAME_VAR (t); > > /* If we don't yet have something recorded, just record it now. > > */ > > if (!DECL_RTL_SET_P (var)) > > SET_DECL_RTL (var, x); > > /* If we have it set already to "multiple places" don't > > change this. */ > > else if (DECL_RTL (var) == pc_rtx) > > ; > > /* If we have something recorded and it's not the same place > > as we want to record now, we have multiple partitions for the > > same base variable, with different places. We can't just > > randomly chose one, hence we have to say that we don't know. > > This only happens with optimization, and there var-tracking > > will figure out the right thing. */ > > else if (DECL_RTL (var) != x) > > SET_DECL_RTL (var, pc_rtx); > > ^^^^^^^^^^^^^^^^^^ > > > > what seems to be lacking is direct info of whether a particular > > DECL has multiple SSA partitions. > > > > Upthread we get here via expand_stack_vars -> expand_one_stack_var_at > > where there is the stack_vars[] meta where I can see for one of the > > testcases: > > > > (gdb) p stack_vars[0] > > $19 = {decl = <ssa_name 0x7ffff672dd10 3>, size = {coeffs = {128, 0}}, > > alignb = 16, representative = 0, next = 4294967295, conflicts = 0x0} > > (gdb) p stack_vars[1] > > $20 = {decl = <ssa_name 0x7ffff6779318 10>, size = {coeffs = {128, 0}}, > > alignb = 16, representative = 1, next = 4294967295, conflicts = 0x0} > > (gdb) p debug_generic_expr (0x7ffff672dd10) > > v_3 > > $21 = void > > (gdb) p debug_generic_expr (0x7ffff6779318) > > v_10 > > > > so there's two stack vars for the same SSA_NAME_VAR, exactly the > > situation above. It should be possible to discover this case > > beforehand and mark the affected stack_vars[] entries as to > > skip using SSA_NAME_VAR here, possibly pass down the decl/type > > to be used to set_rtl directly. > > > > Your out-of-SSA patch might go for preserving as much info > > as possible, so the last quoted piece above could be made > > an assert that this doesn't happen after it? > > that would fire on valid code: with -ftree-coalesce-vars a variable's names > can be coalesced into memory partitions of other variables, and then DECL_RTL > (b) legitimately sees two different MEMs
True, we don't by default force partition different SSA_NAME_VAR into different partitions but happily coalesce. Mixing in with multiple partitions per SSA_NAME_VAR means there's no clear 1:1 relationship to SSA_NAME_VAR and so we shouldn't really attempt to create one by setting DECL_RTL of SSA_NAME_VAR (instead keep it NULL?). We have debug stmts/insns that should provide the association between user variables and SSA partitions. > > > The only thing > > is that keying on BLKmode might not be fully matching what > > we do later. > > Yeah, I can fix that in the patch. > > > In principle the decl to be used could be > > created in expand_stack_vars as well? > > > > It was Alex who at least last touched all this and most of > > set_rtl is attributed to ... > > I can send a v2 with the above fix and more test cases for where the assert > above you suggested would fire once I finish more testing while giving folks > a chance to chime in. Yes please. Thanks, Richard. > Thanks, > Kyrill > > > > > Thanks, > > Richard. > > > > > > > >> Thanks, > >> Kyrill > >> > >> gcc/ChangeLog: > >> > >> PR target/123625 > >> PR target/121957 > >> * tree-outof-ssa.cc (split_overlapping_partition_decls): New function. > >> (remove_ssa_form): Call it. > >> > >> 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.c-torture/execute/pr121957.c | 27 +++++++ > >> .../gcc.c-torture/execute/pr123625.c | 48 ++++++++++++ > >> gcc/tree-outof-ssa.cc | 75 +++++++++++++++++++ > >> 3 files changed, 150 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/testsuite/gcc.c-torture/execute/pr121957.c > >> b/gcc/testsuite/gcc.c-torture/execute/pr121957.c > >> new file mode 100644 > >> index 00000000000..98969566c29 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.c-torture/execute/pr121957.c > >> @@ -0,0 +1,27 @@ > >> +/* 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..2b3b29b5e14 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.c-torture/execute/pr123625.c > >> @@ -0,0 +1,48 @@ > >> +/* 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; > >> +} > >> diff --git a/gcc/tree-outof-ssa.cc b/gcc/tree-outof-ssa.cc > >> index f4bc4878bd6..78372d314c0 100644 > >> --- a/gcc/tree-outof-ssa.cc > >> +++ b/gcc/tree-outof-ssa.cc > >> @@ -25,6 +25,7 @@ along with GCC; see the file COPYING3. If not see > >> #include "rtl.h" > >> #include "tree.h" > >> #include "gimple.h" > >> +#include "gimple-expr.h" > >> #include "cfghooks.h" > >> #include "ssa.h" > >> #include "tree-ssa.h" > >> @@ -1049,6 +1050,75 @@ expand_phi_nodes (struct ssaexpand *sa) > >> } > >> > >> > >> +/* Out-of-SSA can leave several partitions sharing one base VAR_DECL, when > >> + that variable's SSA versions are simultaneously live and so cannot all > >> be > >> + coalesced. When such a partition is spilled (it has no register mode, > >> + e.g. an oversized vector that is BLKmode), set_mem_attributes would > >> give > >> + every one of those slots that single decl as its MEM_EXPR at offset 0, > >> so > >> + the distinct slots appear to be the same object and mislead > >> MEM_EXPR-based > >> + disambiguation, and the load/store pair-fusion pass then fuses across > >> the > >> + slots and corrupts one. Give every partition after the first of such a > >> + decl its own artificial decl so the slots are distinguished at the > >> source. > >> + The new decl carries a DECL_DEBUG_EXPR back to the user variable so > >> debug > >> + info still attributes the storage to it (cf. create_access_replacement > >> in > >> + tree-sra.cc). PARM_DECLs and RESULT_DECLs are left alone, as they > >> require > >> + a single partition holding the canonical RTL. */ > >> + > >> +static void > >> +split_overlapping_partition_decls (var_map map) > >> +{ > >> + unsigned n = num_var_partitions (map); > >> + hash_set<tree> seen; > >> + auto_vec<tree> new_decl; > >> + new_decl.safe_grow_cleared (n); > >> + bool any = false; > >> + > >> + for (unsigned i = 0; i < n; i++) > >> + { > >> + tree repr = partition_to_var (map, i); > >> + if (!repr) > >> + continue; > >> + tree var = SSA_NAME_VAR (repr); > >> + if (!var || !VAR_P (var)) > >> + continue; > >> + /* Only memory partitions (no register mode, e.g. an oversized > >> vector) > >> + can end up with a misleading shared MEM_EXPR. */ > >> + if (TYPE_MODE (TREE_TYPE (var)) != BLKmode) > >> + continue; > >> + /* The first partition of VAR keeps the user decl, the rest are > >> split. */ > >> + if (!seen.add (var)) > >> + continue; > >> + > >> + tree nvar = create_tmp_var_raw (TREE_TYPE (var)); > >> + DECL_CONTEXT (nvar) = DECL_CONTEXT (var); > >> + DECL_SOURCE_LOCATION (nvar) = DECL_SOURCE_LOCATION (var); > >> + SET_DECL_ALIGN (nvar, DECL_ALIGN (var)); > >> + if (!DECL_ARTIFICIAL (var) && DECL_NAME (var)) > >> + { > >> + SET_DECL_DEBUG_EXPR (nvar, var); > >> + DECL_HAS_DEBUG_EXPR_P (nvar) = 1; > >> + } > >> + copy_warning (nvar, var); > >> + add_local_decl (cfun, nvar); > >> + new_decl[i] = nvar; > >> + any = true; > >> + } > >> + > >> + if (!any) > >> + return; > >> + > >> + unsigned ver; > >> + tree name; > >> + FOR_EACH_SSA_NAME (ver, name, cfun) > >> + { > >> + if (SSA_NAME_IS_DEFAULT_DEF (name)) > >> + continue; > >> + int p = var_to_partition (map, name); > >> + if (p != NO_PARTITION && new_decl[p]) > >> + SET_SSA_NAME_VAR_OR_IDENTIFIER (name, new_decl[p]); > >> + } > >> +} > >> + > >> /* Remove the ssa-names in the current function and translate them into > >> normal > >> compiler variables. PERFORM_TER is true if Temporary Expression > >> Replacement > >> should also be used. */ > >> @@ -1080,6 +1150,11 @@ remove_ssa_form (bool perform_ter, struct ssaexpand > >> *sa) > >> dump_replaceable_exprs (dump_file, values); > >> } > >> > >> + /* Distinct partitions of one decl must not share a MEM_EXPR once they > >> are > >> + spilled to separate stack slots. Done after TER so reassigning > >> + SSA_NAME_VAR does not perturb find_replaceable_exprs. */ > >> + split_overlapping_partition_decls (map); > >> + > >> rewrite_trees (map); > >> > >> sa->map = map; > >> > > > > -- > > Richard Biener <[email protected]> > > SUSE Software Solutions Germany GmbH, > > Frankenstrasse 146, 90461 Nuernberg, Germany; > > GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg) > > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
