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