On 5/5/23 09:40, Jakub Jelinek wrote:
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);
}
From this diff it looks like the change stops looking at x.P_BOUNDS
entirely, which seems like more of an optimization than hoisting that
check out of the loop?
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:
I guess doing this would allow gnat_invariant_expr to handle
DECL_INVARIANT_P that save_expr doesn't know about. But it seems that
it makes the same assumption as tree_invariant_p_1 about the pointed-to
object not changing:
case INDIRECT_REF:
if ((!invariant_p && !TREE_READONLY (t)) || TREE_SIDE_EFFECTS (t))
return NULL_TREE;
I don't know if this assumption is any more valid in Ada than in C/C++.
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 <ja...@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
