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

[Jason Merrill <jason@redhat.com>]

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 languages with dynamic types.  SAVE_EXPRs are delicate
> to handle because 1) they are TREE_SIDE_EFFECTS (it's explained in save_expr)
> so out of TREE_READONLY && !TREE_SIDE_EFFECTS trees, you now get side effects
> which then propagate to all parent nodes

Hmm, interesting point.  The C++ front-end often explicitly creates a 
temporary with a TARGET_EXPR rather than SAVE_EXPR, and then later 
refers to just the variable; this avoids spreading TREE_SIDE_EFFECTS 
around, though it wasn't done for that reason.

> 2) their placement is problematic in
> conditional expressions, for example if you replace
> 
>    cond > 0 ? A : A + 1
> 
> with
> 
>    cond > 0 ? SAVE_EXPR (A) : SAVE_EXPR (A) + 1
> 
> then gimplification will, say, create the temporary and initialize it in the
> first arm so, if at runtime you take the second arm, you'll read the temporary
> uninitialized.

Absolutely, you shouldn't have the same SAVE_EXPR on two sides of a ?: 
without also evaluating it in the condition (or earlier).  But that's 
already true for cases that already aren't invariant, so I'm surprised 
that this change would cause new problems of this sort.

> That's caught for scalar values by the SSA form (if your patch
> is applied to a GCC 12 tree, you'll get ICEs in the ACATS testsuite because of
> this through finalize_type_size -> variable_size -> save_expr, it is probably
> mitigated/addressed in GCC 14 by 68e0063397ba820e71adc220b2da0581dce29ffa).
> That's also why making gnat_invariant_expr return (some of) them does not look
> really safe.
> 
> In addition to this, in Ada we have bounds of unconstrained arrays which are
> both read-only and stored indirectly, i.e. you have an INDIRECT_REF in the
> tree (it is marked TREE_THIS_NOTRAP because the bounds are always present),
> and which obviously play a crucial role in loops running over the arrays.
> This issue is responsible for the regressions in the gnat.dg testsuite.

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.

Perhaps that should be a new flag that tree_invariant_p can check 
instead of TREE_READONLY.

Jason