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

[Jason Merrill <jason@redhat.com>]

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