| Message ID | pp775p4p-4540-8954-34op-52702n156330@fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | tree-optimization: do not lose reverse storage order when translating a ref | expand |
> Eric, is the SSO use sound for the testcase (which was LLM generated)?
IIUC it writes into a big-endian structure using a little-endian character, so
the (implicit) bit ordering is changed; I guess that a memcpy would also work?
On Thu, 3 Sep 2026, Eric Botcazou wrote: > > Eric, is the SSO use sound for the testcase (which was LLM generated)? > > IIUC it writes into a big-endian structure using a little-endian character, so > the (implicit) bit ordering is changed; I guess that a memcpy would also work? To get the data flow yes, but of course the FRE issue is that the SSO attribution is lost and we generate wrong code, so actual SSO attribution is needed for that. I was mostly questioning SSO on bitfields given bitfield layout might be special. I suppose avoiding padding in S0 might make this more portable (so signed/unsigned char for f7/f6). The testcase fails with that as well before the fix. Richard.
> To get the data flow yes, but of course the FRE issue is that > the SSO attribution is lost and we generate wrong code, so actual > SSO attribution is needed for that. Sure, but we do not support type punning when it toggles the storage order and this testcase does that (admittedly in a more subtle way than most cases, as you need to realize that the bit order is implicitly changed for bitfields). What needs to be prevented is the optimization of: g.f3 = { 1, 4 }; into *(char *) &g.f3 = ... because the SSO attribution is indeed lost. > I was mostly questioning SSO on bitfields given bitfield layout might > be special. I suppose avoiding padding in S0 might make this more > portable (so signed/unsigned char for f7/f6). The testcase fails > with that as well before the fix. SSO on bitfields was the motivating case in Ada though.
On Thu, 3 Sep 2026, Eric Botcazou wrote: > > To get the data flow yes, but of course the FRE issue is that > > the SSO attribution is lost and we generate wrong code, so actual > > SSO attribution is needed for that. > > Sure, but we do not support type punning when it toggles the storage order and > this testcase does that (admittedly in a more subtle way than most cases, as > you need to realize that the bit order is implicitly changed for bitfields). Hmm, does it? You mean /* Reversed bit order, so this gives f7 == 1 and f6 == 4. */ *(char *) &g.f3 = 0x24; ? But this is just memset (&g.f3, 1, 0x23); as it writes a single byte via a character type there is no storage order involved? The q = (struct S0 *) ((char *) &l + __builtin_offsetof (struct S1, f3)); is just a way to get an offsetted MEM_EXPR I guess, it's also not punning. > What needs to be prevented is the optimization of: > > g.f3 = { 1, 4 }; > > into > > *(char *) &g.f3 = ... > > because the SSO attribution is indeed lost. I think what happened is that return q->f7; gets re-written through l = g; and that then picks up the *(char *) &g.f3 store, eliding the SSO of the q->f7 access. > > I was mostly questioning SSO on bitfields given bitfield layout might > > be special. I suppose avoiding padding in S0 might make this more > > portable (so signed/unsigned char for f7/f6). The testcase fails > > with that as well before the fix. > > SSO on bitfields was the motivating case in Ada though. I see. I have simplified S0 by using 'char' as type for the bitfields, avoiding any bit padding. Do you think this is a non-bug that is fixed? If not I plan to push it. Thanks, Richard.
diff --git a/gcc/testsuite/gcc.dg/torture/sso-fre-1.c b/gcc/testsuite/gcc.dg/torture/sso-fre-1.c new file mode 100644 index 00000000000..2862517f838 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/sso-fre-1.c @@ -0,0 +1,43 @@ +/* { dg-do run } */ +/* { dg-require-effective-target le } */ + +/* Value numbering used to translate a reference through an aggregate copy + by folding the components of the reference into a constant offset into + the right-hand side of the copy. A reversed storage order is a property + of the component and not of its position, so it was lost in the process + and the load of F7 below was resolved to the least significant bits of + the byte instead of to its most significant bits. */ + +struct __attribute__((scalar_storage_order("big-endian"))) S0 +{ + signed f7 : 3; + unsigned f6 : 5; +}; + +struct S1 { int a; struct S0 f3; char pad; short s; }; + +struct S1 g; +struct S1 *escape; + +__attribute__((noipa)) int +foo (void) +{ + struct S1 l; + struct S0 *q; + + /* Reversed bit order, so this gives f7 == 1 and f6 == 4. */ + *(char *) &g.f3 = 0x24; + l = g; + /* Keep L addressable so that it is not scalarized away. */ + escape = &l; + q = (struct S0 *) ((char *) &l + __builtin_offsetof (struct S1, f3)); + return q->f7; +} + +int +main (void) +{ + if (foo () != 1) + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-ssa-sccvn.cc b/gcc/tree-ssa-sccvn.cc index dbb19e4a498..95b5642ba36 100644 --- a/gcc/tree-ssa-sccvn.cc +++ b/gcc/tree-ssa-sccvn.cc @@ -1727,7 +1727,7 @@ contains_storage_order_barrier_p (vec<vn_reference_op_s> ops) /* Return true if OPS represent an access with reverse storage order. */ static bool -reverse_storage_order_for_component_p (vec<vn_reference_op_s> ops) +reverse_storage_order_for_component_p (const vec<vn_reference_op_s> &ops) { unsigned i = 0; if (ops[i].opcode == REALPART_EXPR || ops[i].opcode == IMAGPART_EXPR) @@ -3779,6 +3779,17 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, tree rhs1 = gimple_assign_rhs1 (def_stmt); copy_reference_ops_from_ref (rhs1, &rhs); + /* When none of the original operands survives the storage order of + the translated reference is the one of the RHS of the copy. The + operands we folded into a constant offset above may well have + specified a reverse storage order, which is a property of the + component and not of its position, so it is not recoverable from + that offset. Punt unless both accesses are in natural order. */ + if (i < 0 + && (reverse_storage_order_for_component_p (vr->operands) + || reverse_storage_order_for_component_p (rhs))) + return (void *)-1; + /* Apply an extra offset to the inner MEM_REF of the RHS. */ bool force_no_tbaa = false; if (maybe_ne (extra_off, 0))