Bug 107107 - [10 Regression] Wrong codegen from TBAA when stores to distinct same-mode types are collapsed?
Summary: [10 Regression] Wrong codegen from TBAA when stores to distinct same-mode typ...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: unknown
: P3 normal
Target Milestone: 10.5
Assignee: Richard Biener
URL:
Keywords: alias, wrong-code
Depends on:
Blocks:
 
Reported: 2022-09-30 21:34 UTC by Rich Felker
Modified: 2023-01-26 13:06 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.4.1, 11.3.1, 12.2.1, 13.0
Known to fail: 10.4.0, 11.3.0, 12.2.0, 4.1.2, 4.4.7, 4.5.3
Last reconfirmed: 2022-10-01 00:00:00


Attachments
original test case by supercat (230 bytes, text/plain)
2022-09-30 21:34 UTC, Rich Felker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rich Felker 2022-09-30 21:34:15 UTC
Created attachment 53646 [details]
original test case by supercat

The attached test case is from user supercat on Stack Overflow (original source: https://stackoverflow.com/questions/42178179/will-casting-around-sockaddr-storage-and-sockaddr-in-break-strict-aliasing/42178347?noredirect=1#comment130509588_42178347, https://godbolt.org/z/83v4ssrn4) and demonstrates wrong TBAA apparently assuming an object of type long long was not modified after the code path modifying it was collapsed with a different code path performing the modification via an lvalue of type long.

On 64-bit targets, the test program outputs 1/2 with optimization levels that enable -fstrict-aliasing. The expected output is 2/2. Using -fno-strict-aliasing fixes it.

I have not checked this myself, but according to others who have looked at the test case, the regression came between GCC 4.7 and 4.8.
Comment 1 Rich Felker 2022-10-01 02:25:11 UTC
There's also a potentially related test case at https://godbolt.org/z/jfv1Ge6v4 - I'm not yet clear on whether it's likely to have the same root cause.
Comment 2 Andrew Pinski 2022-10-01 02:59:13 UTC
Confirmed. I don't think it is exactly TBAA which is causing the issue but rather the way PRE does aliasing walks.

Take:
```
static inline void set_longish(int is_long, void *p, long x)
{
    if (is_long)
        *(long*)p = x;
    else
        *(long long*)p = x;
}
static long test(long long *p, int index, int mode)
{
    *p = 1;
    set_longish(mode, p+index, 2);
    return *p;
}
long (*volatile vtest)(long long*, int, int) = test;
#include <stdio.h>
int main(void)
{
    long long x;
    long result = vtest(&x, 0, 0);
    printf("%lu/%llu\n", result, x);
}
```
Which is only difference by which order the if statement (and mode which is swapped). This case works.
Comment 3 Andrew Pinski 2022-10-01 03:02:11 UTC
Here is a testcase which has failed since before 4.1.2:
```
static inline void set_longish(int is_long_long, void *p, long x)
{
    if (is_long_long)
        *(long long*)p = x;
    else
        *(long*)p = x;
}
static long test(long long *p, int index, int mode)
{
    *p = 1;
    set_longish(mode, p+index, 2);
    return *p;
}
long (*volatile vtest)(long long*, int, int) = test;
#include <stdio.h>
int main(void)
{
    long long x;
    long result = vtest(&x, 0, 1);
    printf("%lu/%llu\n", result, x);
}
```
The only difference from the original testcase is marking set_longish as static inline. I suspect what changed between 4.7 and 4.8 for the original testcase is inlining.
Comment 4 Andrew Pinski 2022-10-01 03:05:28 UTC
Note this has been to be a regression since tree level PRE is what is causing the issue and that was added in GCC 4 or 4.1.
Comment 5 Andrew Pinski 2022-10-01 03:05:49 UTC
(In reply to Rich Felker from comment #1)
> There's also a potentially related test case at
> https://godbolt.org/z/jfv1Ge6v4 - I'm not yet clear on whether it's likely
> to have the same root cause.

This might be a different issue I think.
Comment 6 Alexander Monakov 2022-10-01 17:08:35 UTC
(In reply to Andrew Pinski from comment #5)
> (In reply to Rich Felker from comment #1)
> > There's also a potentially related test case at
> > https://godbolt.org/z/jfv1Ge6v4 - I'm not yet clear on whether it's likely
> > to have the same root cause.
> 
> This might be a different issue I think.

Yeah, that's sched2 reordering the accesses (probably cselib is confused). Needs a separate report.
Comment 7 Rich Felker 2022-10-01 20:01:13 UTC
Second one filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107115
Comment 8 Richard Biener 2022-10-06 09:02:17 UTC
I will have a look.
Comment 9 Richard Biener 2022-10-06 09:22:18 UTC
The issue is how we value-number for tail-merging.  We do

Processing block 1: BB4
Value numbering stmt = MEM[(long int *)_3] = 2;
RHS 2 simplified to 2
No store match
Value numbering store MEM[(long int *)_3] to 2
Setting value number of .MEM_12 to .MEM_12 (changed)
marking outgoing edge 4 -> 5 executable
Processing block 2: BB3
Value numbering stmt = *_3 = 2;
RHS 2 simplified to 2
Setting value number of .MEM_11 to .MEM_12 (changed)
marking outgoing edge 3 -> 5 executable
Processing block 3: BB5
Value numbering stmt = .MEM_10 = PHI <.MEM_11(3), .MEM_12(4)>
Setting value number of .MEM_10 to .MEM_12 (changed)
Value numbering stmt = _9 = *p_5(D);
Setting value number of _9 to 1 (changed)

oops.  So we figure that the two stores from '2' are "redundant" which causes
us to only consider one (the wrong one) when later looking up *p_5(D).

That's

  if (!resultsame)
    {
      /* Only perform the following when being called from PRE
         which embeds tail merging.  */
      if (default_vn_walk_kind == VN_WALK)
        {
          assign = build2 (MODIFY_EXPR, TREE_TYPE (lhs), lhs, op);
          vn_reference_lookup (assign, vuse, VN_NOWALK, &vnresult, false);
          if (vnresult)
            {
              VN_INFO (vdef)->visited = true;
              return set_ssa_val_to (vdef, vnresult->result_vdef);
            }

that's a case I never understood fully (and that seems wrong).  That
visited flag set is also odd.

I'm testing a patch.
Comment 10 GCC Commits 2022-10-06 10:07:35 UTC
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:85333b9265720fc4e49397301cb16324d2b89aa7

commit r13-3126-g85333b9265720fc4e49397301cb16324d2b89aa7
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Oct 6 11:20:16 2022 +0200

    tree-optimization/107107 - tail-merging VN wrong-code
    
    The following fixes an unintended(?) side-effect of the special
    MODIFY_EXPR expression entries we add for tail-merging during VN.
    We shouldn't value-number the virtual operand differently here.
    
            PR tree-optimization/107107
            * tree-ssa-sccvn.cc (visit_reference_op_store): Do not
            affect value-numbering when doing the tail merging
            MODIFY_EXPR lookup.
    
            * gcc.dg/pr107107.c: New testcase.
Comment 11 Richard Biener 2022-10-06 10:08:01 UTC
Fixed on trunk sofar.
Comment 12 GCC Commits 2022-10-17 13:10:45 UTC
The releases/gcc-12 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

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

commit r12-8838-gff0a274e5c3026b105c7f51126fa51f8178fa42c
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Oct 6 11:20:16 2022 +0200

    tree-optimization/107107 - tail-merging VN wrong-code
    
    The following fixes an unintended(?) side-effect of the special
    MODIFY_EXPR expression entries we add for tail-merging during VN.
    We shouldn't value-number the virtual operand differently here.
    
            PR tree-optimization/107107
            * tree-ssa-sccvn.cc (visit_reference_op_store): Do not
            affect value-numbering when doing the tail merging
            MODIFY_EXPR lookup.
    
            * gcc.dg/pr107107.c: New testcase.
    
    (cherry picked from commit 85333b9265720fc4e49397301cb16324d2b89aa7)
Comment 13 GCC Commits 2022-12-12 16:33:31 UTC
The releases/gcc-11 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

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

commit r11-10417-gfa11fc62ddee81a8bc3e69d5e3180695a6dbb666
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Oct 6 11:20:16 2022 +0200

    tree-optimization/107107 - tail-merging VN wrong-code
    
    The following fixes an unintended(?) side-effect of the special
    MODIFY_EXPR expression entries we add for tail-merging during VN.
    We shouldn't value-number the virtual operand differently here.
    
            PR tree-optimization/107107
            * tree-ssa-sccvn.c (visit_reference_op_store): Do not
            affect value-numbering when doing the tail merging
            MODIFY_EXPR lookup.
    
            * gcc.dg/pr107107.c: New testcase.
    
    (cherry picked from commit 85333b9265720fc4e49397301cb16324d2b89aa7)
Comment 14 GCC Commits 2023-01-26 13:05:43 UTC
The releases/gcc-10 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:58e39fcaaf298ff54b6f1a45fa9d15390e8113fb

commit r10-11177-g58e39fcaaf298ff54b6f1a45fa9d15390e8113fb
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Oct 6 11:20:16 2022 +0200

    tree-optimization/107107 - tail-merging VN wrong-code
    
    The following fixes an unintended(?) side-effect of the special
    MODIFY_EXPR expression entries we add for tail-merging during VN.
    We shouldn't value-number the virtual operand differently here.
    
            PR tree-optimization/107107
            * tree-ssa-sccvn.c (visit_reference_op_store): Do not
            affect value-numbering when doing the tail merging
            MODIFY_EXPR lookup.
    
            * gcc.dg/pr107107.c: New testcase.
    
    (cherry picked from commit 85333b9265720fc4e49397301cb16324d2b89aa7)
Comment 15 Richard Biener 2023-01-26 13:06:27 UTC
Fixed.