Re: [PATCH] tree: Fix up save_expr [PR52339]

[Jakub Jelinek <jakub@redhat.com>]

On Fri, May 05, 2023 at 07:38:45AM -0400, Jason Merrill wrote:
> On 5/5/23 06:45, Jakub Jelinek wrote:
> > +  if (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
> > +    {
> > +      /* Return true for const qualified vars, but for members or array
> > +	 elements without side-effects return true only if the base
> > +	 object is a decl.  If the base is e.g. a pointer dereference,
> > +	 what the pointer points to could be deallocated or the pointer
> > +	 could be changed.  See PR52339.  */
> > +      tree base = get_base_address (t);
> > +      if (DECL_P (base))
> > +	return true;
> 
> So I think the above is correct.

Ok, will test it with testsuite adjustments for the Ada testcases.
See below.

> > +      /* As an exception, allow pointer dereferences as long as the pointer
> > +	 is invariant.  */
> > +      if (TREE_CODE (base) == INDIRECT_REF
> > +	  && tree_invariant_p_1 (get_base_address (TREE_OPERAND (base, 0))))
> > +	return true;
> 
> And this is unsafe.

Ok, idea withdrawn.

Had further look at the vect6.adb case, but I think it is for the Ada people
to analyze.

The *.original dump differences there are as I said instead of using
r.P_BOUNDS->LB0
r.P_BOUNDS->UB0
x.P_BOUNDS->LB0
x.P_BOUNDS->UB0
wrap those into SAVE_EXPR in various places (that is the expected part,
that is what the patch was about), but also:
-        SAVE_EXPR <x.P_BOUNDS->LB0 < r.P_BOUNDS->LB0 || x.P_BOUNDS->UB0 > r.P_BOUNDS->UB0>;
         <<< Unknown tree: loop_stmt
           I.0 != (unsigned long) vect6__add__L_1__T3b___U
           I.0 = I.0 + 1;
           i = (vect6_pkg__index_type) I.0;
-          if ((SAVE_EXPR <x.P_BOUNDS->LB0 < r.P_BOUNDS->LB0 || x.P_BOUNDS->UB0 > r.P_BOUNDS->UB0>) && .BUILTIN_EXPECT (r.P_BOUNDS->LB0 > i || r.P_BOUNDS->UB0 < i, 0, 15))
+          if (SAVE_EXPR <r.P_BOUNDS->LB0> > i || SAVE_EXPR <r.P_BOUNDS->UB0> < i)
             {
               .gnat_rcheck_CE_Index_Check ("vect6.adb", 9);
             }
So, if the {x,r}.P_BOUNDS->{U,B}B0 expressions aren't wrapped into
SAVE_EXPRs, something in the FE decides to evaluate
x.P_BOUNDS->LB0 < r.P_BOUNDS->LB0 || x.P_BOUNDS->UB0 > r.P_BOUNDS->UB0
expression into a temporary before the loop and && the bounds condition
inside of the loop with it, while with the patch that doesn't happen.
And, that turns out in loop unswitching being successful without my patch
and not with my patch, where we can vectorize the unswitched loop without
the .gnat_rcheck_CE_Index_Check call.

Perhaps ada/gcc-interface/utils2.cc (gnat_invariant_expr) could be taught
to handle SAVE_EXPR by looking at its operand?
--- gcc/ada/gcc-interface/utils2.cc.jj	2023-01-16 23:19:05.539727388 +0100
+++ gcc/ada/gcc-interface/utils2.cc	2023-05-05 15:37:30.193990948 +0200
@@ -3332,6 +3332,7 @@ gnat_invariant_expr (tree expr)
 	case IMAGPART_EXPR:
 	case VIEW_CONVERT_EXPR:
 	CASE_CONVERT:
+	case SAVE_EXPR:
 	  break;
 
 	case INDIRECT_REF:
fixes the vect{1,2,3,4,5,6}.adb regressions but not the
loop_optimization21.adb one.  But I'm afraid I really have no idea what
that code is doing.

2023-05-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/52339
	* tree.cc (tree_invariant_p_1): For TREE_READONLY (t) without
	side-effects, only return true if DECL_P (get_base_address (t)).

	* g++.dg/opt/pr52339.C: New test.
	* gcc.c-torture/execute/pr52339-1.c: New test.
	* gcc.c-torture/execute/pr52339-2.c: New test.
	* gnat.dg/loop_optimization21.adb: Adjust expected match count.
	* gnat.dg/vect1.adb: Likewise.
	* gnat.dg/vect2.adb: Likewise.
	* gnat.dg/vect3.adb: Likewise.
	* gnat.dg/vect4.adb: Likewise.
	* gnat.dg/vect5.adb: Likewise.
	* gnat.dg/vect6.adb: Likewise.

--- gcc/tree.cc.jj	2023-05-01 09:59:46.686293833 +0200
+++ gcc/tree.cc	2023-05-05 10:19:19.061827355 +0200
@@ -3876,10 +3876,21 @@ tree_invariant_p_1 (tree t)
 {
   tree op;
 
-  if (TREE_CONSTANT (t)
-      || (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)))
+  if (TREE_CONSTANT (t))
     return true;
 
+  if (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
+    {
+      /* Return true for const qualified vars, but for members or array
+	 elements without side-effects return true only if the base
+	 object is a decl.  If the base is e.g. a pointer dereference,
+	 what the pointer points to could be deallocated or the pointer
+	 could be changed.  See PR52339.  */
+      tree base = get_base_address (t);
+      if (DECL_P (base))
+	return true;
+    }
+
   switch (TREE_CODE (t))
     {
     case SAVE_EXPR:
--- gcc/testsuite/g++.dg/opt/pr52339.C.jj	2023-05-04 15:23:20.459935705 +0200
+++ gcc/testsuite/g++.dg/opt/pr52339.C	2023-05-04 15:22:35.640578681 +0200
@@ -0,0 +1,19 @@
+// PR c++/52339
+// { dg-do run { target c++11 } }
+
+
+struct B;
+struct A { B *b; };
+struct B {
+  A *a;
+  B () : a(new A{this}) {}
+  ~B () { delete a; }
+};
+ 
+int
+main ()
+{
+  B *b = new B;
+  const A *a = b->a;
+  delete a->b;
+}
--- gcc/testsuite/gcc.c-torture/execute/pr52339-1.c.jj	2023-05-04 15:22:59.177241023 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr52339-1.c	2023-05-04 15:20:19.820527142 +0200
@@ -0,0 +1,29 @@
+/* PR c++/52339 */
+
+struct S { int a; };
+
+void
+bar (int *p, struct S *q)
+{
+  __builtin_free (q);
+}
+
+int
+foo (const struct S *p, struct S *q)
+{
+  int b[p->a];
+  bar (b, q);
+  return sizeof (b);
+}
+
+int
+main ()
+{
+  struct S *p = __builtin_malloc (sizeof (struct S));
+  if (!p)
+    return 0;
+  p->a = 42;
+  if (foo (p, p) != 42 * sizeof (int))
+    __builtin_abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.c-torture/execute/pr52339-2.c.jj	2022-11-21 10:04:00.210677046 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr52339-2.c	2023-05-04 19:34:08.581686806 +0200
@@ -0,0 +1,20 @@
+/* PR c++/52339 */
+
+struct S { int a; };
+
+int
+foo (const struct S *p)
+{
+  int b[p->a];
+  ++p;
+  return sizeof (b);
+}
+
+int
+main ()
+{
+  struct S s[] = { { 42 }, { 43 } };
+  if (foo (s) != 42 * sizeof (int))
+    __builtin_abort ();
+  return 0;
+}
--- gcc/testsuite/gnat.dg/loop_optimization21.adb.jj	2020-01-14 20:02:48.316586870 +0100
+++ gcc/testsuite/gnat.dg/loop_optimization21.adb	2023-05-05 15:26:47.908115394 +0200
@@ -17,4 +17,4 @@ package body Loop_Optimization21 is
 
 end Loop_Optimization21;
 
--- { dg-final { scan-tree-dump-times "Index_Check" 1 "optimized" } }
+-- { dg-final { scan-tree-dump-times "Index_Check" 2 "optimized" } }
--- gcc/testsuite/gnat.dg/vect1.adb.jj	2020-01-14 20:02:48.345586436 +0100
+++ gcc/testsuite/gnat.dg/vect1.adb	2023-05-05 15:27:15.012730339 +0200
@@ -124,4 +124,4 @@ package body Vect1 is
 
 end Vect1;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 13 "vect"  } }
--- gcc/testsuite/gnat.dg/vect2.adb.jj	2020-01-14 20:02:48.345586436 +0100
+++ gcc/testsuite/gnat.dg/vect2.adb	2023-05-05 15:27:24.393597068 +0200
@@ -124,4 +124,4 @@ package body Vect2 is
 
 end Vect2;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 13 "vect"  } }
--- gcc/testsuite/gnat.dg/vect3.adb.jj	2020-01-14 20:02:48.345586436 +0100
+++ gcc/testsuite/gnat.dg/vect3.adb	2023-05-05 15:27:29.166529269 +0200
@@ -124,4 +124,4 @@ package body Vect3 is
 
 end Vect3;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 13 "vect"  } }
--- gcc/testsuite/gnat.dg/vect4.adb.jj	2020-01-14 20:02:48.345586436 +0100
+++ gcc/testsuite/gnat.dg/vect4.adb	2023-05-05 15:27:39.016389337 +0200
@@ -124,4 +124,4 @@ package body Vect4 is
 
 end Vect4;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 14 "vect"  } }
--- gcc/testsuite/gnat.dg/vect5.adb.jj	2020-01-14 20:02:48.346586421 +0100
+++ gcc/testsuite/gnat.dg/vect5.adb	2023-05-05 15:27:41.941347785 +0200
@@ -124,4 +124,4 @@ package body Vect5 is
 
 end Vect5;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 14 "vect"  } }
--- gcc/testsuite/gnat.dg/vect6.adb.jj	2020-01-14 20:02:48.346586421 +0100
+++ gcc/testsuite/gnat.dg/vect6.adb	2023-05-05 15:27:45.053303574 +0200
@@ -124,4 +124,4 @@ package body Vect6 is
 
 end Vect6;
 
--- { dg-final { scan-tree-dump-times "vectorized 1 loops" 15 "vect"  } }
+-- { dg-final { scan-tree-dump-times "vectorized 1 loops" 14 "vect"  } }


	Jakub