[gcc r17-2741] out-of-SSA: Use all partition names to find the decl to split [PR126405]
Kyrylo Tkachov via Gcc-cvs <[email protected]>
| Newsgroups | gmane.comp.gcc.cvs |
|---|---|
| Message-ID | <[email protected]> |
https://gcc.gnu.org/g:6f8fff34e8d854c3f66b2b7c9c64e973a71661cb commit r17-2741-g6f8fff34e8d854c3f66b2b7c9c64e973a71661cb Author: Kyrylo Tkachov <[email protected]> Date: Sun Jul 26 01:48:55 2026 -0700 out-of-SSA: Use all partition names to find the decl to split [PR126405] split_overlapping_partition_decls gives every partition but one of a memory-resident VAR_DECL its own artificial decl, so that distinct stack slots do not end up sharing a MEM_EXPR. It takes the variable of a partition from that partition's representative. set_rtl attaches the base variable of any name in a partition to that partition's location, so the variable a partition is given need not be the one of its representative. Here the partition holding the loop-carried versions of an oversized vector temporary has an anonymous representative: Partition 1 (_2 - 2 14 16 ) _2 has no base variable, while 14 and 16 are g18_lsm.11_14 and _16 Partition 30 (g18_lsm.11_41 - 41 ) Partition 1 is skipped because SSA_NAME_VAR of _2 is null, so partition 30 believes it is the only partition of g18_lsm.11 and keeps the user decl. Both 128 byte slots then get MEM_EXPR g18_lsm.11. The load/store pair-fusion pass groups accesses by MEM_EXPR base and MEM_OFFSET, so it treated two stores that are 144 bytes apart as adjacent and fused them: before (set (mem:V4SI [sp+240]) (reg v17)) ; g18_lsm.11+96 (set (mem:V4SI [sp+384]) (reg v27)) ; g18_lsm.11+112 after (set (mem:V2x16QI [sp+240]) (unspec [(reg v17) (reg v27)] UNSPEC_STP)) [sp+384] is never written. Work out the variable each partition will be given and key the keep/split decision on that, reusing the function set_rtl makes the same choice with, renamed to expand_leader_merge now. That function keeps the variable it is given first unless a later one is DECL_IGNORED_P, and expansion hands set_rtl the representative before the other names, so the representative's variable goes in first. A partition holding names of two variables would otherwise be keyed on the wrong one. Bootstrapped and tested on aarch64-none-linux-gnu. gcc/ChangeLog: PR middle-end/126405 * cfgexpand.cc (leader_merge): Rename to... (expand_leader_merge): ...this. Make external. * cfgexpand.h (expand_leader_merge): Declare. * tree-outof-ssa.cc: Include cfgexpand.h. (split_overlapping_partition_decls): Determine the variable of a partition from all of its names. gcc/testsuite/ChangeLog: PR middle-end/126405 * gcc.c-torture/execute/pr126405.c: New test. * gcc.c-torture/execute/pr126405-2.c: New test. * gcc.dg/pr126405-1.c: New test. * gcc.dg/pr126405-2.c: New test. * gcc.dg/pr126405-3.c: New test. Signed-off-by: Kyrylo Tkachov <[email protected]> Diff: --- gcc/cfgexpand.cc | 10 +++-- gcc/cfgexpand.h | 1 + gcc/testsuite/gcc.c-torture/execute/pr126405-2.c | 54 +++++++++++++++++++++++ gcc/testsuite/gcc.c-torture/execute/pr126405.c | 56 ++++++++++++++++++++++++ gcc/testsuite/gcc.dg/pr126405-1.c | 29 ++++++++++++ gcc/testsuite/gcc.dg/pr126405-2.c | 35 +++++++++++++++ gcc/testsuite/gcc.dg/pr126405-3.c | 26 +++++++++++ gcc/tree-outof-ssa.cc | 29 +++++++++++- 8 files changed, 234 insertions(+), 6 deletions(-) diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc index c5b4a62ace2e..8b6b5b923ee0 100644 --- a/gcc/cfgexpand.cc +++ b/gcc/cfgexpand.cc @@ -155,10 +155,11 @@ gimple_assign_rhs_to_tree (gimple *stmt) /* Choose either CUR or NEXT as the leader DECL for a partition. Prefer ignored decls, to simplify debug dumps and reduce ambiguity out of the same user variable being in multiple partitions (this is - less likely for compiler-introduced temps). */ + less likely for compiler-introduced temps). Also used by out-of-SSA + to work out which variable a partition will be given. */ -static tree -leader_merge (tree cur, tree next) +tree +expand_leader_merge (tree cur, tree next) { if (cur == NULL || cur == next) return next; @@ -251,7 +252,8 @@ set_rtl (tree t, rtx x) else gcc_unreachable (); - tree next = skip ? cur : leader_merge (cur, SSAVAR (t) ? SSAVAR (t) : t); + tree next + = skip ? cur : expand_leader_merge (cur, SSAVAR (t) ? SSAVAR (t) : t); if (cur != next) { diff --git a/gcc/cfgexpand.h b/gcc/cfgexpand.h index f3acb8862257..feda4e441732 100644 --- a/gcc/cfgexpand.h +++ b/gcc/cfgexpand.h @@ -24,6 +24,7 @@ extern tree gimple_assign_rhs_to_tree (gimple *); extern HOST_WIDE_INT estimated_stack_frame_size (struct cgraph_node *); extern void expand_remove_edge (edge); extern void set_parm_rtl (tree, rtx); +extern tree expand_leader_merge (tree, tree); #endif /* GCC_CFGEXPAND_H */ diff --git a/gcc/testsuite/gcc.c-torture/execute/pr126405-2.c b/gcc/testsuite/gcc.c-torture/execute/pr126405-2.c new file mode 100644 index 000000000000..49ce58539a27 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr126405-2.c @@ -0,0 +1,54 @@ +/* The same out-of-SSA defect as pr126405.c, with a narrower companion vector. + That changes the register pressure around the copy of the oversized vector + and so the pair the fusion pass picks, but the cause is the same: two stack + slots of one variable sharing a MEM_EXPR. Self-checking: aborts if the + result is wrong. */ + +typedef long __attribute__((vector_size (16 * sizeof (long)))) v16di; +typedef int __attribute__((vector_size (8 * sizeof (int)))) v8si; + +long g2, g12; +v16di g18; +v8si g3; +void *g27; + +/* The wrong value is read from an uninitialised stack slot, so make sure the + stack the callee reuses does not happen to be zero. */ +__attribute__((noipa)) static void +dirty_stack (void) +{ + volatile char buf[1024]; + for (unsigned i = 0; i < sizeof (buf); i++) + buf[i] = 0xa5; +} + +void +f31 (void) +{ +lbl_br1: + g18 = ~g18; + g3 = ~g3; + if (g2) + goto lbl_br1; +lbl_b5: + switch (g12) + case 4: + case 0: + goto lbl_sw8; + __builtin_abort (); +lbl_sw8: + if (g27) + goto lbl_b5; + g18 = ~g18; +} + +int +main (void) +{ + dirty_stack (); + f31 (); + for (int i = 0; i < 16; i++) + if (g18[i] != 0) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/gcc.c-torture/execute/pr126405.c b/gcc/testsuite/gcc.c-torture/execute/pr126405.c new file mode 100644 index 000000000000..84d54364e957 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr126405.c @@ -0,0 +1,56 @@ +/* AArch64 wrong code at -O2. Store motion creates several SSA versions of an + oversized vector temporary (V16DI, 128 bytes, no register mode). The + partition holding the loop-carried versions has an anonymous representative, + so out-of-SSA left it and the partition of the copy taken for the use after + the loop sharing one MEM_EXPR. The load/store pair-fusion pass then treated + two stores 144 bytes apart as adjacent, fused them, and left the tail of one + slot uninitialised. Self-checking: aborts if the result is wrong. */ + +typedef long __attribute__((vector_size (16 * sizeof (long)))) v16di; +typedef int __attribute__((vector_size (16 * sizeof (int)))) v16si; + +long g2, g12; +v16di g18; +v16si g3; +void *g27; + +/* The wrong value is read from an uninitialised stack slot, so make sure the + stack the callee reuses does not happen to be zero. */ +__attribute__((noipa)) static void +dirty_stack (void) +{ + volatile char buf[1024]; + for (unsigned i = 0; i < sizeof (buf); i++) + buf[i] = 0xa5; +} + +void +f31 (void) +{ +lbl_br1: + g18 = ~g18; + g3 = ~g3; + if (g2) + goto lbl_br1; +lbl_b5: + switch (g12) + case 4: + case 0: + goto lbl_sw8; + __builtin_abort (); +lbl_sw8: + if (g27) + goto lbl_b5; + g18 = ~g18; +} + +int +main (void) +{ + dirty_stack (); + f31 (); + for (int i = 0; i < 16; i++) + if (g18[i] != 0) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/gcc.dg/pr126405-1.c b/gcc/testsuite/gcc.dg/pr126405-1.c new file mode 100644 index 000000000000..40007b108ebc --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr126405-1.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fgimple -ffloat-store -ftree-coalesce-vars -fdump-rtl-expand" } */ + +/* The partition holding v_2 has an anonymous representative, _1, so the + variable that expansion attaches to its stack slot comes from a name other + than the representative. -ffloat-store puts both partitions of v in memory, + and they must not end up sharing a MEM_EXPR. */ + +double in1, in2, out1, out2, out3, out4; + +void __GIMPLE (ssa, startwith ("expand")) +f (void) +{ + double v; + double _1; + +__BB(2): + _1 = in1; + v_2 = _1; + v_3 = in2; + out1 = v_2; + out2 = v_3; + out3 = v_2; + out4 = v_3; + return; +} + +/* { dg-final { scan-rtl-dump {\[[0-9]+ v\+0} "expand" } } */ +/* { dg-final { scan-rtl-dump {\[[0-9]+ D\.[0-9]+\+0} "expand" } } */ diff --git a/gcc/testsuite/gcc.dg/pr126405-2.c b/gcc/testsuite/gcc.dg/pr126405-2.c new file mode 100644 index 000000000000..c320824e4e43 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr126405-2.c @@ -0,0 +1,35 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fgimple -ffloat-store -ftree-coalesce-vars -fdump-rtl-expand" } */ + +/* Three partitions of v, each with an anonymous representative. One keeps v + and the other two need their own decls, so that no two of the three stack + slots share a MEM_EXPR. */ + +double in1, in2, in3, out1, out2, out3, out4, out5, out6; + +void __GIMPLE (ssa, startwith ("expand")) +f (void) +{ + double v; + double _1; + double _3; + double _5; + +__BB(2): + _1 = in1; + v_2 = _1; + _3 = in2; + v_4 = _3; + _5 = in3; + v_6 = _5; + out1 = v_2; + out2 = v_4; + out3 = v_6; + out4 = v_2; + out5 = v_4; + out6 = v_6; + return; +} + +/* { dg-final { scan-rtl-dump {\[[0-9]+ v\+0} "expand" } } */ +/* { dg-final { scan-rtl-dump {\[[0-9]+ D\.[0-9]+\+0} "expand" } } */ diff --git a/gcc/testsuite/gcc.dg/pr126405-3.c b/gcc/testsuite/gcc.dg/pr126405-3.c new file mode 100644 index 000000000000..b6e1bdc2498f --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr126405-3.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -ffloat-store -ftree-coalesce-vars -fdump-rtl-expand" } */ + +/* Coalescing puts names of u and of v in one partition, and its representative + is a name of v rather than the lowest numbered member. The variable that + expansion attaches to that partition is therefore the one of the + representative, and keying the split on any other member of the partition + leaves it and the second partition of v sharing a MEM_EXPR. */ + +double g1, g2, g3, g4, g5; + +void +f (int n) +{ + double u = g1; + double v = u; + for (int i = 0; i < n; i++) + v = v + g3; + double t = v; + v = g2; + g4 = t; + g5 = v; +} + +/* { dg-final { scan-rtl-dump {\[[0-9]+ v\+0} "expand" } } */ +/* { dg-final { scan-rtl-dump {\[[0-9]+ D\.[0-9]+\+0} "expand" } } */ diff --git a/gcc/tree-outof-ssa.cc b/gcc/tree-outof-ssa.cc index 17d5e70e71d8..309acd4fde2d 100644 --- a/gcc/tree-outof-ssa.cc +++ b/gcc/tree-outof-ssa.cc @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa-ter.h" #include "tree-ssa-coalesce.h" #include "tree-outof-ssa.h" +#include "cfgexpand.h" #include "dojump.h" #include "internal-fn.h" #include "gimple-fold.h" @@ -1072,13 +1073,39 @@ split_overlapping_partition_decls (var_map map) auto_vec<tree> new_decl; new_decl.safe_grow_cleared (n); bool any = false; + unsigned ver; + tree name; + + /* set_rtl attaches the base variable of any name in a partition to that + partition's location, not just the one of its representative, so collect + what the names of each partition contribute. A name with no base + variable contributes nothing, since set_rtl passes a type rather than a + decl for those and leaves the MEM_EXPR it has in place. */ + auto_vec<tree> part_var; + part_var.safe_grow_cleared (n); + FOR_EACH_SSA_NAME (ver, name, cfun) + { + int p = var_to_partition (map, name); + if (p == NO_PARTITION) + continue; + tree var = SSA_NAME_VAR (name); + if (!var) + continue; + part_var[p] = expand_leader_merge (part_var[p], var); + } for (unsigned i = 0; i < n; i++) { tree repr = partition_to_var (map, i); if (!repr) continue; + /* Expansion hands set_rtl the representative before the other names, + and expand_leader_merge keeps the variable it is given first unless a + later one is DECL_IGNORED_P, so merging the two gives the variable + this partition ends up with. */ tree var = SSA_NAME_VAR (repr); + if (part_var[i]) + var = expand_leader_merge (var, part_var[i]); if (!var || !VAR_P (var)) continue; /* Only partitions that will live in memory can end up with a @@ -1123,8 +1150,6 @@ split_overlapping_partition_decls (var_map map) if (!any) return; - unsigned ver; - tree name; FOR_EACH_SSA_NAME (ver, name, cfun) { if (SSA_NAME_IS_DEFAULT_DEF (name))