Bug 60183 - [4.7 Regression] phiprop creates invalid code
Summary: [4.7 Regression] phiprop creates invalid code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.8.2
: P3 normal
Target Milestone: 4.7.4
Assignee: Richard Biener
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-13 17:20 UTC by Jakub Jelinek
Modified: 2014-03-17 14:40 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work: 4.8.3, 4.9.0
Known to fail: 4.8.2
Last reconfirmed: 2014-02-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2014-02-13 17:20:56 UTC
On:

unsigned long c[32] = { 1 };

static void
foo (unsigned long *x, unsigned long *y)
{
  int i;
  unsigned long w = x[0];
  for (i = 0; i < 8; i++)
    {
      w ^= *y++;
      w += *y++;
      w ^= *y++;
      w += *y++;
    }
  x[1] = w;
}

__attribute__((noinline, noclone)) void
bar (unsigned long *x)
{
  foo (x, c);
}

int
main ()
{
  unsigned long a[2] = { 0, -1UL };
  asm volatile ("" : : "r" (c) : "memory");
  c[0] = 0;
  bar (a);
  if (a[1] != 0)
    __builtin_abort ();
  return 0;
}

at -O1 or higher phiprop causes invalid code to be generated, where the loop body reads the next *y value into a SSA_NAME and in loop preheader it reads c[0] into a SSA_NAME which is then used in a PHI on the loop header and the result of the PHI is used instead of the first *y read.  In this particular case, I don't even see any advantages of doing that, but more importantly it can read one past the end of the array.  With -O{1,2,3} -fsanitize=address
this fails loudly, otherwise if you are unlucky enough and the variable is at the end of some mmapped area, you could get a crash as well.
Comment 1 Richard Biener 2014-02-14 09:18:16 UTC
Doesn't abort for me.  phiprop does

Inserting PHI for result of load _6 = *y_5;
  for edge defining &c inserting load _19 = MEM[(long unsigned int *)&c];
  for edge defining y_13 inserting load _20 = *y_13;
_6 = PHI <_19(2), _20(3)>

on

  <bb 2>:
  w_4 = *x_2(D);
  goto <bb 4>;

  <bb 3>:
  _6 = *y_5;
  w_8 = _6 ^ w_7;
  _9 = MEM[(long unsigned int *)y_5 + 8B];
  w_10 = w_8 + _9;
  _11 = MEM[(long unsigned int *)y_5 + 16B];
  w_12 = w_10 ^ _11;
  y_13 = &MEM[(void *)y_5 + 32B];
  _14 = MEM[(long unsigned int *)y_5 + 24B];
  w_15 = w_12 + _14;
  i_17 = i_16 + 1;

  <bb 4>:
  # y_5 = PHI <&c(2), y_13(3)>

which results in

  <bb 2>:
  w_4 = *x_2(D);
  _19 = MEM[(long unsigned int *)&c];
  goto <bb 4>;

  <bb 3>:
  w_8 = _6 ^ w_7;
  _9 = MEM[(long unsigned int *)y_5 + 8B];
  w_10 = w_8 + _9;
  _11 = MEM[(long unsigned int *)y_5 + 16B];
  w_12 = w_10 ^ _11;
  y_13 = &MEM[(void *)y_5 + 32B];
  _14 = MEM[(long unsigned int *)y_5 + 24B];
  w_15 = w_12 + _14;
  i_17 = i_16 + 1;
  _20 = *y_13;

  <bb 4>:
  # y_5 = PHI <&c(2), y_13(3)>
  # _6 = PHI <_19(2), _20(3)>

I see nothing wrong with that.  Ah, but the load is conditional on i <= 7,
so we effectively speculate it.

Mine.
Comment 2 Jakub Jelinek 2014-02-14 09:22:03 UTC
Updated testcase that segfaults for me, no -fsanitize=address is then needed to trigger it.  Works with -O0 or -O? -fno-tree-phiprop:

unsigned char c[0x300001] = { 1 };
int j = 2;

static void
foo (unsigned long *x, unsigned char *y)
{
  int i;
  unsigned long w = x[0];
  for (i = 0; i < j; i++)
    {
      w += *y;
      y += 0x100000;
      w += *y;
      y += 0x100000;
    }
  x[1] = w;
}

__attribute__ ((noinline, noclone)) void
bar (unsigned long *x)
{
  foo (x, c);
}

int
main ()
{
  unsigned long a[2] = { 0, -1UL };
  asm volatile (""::"r" (c):"memory");
  c[0] = 0;
  bar (a);
  if (a[1] != 0)
    __builtin_abort ();
  return 0;
}

Perhaps phiprop is confused by the &MEM[(void *)y_5 + 2097152B] and thinks that because of the MEM_REF in there it is safe to dereference it?
Comment 3 Richard Biener 2014-02-14 09:28:16 UTC
(In reply to Jakub Jelinek from comment #2)
> Updated testcase that segfaults for me, no -fsanitize=address is then needed
> to trigger it.  Works with -O0 or -O? -fno-tree-phiprop:
> 
> unsigned char c[0x300001] = { 1 };
> int j = 2;
> 
> static void
> foo (unsigned long *x, unsigned char *y)
> {
>   int i;
>   unsigned long w = x[0];
>   for (i = 0; i < j; i++)
>     {
>       w += *y;
>       y += 0x100000;
>       w += *y;
>       y += 0x100000;
>     }
>   x[1] = w;
> }
> 
> __attribute__ ((noinline, noclone)) void
> bar (unsigned long *x)
> {
>   foo (x, c);
> }
> 
> int
> main ()
> {
>   unsigned long a[2] = { 0, -1UL };
>   asm volatile (""::"r" (c):"memory");
>   c[0] = 0;
>   bar (a);
>   if (a[1] != 0)
>     __builtin_abort ();
>   return 0;
> }
> 
> Perhaps phiprop is confused by the &MEM[(void *)y_5 + 2097152B] and thinks
> that because of the MEM_REF in there it is safe to dereference it?

It doesn't check whether it's safe to dereference because it thinks it's
dereferenced anyway.  It wasn't supposed to speculate loads.  We miss

Index: tree-ssa-phiprop.c
===================================================================
--- tree-ssa-phiprop.c  (revision 207757)
+++ tree-ssa-phiprop.c  (working copy)
@@ -309,6 +309,10 @@ propagate_with_phi (basic_block bb, gimp
       gimple def_stmt;
       tree vuse;
 
+      /* Only replace loads in the same block as the PHI node.  */
+      if (gimple_bb (use_stmt) != bb)
+       continue;
+         
       /* Check whether this is a load of *ptr.  */
       if (!(is_gimple_assign (use_stmt)
            && TREE_CODE (gimple_assign_lhs (use_stmt)) == SSA_NAME

or really a post-dominator check - but we don't compute post-dominators
and I'm not sure it would be worth doing that.
Comment 4 Richard Biener 2014-02-14 12:58:22 UTC
Ok, we do need the post-dominators to avoid

FAIL: g++.dg/tree-ssa/pr21463.C -std=gnu++98  scan-tree-dump-times phiopt1 "MIN_
EXPR" 2
FAIL: g++.dg/tree-ssa/pr21463.C -std=gnu++98  scan-tree-dump-times phiopt1 "MAX_
EXPR" 2
FAIL: g++.dg/tree-ssa/pr21463.C -std=gnu++11  scan-tree-dump-times phiopt1 "MIN_
EXPR" 2
FAIL: g++.dg/tree-ssa/pr21463.C -std=gnu++11  scan-tree-dump-times phiopt1 "MAX_
EXPR" 2
FAIL: g++.dg/tree-ssa/pr57380.C -std=gnu++98  scan-tree-dump phiopt1 "MAX_EXPR"
FAIL: g++.dg/tree-ssa/pr57380.C -std=gnu++11  scan-tree-dump phiopt1 "MAX_EXPR"

Re-testing that variant.
Comment 5 Richard Biener 2014-02-15 09:55:23 UTC
Author: rguenth
Date: Sat Feb 15 09:54:52 2014
New Revision: 207797

URL: http://gcc.gnu.org/viewcvs?rev=207797&root=gcc&view=rev
Log:
2014-02-15  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/60183
	* tree-ssa-phiprop.c (propagate_with_phi): Avoid speculating
	loads.
	(tree_ssa_phiprop): Calculate and free post-dominators.

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

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr60183.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-phiprop.c
Comment 6 Richard Biener 2014-02-17 11:11:32 UTC
Fixed on trunk sofar.
Comment 7 Richard Biener 2014-02-25 10:47:53 UTC
Author: rguenth
Date: Tue Feb 25 10:47:21 2014
New Revision: 208118

URL: http://gcc.gnu.org/viewcvs?rev=208118&root=gcc&view=rev
Log:
2014-02-25  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2014-02-21  Richard Biener  <rguenther@suse.de>

	PR middle-end/60291
	* tree-ssa-live.c (mark_all_vars_used_1): Do not walk
	DECL_INITIAL for globals not in the current function context.

	2014-02-20  Richard Biener  <rguenther@suse.de>

	PR middle-end/60221
	* tree-eh.c (execute_cleanup_eh_1): Also cleanup empty EH
	regions at -O0.

	2014-02-14  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/60183
	* tree-ssa-phiprop.c (propagate_with_phi): Avoid speculating
	loads.
	(tree_ssa_phiprop): Calculate and free post-dominators.

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

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/torture/pr60183.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_8-branch/gcc/tree-eh.c
    branches/gcc-4_8-branch/gcc/tree-ssa-live.c
    branches/gcc-4_8-branch/gcc/tree-ssa-phiprop.c
Comment 8 Richard Biener 2014-02-25 10:48:32 UTC
And for 4.8.3.
Comment 9 Richard Biener 2014-03-17 14:39:28 UTC
Author: rguenth
Date: Mon Mar 17 14:38:55 2014
New Revision: 208618

URL: http://gcc.gnu.org/viewcvs?rev=208618&root=gcc&view=rev
Log:
2014-03-17  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2013-05-21  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/57303
	* tree-ssa-sink.c (statement_sink_location): Properly handle
	self-assignments.

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

	2013-12-02  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/59139
	* tree-ssa-loop-niter.c (chain_of_csts_start): Properly match
	code in get_val_for.
	(get_val_for): Use gcc_checking_asserts.

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

	2014-02-14  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/60183
	* tree-ssa-phiprop.c (propagate_with_phi): Avoid speculating
	loads.
	(tree_ssa_phiprop): Calculate and free post-dominators.

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

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr57303.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr59139.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr60183.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_7-branch/gcc/tree-ssa-loop-niter.c
    branches/gcc-4_7-branch/gcc/tree-ssa-phiprop.c
    branches/gcc-4_7-branch/gcc/tree-ssa-sink.c
Comment 10 Richard Biener 2014-03-17 14:40:22 UTC
Fixed.