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

[Jakub Jelinek <jakub@redhat.com>]

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)
> case in tree_invariant_p_1.  TREE_READONLY (t) in that case says
> that the object shouldn't be modified during its lifetime and
> !TREE_SIDE_EFFECTS (t) that it can be evaluated safely multiple times,
> but that doesn't mean we can avoid wrapping the expression into SAVE_EXPR
> say for a TREE_READONLY COMPONENT_REF with INDIRECT_REF as first operand
> - either the lifetime of the TREE_READONLY object could end earlier than
> when we need to reevaluate the object (that happens in the
> pr52339-1.c case where save_expr is called on p->a and then free (p) is
> done or pr52339.C where delete a->b when calling ~B () dtor deallocates a),
> or e.g. the pointer could change as in pr52339-2.c (so evaluating p->a again
> after ++p yields a possibly different value than originally and again we need
> a SAVE_EXPR).
> 
> Attached are two patches which fix this, unfortunately both regress
> FAIL: gnat.dg/loop_optimization21.adb scan-tree-dump-times optimized "Index_Check" 1
> FAIL: gnat.dg/vect1.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> FAIL: gnat.dg/vect2.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> FAIL: gnat.dg/vect3.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> FAIL: gnat.dg/vect4.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> FAIL: gnat.dg/vect5.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> FAIL: gnat.dg/vect6.adb scan-tree-dump-times vect "vectorized 1 loops" 15
> on x86_64-linux (the first scan triggers 2 times rather than once,
> the next 3 13 times rather than 15 and the last 3 14 times rather than 15
> times).
> The first patch has been otherwise successfully bootstrapped/regtested on
> x86_64-linux and i686-linux (with that above regressions), the second one
> is probably better but has been so far tested just on the new testcases and
> verified to also cause the above Ada regressions.

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 save_expr
used to return the argument and no longer does are those
r.P_BOUNDS->LB0
etc. cases.  Now, I wondered if (pre-gimplification) we couldn't make an
exception and allow the base to be INDIRECT_REF or of a REFERENCE_TYPE
with the idea that references are really imutable and can't be changed
during its lifetime (after gimplification whether something is
REFERENCE_TYPE or POINTER_TYPE is lost), but that isn't what Ada is using.

So, another possibility would be to allow bases of TREE_READONLY (t) &&
!TREE_SIDE_EFFECTS (t) which are INDIRECT_REFs of tree_invariant_p_1
addresses.  That doesn't work either, in the r.P_BOUNDS->LB0 case
P_BOUNDS is a FIELD_DECL with POINTER_TYPE, LB0 is TREE_READONLY FIELD_DECL
and that COMPONENT_REF is  also TREE_READONLY, r is TREE_READONLY PARM_DECL,
but unforuntately the r.P_BOUNDS COMPONENT_REF isn't marked TREE_READONLY.

Thus, shall we treat as tree_invariant_p_1 also handled components which
are !TREE_SIDE_EFFECTS (t), but not TREE_READONLY and only their base
is TREE_READONLY?  Or do that only during the recursion?

	Jakub