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
