[Bug tree-optimization/108199] New: Bitfields and storage_order_attribute

[Bug tree-optimization/108199] New: Bitfields and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

            Bug ID: 108199
           Summary: Bitfields and storage_order_attribute
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: krebbel at gcc dot gnu.org
  Target Milestone: ---

[Bug tree-optimization/108199] Bitfields and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #1 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Created attachment 54150
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54150&action=edit
Testcase

[Bug tree-optimization/108199] Bitfields and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |x86_64
              Build|                            |x86_64
           Keywords|                            |wrong-code
               Host|                            |x86_64

--- Comment #2 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
The testcase does an assigned between two struct with endianess differing from
host endianess (assumed to be little). Here the required byteswaps are supposed
to cancel each other out. After that a bitfield comparison on the target struct
is done. This comparison uses the wrong byte offset into the bitfield:

        testb   $64, 7(%rdi)
        jne     .L11

On a big endian target the first bits in the bitfield are supposed to reside in
the first bytes in memory.

The problem appears to get introduced when dead store elimination removes the
assignment to the target struct in FRE.

Before FRE we  have the following:

  _1 = src_6(D)->a;               bswap
  dst$val_9 = _1;                 bswap
  _2 = BIT_FIELD_REF <dst$val_9, 8, 0>;  bswap
  _3 = _2 & 64;
  if (_3 != 0)
...
This would result in 3 bswaps chained to each other. However, after FRE we have
only two because the dead store to dst$val is removed.

  _1 = src_6(D)->a;
  _2 = BIT_FIELD_REF <_1, 8, 0>;
  _3 = _2 & 64;
  if (_3 != 0)

Now we have only which cancel each other out.

Looks like we have to prevent depending stores/loads with different endianess
from getting removed - perhaps by making them also volatile? I think we have to
keep the number of memory accesses with foreign endianess constant over the
optimizations.

[Bug tree-optimization/108199] Bitfields and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #3 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Moving the local definition of dst out of the function to global scope prevents
the store from getting eliminated.

union DST dst;

As expected the store is still in the FRE dump:

  _1 = src_6(D)->a;
  dst.val = _1;                <---
  _2 = BIT_FIELD_REF <dst, 8, 0>;
  _3 = _2 & 64;
  if (_3 != 0)
...

and the first by is accessed:

bar:
        movq    (%rdi), %rax
        movq    %rax, dst(%rip)
        testb   $64, %al
        jne     .L11

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Andrew Pinski <pinskia at gcc dot gnu.org> 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 <pinskia at gcc dot gnu.org> ---
(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 <dst$val_9, 8, 0>;  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.

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #5 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
In:

  _1 = src_6(D)->a;
  dst$val_9 = _1;
  _2 = BIT_FIELD_REF <dst$val_9, 8, 0>;
  _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

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andreas Krebbel from comment #5)
> In:
> 
>   _1 = src_6(D)->a;
>   dst$val_9 = _1;
>   _2 = BIT_FIELD_REF <dst$val_9, 8, 0>;
>   _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

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #7 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
(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 <dst$val_9, 8, 0>;
> >   _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

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Please mention the exact compiler version and the compilation switches.

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
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(&t);
    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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #11 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> 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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> 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 <dst$val_9, 8, 0>;

In this case BIT_FIELD_REF <dst$val_9, 8, 0> 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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Eric Botcazou <ebotcazou at gcc dot gnu.org> 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 <ebotcazou at gcc dot gnu.org> ---
Fixing.

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:3e1cba12a8d71e70235a9a9b8f1a237a561db3e7

commit r13-5109-g3e1cba12a8d71e70235a9a9b8f1a237a561db3e7
Author: Eric Botcazou <ebotcazou@adacore.com>
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

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #15 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Your patch fixes the problem for me. Thanks for the quick fix!

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|13.0                        |12.2.1

--- Comment #16 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #17 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Yes, that's indeed the plan.

[Bug tree-optimization/108199] Bitfields, unions and SRA and storage_order_attribute

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:eec3a65ed638a1c58fa08ddf508d2d60b64d311d

commit r12-9041-geec3a65ed638a1c58fa08ddf508d2d60b64d311d
Author: Eric Botcazou <ebotcazou@adacore.com>
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

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

--- Comment #19 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:01e80c4c630a5a6286a521b047d0ef80631c892c

commit r11-10463-g01e80c4c630a5a6286a521b047d0ef80631c892c
Author: Eric Botcazou <ebotcazou@adacore.com>
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

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108199

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |11.4
             Status|REOPENED                    |RESOLVED

--- Comment #20 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Fixed on mainline, 12 and 11 branches.