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 <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