[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Eric Botcazou changed: What|Removed |Added Resolution|--- |FIXED Target Milestone|--- |11.4 Status|REOPENED|RESOLVED --- Comment #20 from Eric Botcazou --- Fixed on mainline, 12 and 11 branches.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #19 from CVS Commits --- The releases/gcc-11 branch has been updated by Eric Botcazou : https://gcc.gnu.org/g:01e80c4c630a5a6286a521b047d0ef80631c892c commit r11-10463-g01e80c4c630a5a6286a521b047d0ef80631c892c Author: Eric Botcazou Date: Wed Jan 11 15:58:47 2023 +0100 Fix problematic interaction between bitfields, unions, SSO and SRA The handling of bitfields by the SRA pass is peculiar and this must be taken into account to support the scalar_storage_order attribute. gcc/ PR tree-optimization/108199 * tree-sra.c (sra_modify_expr): Deal with reverse storage order for bit-field references. gcc/testsuite/ * gcc.dg/sso-17.c: New test.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #18 from CVS Commits --- The releases/gcc-12 branch has been updated by Eric Botcazou : https://gcc.gnu.org/g:eec3a65ed638a1c58fa08ddf508d2d60b64d311d commit r12-9041-geec3a65ed638a1c58fa08ddf508d2d60b64d311d Author: Eric Botcazou Date: Wed Jan 11 15:58:47 2023 +0100 Fix problematic interaction between bitfields, unions, SSO and SRA The handling of bitfields by the SRA pass is peculiar and this must be taken into account to support the scalar_storage_order attribute. gcc/ PR tree-optimization/108199 * tree-sra.cc (sra_modify_expr): Deal with reverse storage order for bit-field references. gcc/testsuite/ * gcc.dg/sso-17.c: New test.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Eric Botcazou changed: What|Removed |Added Resolution|FIXED |--- Status|RESOLVED|REOPENED --- Comment #17 from Eric Botcazou --- Yes, that's indeed the plan.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Andreas Krebbel changed: What|Removed |Added Version|13.0|12.2.1 --- Comment #16 from Andreas Krebbel --- The testcase fails on GCC 12.2.1 as well. Should we apply it there as well after giving it some time in mainline?
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Andreas Krebbel changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #15 from Andreas Krebbel --- Your patch fixes the problem for me. Thanks for the quick fix!
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #14 from CVS Commits --- The master branch has been updated by Eric Botcazou : https://gcc.gnu.org/g:3e1cba12a8d71e70235a9a9b8f1a237a561db3e7 commit r13-5109-g3e1cba12a8d71e70235a9a9b8f1a237a561db3e7 Author: Eric Botcazou Date: Wed Jan 11 15:58:47 2023 +0100 Fix problematic interaction between bitfields, unions, SSO and SRA The handling of bitfields by the SRA pass is peculiar and this must be taken into account to support the scalar_storage_order attribute. gcc/ PR tree-optimization/108199 * tree-sra.cc (sra_modify_expr): Deal with reverse storage order for bit-field references. gcc/testsuite/ * gcc.dg/sso-17.c: New test.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Eric Botcazou changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |ebotcazou at gcc dot gnu.org Status|NEW |ASSIGNED --- Comment #13 from Eric Botcazou --- Fixing.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #12 from Eric Botcazou --- > How are the bits numbered in there, IOW is bit 0 always the LSB or not? Answering to myself: no, they are numbered in memory order, which is problematic because, in the implementation model, stand-alone scalars do not have storage order and are only considered as values, so this breaks the model. I think that Andreas' latest patch does not work in the opposite case, namely a little-endian structure on a big-endian platform: dst$val_9 = _1; _2 = BIT_FIELD_REF ; In this case BIT_FIELD_REF would refer to the MSB, but the assignment dst$val_9 = _1 assigns the interesting bit to the LSB as always for stand-alone scalar, so we would need to preserve the swapping.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #11 from Eric Botcazou --- > Here is a testcase for the trunk on x86_64-linux-gnu: Thanks. The problem is indeed the BIT_FIELD_REF of a scalar, which is an undocumented extension of GENERIC: /* Reference to a group of bits within an object. Similar to COMPONENT_REF except the position is given explicitly rather than via a FIELD_DECL. Operand 0 is the structure or union expression; operand 1 is a tree giving the constant number of bits being referenced; operand 2 is a tree giving the constant position of the first referenced bit. The result type width has to match the number of bits referenced. If the result type is integral, its signedness specifies how it is extended to its mode width. */ DEFTREECODE (BIT_FIELD_REF, "bit_field_ref", tcc_reference, 3) How are the bits numbered in there, IOW is bit 0 always the LSB or not?
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199
Andrew Pinski changed:
What|Removed |Added
Status|WAITING |NEW
--- Comment #10 from Andrew Pinski ---
Here is a testcase for the trunk on x86_64-linux-gnu:
```
#define BITFIELD_ENDIAN "big-endian"
#define SRC_ENDIAN "big-endian"
#define DST_ENDIAN "big-endian"
typedef unsigned long long u64;
union DST {
unsigned long val;
struct {
u64 x : 1;
u64 y : 1;
u64 r: 62;
} __attribute__((scalar_storage_order("big-endian")));
} __attribute__((scalar_storage_order("big-endian")));
struct SRC {
u64 a;
} __attribute__((scalar_storage_order("big-endian")));
[[gnu::noipa]]
void foo (){__builtin_abort();}
[[gnu::noinline]]
int bar(struct SRC *src)
{
union DST dst;
dst.val = src->a;
if (dst.y) {
foo();
}
return 0;
}
int main(void)
{
struct SRC t = {-1ull & (~(0x01ull<<62))};
bar();
return 0;
}
```
It does not cause an abort at -O0 but does at -O2.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Eric Botcazou changed: What|Removed |Added Status|NEW |WAITING --- Comment #9 from Eric Botcazou --- Please mention the exact compiler version and the compilation switches.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Richard Biener changed: What|Removed |Added CC||ebotcazou at gcc dot gnu.org --- Comment #8 from Richard Biener --- Eric fixed multiple storage-order issues in SRA so probably can comment here.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199
--- Comment #7 from Andreas Krebbel ---
(In reply to Andrew Pinski from comment #6)
> (In reply to Andreas Krebbel from comment #5)
> > In:
> >
> > _1 = src_6(D)->a;
> > dst$val_9 = _1;
> > _2 = BIT_FIELD_REF ;
> > _3 = _2 & 64;
> > if (_3 != 0)
>
> There is only 2 accesses going on in the above IR because SRA removed the
> 3rd when it replaced the access of dst.val with dst$val but didn't update
> BIT_FIELD_REF to remove the byteswap ...
Ok, got it. It isn't the removal of the assignment. As you say it happens in
early SRA when changing dst.val to dst$val and with that going from the union
with the storage order marker to a long int without it. The marker on the
BIT_FIELD_REF needs to be in sync with the marker on its inner reference.
Dropping one without adjusting the other is the problem here. Thanks for the
pointer!
The following change helps with that testcase:
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 8dfc923ed7e..6b1ce6e8b4a 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -3815,8 +3815,13 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi,
bool write)
}
}
else
- *expr = repl;
- sra_stats.exprs++;
+ {
+ if (bfr && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (*expr)))
+ REF_REVERSE_STORAGE_ORDER (bfr) = 0;
+
+ *expr = repl;
+ sra_stats.exprs++;
+ }
}
else if (write && access->grp_to_be_debug_replaced)
{
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #6 from Andrew Pinski --- (In reply to Andreas Krebbel from comment #5) > In: > > _1 = src_6(D)->a; > dst$val_9 = _1; > _2 = BIT_FIELD_REF ; > _3 = _2 & 64; > if (_3 != 0) There is only 2 accesses going on in the above IR because SRA removed the 3rd when it replaced the access of dst.val with dst$val but didn't update BIT_FIELD_REF to remove the byteswap ...
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 --- Comment #5 from Andreas Krebbel --- In: _1 = src_6(D)->a; dst$val_9 = _1; _2 = BIT_FIELD_REF ; _3 = _2 & 64; if (_3 != 0) src, dst and the BIT_FIELD_REF carry storage order flags which result in either bswaps being emitted or, in case of the bitfield, the constant for the compare to be adjusted. So from reading "src" to evaluating "_2" 3 "bswaps" will be applied. After dropping the assignment to dst only two remain which cancel each other out. So in the end we access the value without any adjustments. Just to check I did: diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc index b36dd97802b..b858194a432 100644 --- a/gcc/c-family/c-attribs.cc +++ b/gcc/c-family/c-attribs.cc @@ -1820,6 +1820,7 @@ handle_scalar_storage_order_attribute (tree *node, tree name, tree args, } TYPE_REVERSE_STORAGE_ORDER (type) = reverse; + TYPE_VOLATILE (type) = reverse; return NULL_TREE; } As expected this "fixes" the problem but is probably too big of a hammer here since it basically voids many of the advantages of the attribute which is folding away many of the bswaps.
[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199 Andrew Pinski changed: What|Removed |Added Last reconfirmed||2022-12-22 Summary|Bitfields and |Bitfields, unions and SRA |storage_order_attribute |and storage_order_attribute Ever confirmed|0 |1 Status|UNCONFIRMED |NEW --- Comment #4 from Andrew Pinski --- (In reply to Andreas Krebbel from comment #2) > The problem appears to get introduced when dead store elimination removes > the assignment to the target struct in FRE. There is no DSE in FRE going on Rather SRA did messed up. > > Before FRE we have the following: > > _1 = src_6(D)->a; bswap > dst$val_9 = _1; bswap > _2 = BIT_FIELD_REF ; bswap > _3 = _2 & 64; > if (_3 != 0) dst$val_9/_1 are both SSA names and not a store/load. so there is no swapping going on. It is definitely SRA going wrong. Turning off SRA (-fno-sra) and we get the correct thing at the end.
