[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
  Known to work||7.3.1
 Resolution|--- |FIXED

--- Comment #10 from Richard Biener  ---
Fixed.

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener  ---
Author: rguenth
Date: Tue Nov 20 14:47:49 2018
New Revision: 266318

URL: https://gcc.gnu.org/viewcvs?rev=266318=gcc=rev
Log:
2018-11-20  Richard Biener  

Backport from mainline
2018-03-12  Richard Biener  

PR tree-optimization/84777
* tree-ssa-loop-ch.c (should_duplicate_loop_header_p): For
force-vectorize loops ignore whether we are optimizing for size.

2018-01-26  Richard Biener  

PR rtl-optimization/84003
* dse.c (record_store): Only record redundant stores when
the earlier store aliases at least all accesses the later one does.

* g++.dg/torture/pr77745.C: Mark foo noinline to trigger
latent bug in DSE if NOINLINE is appropriately defined.
* g++.dg/torture/pr77745-2.C: New testcase including pr77745.C
and defining NOINLINE.

Added:
branches/gcc-7-branch/gcc/testsuite/g++.dg/torture/pr77745-2.C
Modified:
branches/gcc-7-branch/gcc/ChangeLog
branches/gcc-7-branch/gcc/dse.c
branches/gcc-7-branch/gcc/testsuite/ChangeLog
branches/gcc-7-branch/gcc/testsuite/g++.dg/torture/pr77745.C
branches/gcc-7-branch/gcc/tree-ssa-loop-ch.c

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener  ---
I'm going to backport.

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[marxin at gcc dot gnu.org]

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

--- Comment #7 from Martin Liška  ---
Richi: Will you backport or do we close it?

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

  Known to work||8.0
  Known to fail||7.3.0

--- Comment #6 from Richard Biener  ---
Fixed on trunk sofar.  I'm considering a backport to GCC 7.

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener  ---
Author: rguenth
Date: Fri Jan 26 14:50:25 2018
New Revision: 257091

URL: https://gcc.gnu.org/viewcvs?rev=257091=gcc=rev
Log:
2018-01-26  Richard Biener  

PR rtl-optimization/84003
* dse.c (record_store): Only record redundant stores when
the earlier store aliases at least all accesses the later one does.

* g++.dg/torture/pr77745.C: Mark foo noinline to trigger
latent bug in DSE if NOINLINE is appropriately defined.
* g++.dg/torture/pr77745-2.C: New testcase including pr77745.C
and defining NOINLINE.

Added:
trunk/gcc/testsuite/g++.dg/torture/pr77745-2.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/dse.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/testsuite/g++.dg/torture/pr77745.C

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener  ---
Looks like we eventually need sth like

  /* If the later store we delete could have changed the
 dynamic type of the memory make sure the one we
 preserve serves as a store for all loads that could
 validly have accessed the one we delete.  */
  store_info *r_info = s_info->redundant_reason->store_rec;
  while (r_info)
{
  if (r_info->is_set
  && (MEM_ALIAS_SET (s_info->mem)
  != MEM_ALIAS_SET (r_info->mem)))
set_mem_alias_set (r_info->mem, 0);
  r_info = r_info->next;
}

to catch them all?  Also there's another "copy" of the code in dse_step6.

Adjusting the patch accordingly.

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||ebotcazou at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |rguenth at gcc dot 
gnu.org

--- Comment #3 from Richard Biener  ---
Ok, so it's not scheduling but RTL DSE removing the dynamic type changing
store:

**scanning insn=16
mems_found = 0, cannot_delete = false
Locally deleting insn 9 because insn 8 stores the same value and couldn't be
eliminated
Locally deleting insn 9

this is exactly the "feature" we had to guard properly on the GIMPLE level.
Later it isn't CSE that messes up things (like on GIMPLE) but the new
sequence is of course prone to scheduling.

But instead of not doing the dead store removal like on GIMPLE we can
choose to adjust the earlier store accordingly (we don't get a hold on
that easily on GIMPLE, but it seems to be available in DSE).

So the following fixes the testcase, still removes the redundant store
but possibly pessimizes followup TBAA because alias-set zero is the
safe and pessimistic choice.  I guess the alias sets will usually match
though.  Testing this now - hopefully the data structures reference the
correct stores and are always populated like I access them ;)

Index: gcc/dse.c
===
--- gcc/dse.c   (revision 257043)
+++ gcc/dse.c   (working copy)
@@ -2725,6 +2725,14 @@ dse_step1 (void)
"eliminated\n",
 INSN_UID (ptr->insn),
 INSN_UID (s_info->redundant_reason->insn));
+ /* If the later store we delete could have changed the
+dynamic type of the memory make sure the one we
+preserve serves as a store for all loads that could
+validly have accessed the one we delete.  */
+ store_info *r_info = s_info->redundant_reason->store_rec;
+ if (MEM_ALIAS_SET (s_info->mem)
+ != MEM_ALIAS_SET (r_info->mem))
+   set_mem_alias_set (r_info->mem, 0);
  delete_dead_store_insn (ptr);
}
  free_store_info (ptr);

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 CC||marxin at gcc dot gnu.org

--- Comment #2 from Martin Liška  ---
So running:

$ g++ pr77745.C -O2 -flto -fno-use-linker-plugin -m32 -fpic -mtune=native

for:

$ cat pr77745.C
// { dg-do run }
//
#include 
#include 

//inline void* operator new(__SIZE_TYPE__, void* __p) noexcept { return __p; }

long
__attribute__((noinline))
foo(char *c1, char *c2)
{
  long *p1 = new (c1) long;
  *p1 = 100;
  long long *p2 = new (c2) long long;
  *p2 = 200;
  long *p3 = new (c2) long;
  *p3 = 200;
  return *p1;
}
int main()
{
  union {
  char c;
  long l;
  long long ll;
  } c;
  if (foo(, ) != 200)
  {
fprintf (stderr, "aborting\n");
__builtin_abort();
  }
}

Aborts for all GCC releases I have (4.5.0+).

[Bug rtl-optimization/84003] FAIL: g++.dg/torture/pr77745.C with noinline foo

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

   Keywords||alias, wrong-code
 Status|UNCONFIRMED |NEW
   Last reconfirmed||2018-01-24
 CC||rguenth at gcc dot gnu.org
  Component|target  |rtl-optimization
Summary|FAIL:   |FAIL:
   |g++.dg/torture/pr77745.C|g++.dg/torture/pr77745.C
   |with -fpic  |with noinline foo
 Ever confirmed|0   |1

--- Comment #1 from Richard Biener  ---
Confirmed.  Goes wrong during sched2 which re-orders the read and writes in
foo:

foo (char * c1, char * c2)
{
  long int _7;

   [local count: 1073741825]:
  MEM[(long int *)c1_1(D)] = 100;
  MEM[(long long int *)c2_4(D)] = 200;
  MEM[(long int *)c2_4(D)] = 200;
  _7 = MEM[(long int *)c1_1(D)];
  return _7;

compiles to

_Z3fooPcS_:
.LFB1:
.cfi_startproc
movl4(%esp), %eax
movl8(%esp), %edx
movl$100, (%eax)
movl(%eax), %eax
movl$200, (%edx)
movl$0, 4(%edx)
ret

note c1_1 == c2_4.  This must be a latent bug in scheduling somehow not
triggered without -fpic (inlining probably).  Indeed, marking foo as
noinline makes it trigger without -fpic.