[Bug middle-end/107115] New: Wrong codegen from TBAA under stores that change effective type?

[Bug middle-end/107115] New: Wrong codegen from TBAA under stores that change effective type?

[bugdal at aerifal dot cx]

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

            Bug ID: 107115
           Summary: Wrong codegen from TBAA under stores that change
                    effective type?
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bugdal at aerifal dot cx
  Target Milestone: ---

Created attachment 53648
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53648&action=edit
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#comment130510083_42178347,
https://godbolt.org/z/jfv1Ge6v4) and demonstrates what appears to be wrong TBAA
optimization on an object with allocated storage whose effective type changes
under stores. It was first presented as another example of this kind of problem
alongside the example that became
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107107, but it seems likely that
the root cause is distinct.

Reportedly clang/LLVM also transforms this example wrong.

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

[Bug rtl-optimization/107115] Wrong codegen from TBAA under stores that change effective type?

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
          Component|middle-end                  |rtl-optimization
   Last reconfirmed|                            |2022-10-01
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The tree level looks fine, though I didn't look at the aliasing info that is
exported:
  <bb 2> [local count: 1073741824]:
  MEM[(long int *)p4_2(D)] = 1;
  _6 = (long unsigned int) index1_3(D);
  _7 = _6 * 8;
  _8 = p4_2(D) + _7;
  *_8 = 2;
  _9 = (long unsigned int) index2_4(D);
  _10 = _9 * 8;
  _11 = p4_2(D) + _10;
  _12 = *_11;
  MEM[(long int *)_11] = _12;
  _5 = MEM[(long int *)p4_2(D)];
  return _5;

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|rtl-optimization            |middle-end

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There are two bugs I think.

One in expansion and the other in the RTL optimizers.

Without -fno-tree-ter we get:
;; MEM[(long int *)_11] = _12;

(nil)

And then we don't record the aliasing change.
This because we had got:
;; _12 = *_11;
;; MEM[(long int *)_11] = _12;


With -fno-tree-ter, we lose it during combine (on the trunk):
Trying 16 -> 17:
   16: r88:DI=[r87:DI]
   17: [r87:DI]=r88:DI
      REG_DEAD r88:DI
      REG_DEAD r87:DI
Failed to match this instruction:
(set (mem:DI (reg/f:DI 87 [ _11 ]) [1 MEM[(long int *)_11]+0 S8 A64])
    (mem:DI (reg/f:DI 87 [ _11 ]) [2 *_11+0 S8 A64]))
allowing combination of insns 16 and 17
original costs 5 + 4 = 9
replacement cost 5
deferring deletion of insn with uid = 16.
modifying insn i3    17: [r87:DI]=[r87:DI]
      REG_DEAD r87:DI
deferring rescan insn with uid = 17.


The aliasing set 1 represents `long` and 2 represents `long long` (see the
difference there).

I don't know what is the best way to represent an aliasing set change even
though the value didn't change on the RTL level.

This needed for both expand and combine (and maybe a few other places).


I should note the Gimple level was fixed for this testcase between GCC 6.2 and
GCC 6.3 so it would be interesting to find the patch which fixed that.

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So possibly expand_assignment is eliding the store somewhere downthread
(it's operand_equal_p check shouldn't fire).  It's

rtx
store_expr (tree exp, rtx target, int call_param_p,
            bool nontemporal, bool reverse)
{
...
  if ((! rtx_equal_p (temp, target)
       || (temp != target && (side_effects_p (temp)
                              || side_effects_p (target))))

which doesn't fire.  We have

(gdb) p debug_rtx (temp)
(mem:DI (reg/f:DI 87 [ _11 ]) [2 *_11+0 S8 A64])
(gdb) p debug_rtx (target)
(mem:DI (reg/f:DI 87 [ _11 ]) [1 MEM[(long int *)_11]+0 S8 A64])

Note that even just matching up MEM_ALIAS_SET isn't enough.  I'd call the
above optimization premature at least.

(gdb) p mems_same_for_tbaa_p (temp, target)
$13 = false

so

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 80bb1b8a4c5..71a7fc19c42 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -6207,7 +6207,8 @@ store_expr (tree exp, rtx target, int call_param_p,

   if ((! rtx_equal_p (temp, target)
        || (temp != target && (side_effects_p (temp)
-                             || side_effects_p (target))))
+                             || side_effects_p (target)
+                             || !mems_same_for_tbaa_p (temp, target))))
       && TREE_CODE (exp) != ERROR_MARK
       /* If store_expr stores a DECL whose DECL_RTL(exp) == TARGET,
         but TARGET is not valid memory reference, TEMP will differ

fixes this mistake.  But then we run into the sched issue which
-fno-schedule-insns2 "fixes".

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:0af8d957d5911fc7659b4174cfc2213289bbed23

commit r13-3131-g0af8d957d5911fc7659b4174cfc2213289bbed23
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Oct 6 11:48:03 2022 +0200

    middle-end/107115 - avoid bogus redundant store removal during RTL
expansion

    The following preserves the (premature) redundant store removal
    done in store_expr by appropriately guarding it with
    mems_same_for_tbaa_p.  The testcase added needs scheduling disabled
    for now since there's a similar bug there still present.

            PR middle-end/107115
            * expr.cc (store_expr): Check mems_same_for_tbaa_p before
            eliding a seemingly redundant store.

            * gcc.dg/torture/pr107115.c: New testcase.

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
The bug in instruction scheduling remains (if it is really there).

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org

--- Comment #6 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Cc'ing Jakub for the problematic i386.md peephole that loses alias set info.

As Andrew mentioned in comment #2, the next stop is combine, which sees

(set (mem:DI (plus:DI (mult:DI (sign_extend:DI (reg:SI 101))
                (const_int 8 [0x8]))
            (reg/v/f:DI 90 [ p4 ])) [1 MEM[(long int *)_11]+0 S8 A64])
    (mem:DI (plus:DI (mult:DI (sign_extend:DI (reg:SI 101))
                (const_int 8 [0x8]))
            (reg/v/f:DI 90 [ p4 ])) [2 *_11+0 S8 A64]))

as a no-op move and removes it (but note differing alias sets in the MEMs).

And with -fdisable-rtl-combine it is then broken by peephole2 of all things:

;; Attempt to optimize away memory stores of values the memory already
;; has.  See PR79593.
(define_peephole2
  [(set (match_operand 0 "register_operand")
        (match_operand 1 "memory_operand"))
   (set (match_operand 2 "memory_operand") (match_dup 0))]
  "!MEM_VOLATILE_P (operands[1])
   && !MEM_VOLATILE_P (operands[2])
   && rtx_equal_p (operands[1], operands[2])
   && !reg_overlap_mentioned_p (operands[0], operands[2])"
  [(set (match_dup 0) (match_dup 1))])

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If we don't want to treat such copies as noop moves, then either we need to
change rtx_equal_p such that it will say MEMs aren't equal if the the alias
sets are different, or should tweak in the same spirit the i386.md peephole2
and set_noop_p (and perhaps some other spot).  But if we do either of that, it
would be nice to get rid of the for assembly actually noop move later on
shortly before expansion or emit it as nothing, because assembler then doesn't
care about alias sets.

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[amonakov at gcc dot gnu.org]

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

--- Comment #8 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Just optimizing out the redundant store seems difficult because on some targets
scheduling is invoked from reorg (and it relies on alias sets).

We need a solution that works for combine too — is it possible to invent a
representation for a no-op in-place MEM "move" that only changes its alias set?

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #8) 
> We need a solution that works for combine too — is it possible to invent a
> representation for a no-op in-place MEM "move" that only changes its alias
> set?

That is the same as my comment here:
> I don't know what is the best way to represent an aliasing set change
> even though the value didn't change on the RTL level.

Clobber might work though I don't know if that is 100% correct as the value
didn't change so maybe a new RTL code for this? But that would require many
changes in the backend ...

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #9)
> (In reply to Alexander Monakov from comment #8) 
> > We need a solution that works for combine too — is it possible to invent a
> > representation for a no-op in-place MEM "move" that only changes its alias
> > set?
> 
> That is the same as my comment here:
> > I don't know what is the best way to represent an aliasing set change
> > even though the value didn't change on the RTL level.
> 
> Clobber might work though I don't know if that is 100% correct as the value
> didn't change so maybe a new RTL code for this? But that would require many
> changes in the backend ...

We need something similar on GIMPLE but I have not yet made up my mind
for something "nice" either ... I'm leaning towards something similar
to a CLOBBER on GIMPLE since that allows us to simply preserve the LHS,
so ...

 MEM[(long int *)_11] = {NOOP};

but we have to make sure this isn't perceived as KILL for DSE purposes
though it does have KILL semantics otherwise (it needs to be a barrier
for load motion).  DSE could of course replace the RHS when eliding an
earlier store.

On RTL a CLOBBER would have the same problem (with DSE), so maybe it
should be a

(parallel
  [(use (MEM ...))]
  [(clobber (MEM ...))])

?  That's also the alternative on GIMPLE - have it be

 MEM[(long int *)_11] = MEM[...];

with the caveat (same as above) of breaking the register typed temporary
rule.

On RTL other unwanted side-effects would be that register allocation
and scheduling have to handle the address compute of the MEM unless
we cobble up everything not allocated into a complex expression and
not require those MEMs to be recognized ...

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> If we don't want to treat such copies as noop moves, then either we need to
> change rtx_equal_p such that it will say MEMs aren't equal if the the alias
> sets are different

But in other context we can treat them as equal ...

> or should tweak in the same spirit the i386.md peephole2
> and set_noop_p (and perhaps some other spot).

Yep, we've been playing whack-a-mole already, so we should just continue
and fix places that pop up with this issue.

> But if we do either of that,
> it would be nice to get rid of the for assembly actually noop move later on
> shortly before expansion or emit it as nothing, because assembler then
> doesn't care about alias sets.

We eventually could do a flag_strict_aliasing = 0 at some point?  Similar
to the idea of doing flag_wrapv = 1 to allow more association.

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[amonakov at gcc dot gnu.org]

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

--- Comment #12 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
For reference, the previous whacked mole appears to be PR 106187 (where
mems_same_for_tbaa_p comes from).

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[gabravier at gmail dot com]

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

Gabriel Ravier <gabravier at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabravier at gmail dot com

--- Comment #13 from Gabriel Ravier <gabravier at gmail dot com> ---
Idk if it qualifies as the same bug or if this will prove to bee particularly
useful, but just to make sure the corresponding issue in C++, triggered by this
slightly altered code (so that it hopefully respects the stricter rules C++ has
w.r.t. implicit object creation) which also fails to execute correctly on GCC,
is fixed by the fix to this issue, I'll post here the corresponding code:

#include <new>
#include <stdio.h>
#include <stdlib.h>

void test1(long *p1)
{
    p1 = (long *)new ((char*)p1) char[sizeof(long)];
    p1[0] = 1;
}

long test2(long long *p2, int index1, int index2)
{
    p2 = (long long *)new ((char*)p2) char[sizeof(long long)];
    p2[index1] = 2;
    return p2[index2];
}

long test3(long *p3, int index2, long value)
{
    p3 = (long *)new ((char*)p3) char[sizeof(long)];
    p3[index2] = 3;
    p3[index2] = value;
    return p3[0];
}

long test4(void *p4, int index1, int index2)
{
    test1((long *)p4);
    long temp = test2((long long *)p4, index1, index2);
    return test3((long *)p4, index2, temp);
}

long (*volatile vtest)(void *, int, int) = test4;

int main(void)
{
    void *pp = malloc(sizeof(long long));
    if (!pp) abort();
    long result = vtest(pp, 0, 0);
    printf("%lu/%lu\n", *std::launder((long *)pp), result);
}

[Bug middle-end/107115] Wrong codegen from TBAA under stores that change effective type?

[gabravier at gmail dot com]

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

--- Comment #14 from Gabriel Ravier <gabravier at gmail dot com> ---
Actually I think there's some aliasing violations in the C++ code w.r.t. the
re-usage of `p4` after another object has been created in its place so I think
this code would be more correct:

void test1(long *p1)
{
    p1 = (long *)new ((char*)p1) char[sizeof(long)];
    p1[0] = 1;
}

long test2(long long *p2, int index1, int index2)
{
    p2 = (long long *)new ((char*)p2) char[sizeof(long long)];
    p2[index1] = 2;
    return p2[index2];
}

long test3(long *p3, int index2, long value)
{
    p3 = (long *)new ((char*)p3) char[sizeof(long)];
    p3[index2] = 3;
    p3[index2] = value;
    return p3[0];
}

long test4(void *p4, int index1, int index2)
{
    test1((long *)p4);
    long temp = test2((long long *)std::launder((long *)p4), index1, index2);
    return test3((long *)std::launder((long long *)p4), index2, temp);
}

long (*volatile vtest)(void *, int, int) = test4;

int main(void)
{
    void *pp = malloc(sizeof(long long));
    if (!pp) abort();
    long result = vtest(pp, 0, 0);
    printf("%lu/%lu\n", *std::launder((long *)pp), result);
}