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

2023-05-30 Thread Jason Merrill via Gcc-patches
On 5/30/23 16:51, Jakub Jelinek wrote: On Tue, May 30, 2023 at 04:36:34PM -0400, Jason Merrill wrote: Note that it is fine to treat p->fld as invariant in C++ if fld is TREE_READONLY and p is itself invariant. The implementation is allowed to assume that other code didn't destroy *p and create

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

2023-05-30 Thread Jakub Jelinek via Gcc-patches
On Tue, May 30, 2023 at 04:36:34PM -0400, Jason Merrill wrote: > Note that it is fine to treat p->fld as invariant in C++ if fld is > TREE_READONLY and p is itself invariant. The implementation is allowed to > assume that other code didn't destroy *p and create a new object with a > different

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

2023-05-30 Thread Jason Merrill via Gcc-patches
On 5/30/23 04:23, Jakub Jelinek wrote: On Tue, May 30, 2023 at 10:03:05AM +0200, Eric Botcazou wrote: We want to be able to treat such things as invariant somehow even if we can't do that for references to user data that might be changed by intervening code. That is, indicate that we know that

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

2023-05-30 Thread Jakub Jelinek via Gcc-patches
On Tue, May 30, 2023 at 10:03:05AM +0200, Eric Botcazou wrote: > > We want to be able to treat such things as invariant somehow even if we > > can't do that for references to user data that might be changed by > > intervening code. > > > > That is, indicate that we know that the _REF actually

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

2023-05-30 Thread Eric Botcazou via Gcc-patches
> We want to be able to treat such things as invariant somehow even if we > can't do that for references to user data that might be changed by > intervening code. > > That is, indicate that we know that the _REF actually refers to a const > variable or is otherwise known to be unchanging. > >

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

2023-05-28 Thread Jason Merrill via Gcc-patches
On 5/13/23 06:58, Eric Botcazou wrote: I think we really need Eric (as one who e.g. introduced the DECL_INVARIANT_P apparently for this kind of stuff) to have a look at that on the Ada side. I have been investigating this for a few days and it's no small change for Ada and probably for other

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

2023-05-13 Thread Eric Botcazou via Gcc-patches
> I think we really need Eric (as one who e.g. introduced the > DECL_INVARIANT_P apparently for this kind of stuff) to have a look at that > on the Ada side. I have been investigating this for a few days and it's no small change for Ada and probably for other languages with dynamic types.

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

2023-05-09 Thread Eric Botcazou via Gcc-patches
> I think we really need Eric (as one who e.g. introduced the > DECL_INVARIANT_P apparently for this kind of stuff) to have a look at that > on the Ada side. DECL_INVARIANT_P is only set on discriminants with no default value and those are really invariant in Ada, i.e. do not change once set. >

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

2023-05-08 Thread Jakub Jelinek via Gcc-patches
On Mon, May 08, 2023 at 06:23:54AM +, Richard Biener wrote: > I wonder if we should defer some of the choices to a langhook > like make the tree_invariant_p_1 a langhook invocation with the > default to call tree_invariant_p_1. After lowering we can reset > the langhook to the default. We

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

2023-05-08 Thread Richard Biener via Gcc-patches
On Fri, 5 May 2023, Jakub Jelinek wrote: > On Fri, May 05, 2023 at 01:32:02PM -0400, Jason Merrill wrote: > > > --- gcc/ada/gcc-interface/utils2.cc.jj2023-01-16 23:19:05.539727388 > > > +0100 > > > +++ gcc/ada/gcc-interface/utils2.cc 2023-05-05 15:37:30.193990948 > > > +0200 > > > @@

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

2023-05-05 Thread Jakub Jelinek via Gcc-patches
On Fri, May 05, 2023 at 01:32:02PM -0400, Jason Merrill wrote: > > --- 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

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

2023-05-05 Thread Jason Merrill via Gcc-patches
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

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

2023-05-05 Thread Jakub Jelinek via Gcc-patches
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

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

2023-05-05 Thread Jason Merrill via Gcc-patches
On 5/5/23 06:45, Jakub Jelinek wrote: On Fri, May 05, 2023 at 11:55:41AM +0200, Jakub Jelinek via Gcc-patches wrote: Looking at the Ada cases (I admit I don't really understand why it isn't vectorized, the IL is so different from the start because of the extra SAVE_EXPRs that it is very hard to

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

2023-05-05 Thread Jakub Jelinek via Gcc-patches
On Fri, May 05, 2023 at 11:55:41AM +0200, Jakub Jelinek via Gcc-patches wrote: > Looking at the Ada cases (I admit I don't really understand why it isn't > vectorized, the IL is so different from the start because of the extra > SAVE_EXPRs that it is very hard to diff stuff), the case where

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

2023-05-05 Thread Jakub Jelinek via Gcc-patches
On Fri, May 05, 2023 at 11:04:09AM +0200, Jakub Jelinek via Gcc-patches wrote: > As mentioned in the PR, save_expr seems to be very optimistic when > some expression is invariant, which can result in various wrong-code > issues. > The problem is with the TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)

[PATCH] tree: Fix up save_expr [PR52339]

2023-05-05 Thread Jakub Jelinek via Gcc-patches
Hi! As mentioned in the PR, save_expr seems to be very optimistic when some expression is invariant, which can result in various wrong-code issues. The problem is with the TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t) case in tree_invariant_p_1. TREE_READONLY (t) in that case says that the object