[Bug tree-optimization/115298] New: [15 Regression] Various targets failing DSE tests after recent changes

[Bug tree-optimization/115298] New: [15 Regression] Various targets failing DSE tests after recent changes

[law at gcc dot gnu.org]

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

            Bug ID: 115298
           Summary: [15 Regression] Various targets failing DSE tests
                    after recent changes
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: law at gcc dot gnu.org
  Target Milestone: ---

A few targets (nds32be-elf, nds32le-elf, avr-elf) have started failing a few
tests after recent aliasing changes:

Tests that now fail, but worked before (9 tests):

nds32-sim: gcc: gcc.c-torture/execute/strcpy-1.c   -O3 -g  (test for excess
errors)
nds32-sim: gcc: gcc.dg/tree-ssa/pr86061.c scan-tree-dump-times dse1 "Deleted
dead call" 1
nds32-sim: gcc: gcc.dg/tree-ssa/pr86061.c scan-tree-dump-times dse1 "Deleted
dead call" 1
nds32-sim: gcc: gcc.dg/tree-ssa/ssa-dse-37.c scan-tree-dump-times dse1 "Deleted
dead call" 2
nds32-sim: gcc: gcc.dg/tree-ssa/ssa-dse-37.c scan-tree-dump-times dse1 "Deleted
dead call" 2
nds32-sim: gcc: gcc.dg/tree-ssa/ssa-dse-37.c scan-tree-dump-times dse1
"Trimming statement " 4
nds32-sim: gcc: gcc.dg/tree-ssa/ssa-dse-37.c scan-tree-dump-times dse1
"Trimming statement " 4
nds32-sim: gcc: gcc.dg/tree-ssa/strncpy-1.c scan-tree-dump-not optimized
"memset"
nds32-sim: gcc: gcc.dg/tree-ssa/strncpy-1.c scan-tree-dump-not optimized
"memset"

This was bisected to:
commit c08b0d3f7b3539b26031de31d88dea6b94474577
Author: Richard Biener <rguenther@suse.de>
Date:   Mon May 27 10:41:02 2024 +0200

    tree-optimization/115236 - more points-to *ANYTHING = x fixes

    The stored-to ANYTHING handling has more holes, uncovered by treating
    volatile accesses as ANYTHING.  We fail to properly build the
    pred and succ graphs, in particular we may not elide direct nodes
    from receiving from STOREDANYTHING.

            PR tree-optimization/115236
            * tree-ssa-structalias.cc (build_pred_graph): Properly
            handle *ANYTHING = X.
            (build_succ_graph): Likewise.  Do not elide direct nodes
            from receiving from STOREDANYTHING.

            * gcc.dg/pr115236.c: New testcase.


I haven't done any deeper analysis.

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-05-31
           Keywords|                            |testsuite-fail
     Ever confirmed|0                           |1
   Target Milestone|---                         |15.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Huh, I honestly have no idea how those targets would differ here ...

I do see

void h (char * s)
{
  # PT = anything
  char * s_3(D) = s;
  char a[8];

  <bb 2> :
  __builtin_memset (&a, 0, 8);
  __builtin_strncpy (&a, s_3(D), 8);
  # USE = anything
  # CLB = anything
  frob (&a);
  a ={v} {CLOBBER(eos)};
  return;

for nds32-sim but

  Deleted dead call: __builtin_memset (&a, 0, 8);

void h (char * s)
{
  # PT = nonlocal null
  char * s_3(D) = s;
  char a[8];

  <bb 2> :
  __builtin_strncpy (&a, s_3(D), 8);
  # USE = nonlocal escaped null { D.2716 } (escaped)
  # CLB = nonlocal escaped null { D.2716 } (escaped)
  frob (&a);
  a ={v} {CLOBBER(eos)};
  return;

for x86-64.  But then the points-to solutions should not make any difference
for DSE in this case ... (the points-to difference is odd in the first place
of course).

So for the points-to difference this is caused by

-a = &NULL
+a = INTEGER

which likely means a different default of -fno-delete-null-pointer-checks
or ADDR_SPACE_ADDRESS_ZERO_VALID.  That causes us to bring in what the
object at (void *)0 points to, and that's ANYTHING since we do not track
objects at constant addresses in any way, and those might alias all other
objects.  The question is more why we generate a = &NULL at all, but that's
a pre-existing issue.  We now simply handle all this correctly (we didn't
before, with latent wrong-code).

Ah, and the DSE effect then is obviously that now 'strncpy (&a, s_3(D),..)'
reads from a since s_3(D) points to anything now (which includes 'a'), so
we can no longer remove/trim an earlier store to 'a'.

Ah, and the a = &NULL constraint is from the memset.

Since we pass a to frob it escapes and everything escaped memory points
to also escapes so anything escapes.

So I'd say it works correctly now.

There might be a missing indirection between NONLOCAL and ESCAPED.  Since
s = &NONLOCAL even when anything is in ESCAPED anything isn't NONLOCAL
itself (well, but of course technically s can point to NULL as well -
another latent incorrectness in PTA, we do not track NULL conservatively,
a correctness mistake with ADDR_SPACE_ADDRESS_ZERO_VALID).

Btw, changing the testcases to

extern void frob (char *);

void h (char *s)
{
  char a[8];
  __builtin_memset (a, 1, sizeof a);
  __builtin_strncpy (a, s, sizeof a);
  frob (a);
}

shows the same effect on x86_64 - suddenly 'a' points to ANYTHING
(0x010101010101...), which makes 's' point to ANYTHING and DSE is gone.

Confirmed for the testsuite regression.  I don't see how this is a bug
though.  Maybe the stack object 'a' can never be at address zero?  Or
any "fixed" address?  I'm not sure that such constraint can be modeled in PTA
("split" ANYTHING somehow).

Adding -fdelete-null-pointer-checks to the test makes it succeed also on
nds32le-elf.

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes

[law at gcc dot gnu.org]

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

--- Comment #2 from Jeffrey A. Law <law at gcc dot gnu.org> ---
What still doesn't make sense is why nds32 would be special here.  It doesn't
do anything special with flag_delete_null_pointer_checks and I don't think it
uses any of the address space hooks.  So why does nds32 behave differently than
x86?

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes

[rguenther at suse dot de]

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

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 31 May 2024, law at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115298
> 
> --- Comment #2 from Jeffrey A. Law <law at gcc dot gnu.org> ---
> What still doesn't make sense is why nds32 would be special here.  It doesn't
> do anything special with flag_delete_null_pointer_checks and I don't think it
> uses any of the address space hooks.  So why does nds32 behave differently than
> x86?

/* Implement TARGET_OPTION_OPTIMIZATION_TABLE.  */ 
static const struct default_options nds32_option_optimization_table[] =
{  
#if TARGET_LINUX_ABI == 0
  /* Disable -fdelete-null-pointer-checks by default in ELF toolchain.  */
  { OPT_LEVELS_ALL,               OPT_fdelete_null_pointer_checks,
                                                           NULL, 0 },
#endif

that's in common/config/nds32/nds32-common.cc, a place to easily
overlook.  So it should be nds32-elf only.  And yeah, points-to
should use TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID but since it tracks
pointers across integer conversions we have to decide on integer
zero as well where we do not know what address-space is affected.
I guess we could use the default address space and try to be clever
with ADDR_SPACE_CONVERT.  So points-to only checks 
flag_delete_null_pointer_checks at the moment.

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes

[law at gcc dot gnu.org]

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

--- Comment #4 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Agh.  I was looking in the main config directory, not common/config.  So it all
makes sense now.

So if we go back to your original analysis, I think we can say things are
behaving correctly and we just need to adjust the testcase to either skip on
these targets or add the flag.

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4

[Bug tree-optimization/115298] [15 Regression] Various targets failing DSE tests after recent changes due to default of -fno-fdelete-null-pointer-checks on those targets

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 31 May 2024, law at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115298
> 
> --- Comment #4 from Jeffrey A. Law <law at gcc dot gnu.org> ---
> Agh.  I was looking in the main config directory, not common/config.  So it all
> makes sense now.
> 
> So if we go back to your original analysis, I think we can say things are
> behaving correctly and we just need to adjust the testcase to either skip on
> these targets or add the flag.

Yes.  It's still bad now with a memset(p, 1) having the same effect on
other archs.  But it's still "correct" given in the caller one could do

  if (q == (void *)0x0101010101010101)
    foo (p, q);

so I don't really see how we can avoid this without magic hand-waving
and muttering "unlikely", aka not what a compiler should do.

Of course it now means that every time an INTEGER escapes which means
always (a literal INTEGER would always have to be considered escaped)
everything breaks down.  So do we need to consider

void bar () { *(int *)0xdeadbeef = 1; }

int foo ()
{
  int x = 0;
  bar ();
  return x;
}

and consider carefully laid out stack so bar () clobbers 'x'?  We
do have to assume 0xdeadbeef is a valid address as otherwise UB.

We do close bugs as INVALID that use the address of 'a' to get to
'b' for 'int a, b;' based on pointer arithmetic rules but we
have bugs with conditional copy propagation wrecking things.

That said, I'm somewhat considering to change INTEGER address
behavior in points-to, making them _not_ alias any named object.
Would that work in practice?  We currently have

ANYTHING = &ANYTHING
ESCAPED = *ESCAPED
ESCAPED = ESCAPED + UNKNOWN
*ESCAPED = NONLOCAL
NONLOCAL = &NONLOCAL
NONLOCAL = &ESCAPED
INTEGER = &ANYTHING

and I'd change default INTEGER constraints to

INTEGER = &INTEGER
INTEGER = NONLOCAL

'INTEGER' are objects at literal addresses, they can initially
refer to other 'INTEGER' objects and to global (named) objects.
That excludes pointing to the stack top for example (we'd be
back to ANYTHING if that's OK).

Anyway, we'll see if any concrete performance issues arise from
the fixed ANYTHING handling.