On 15/08/2024 16:55, Jason Merrill wrote:
> On 8/12/24 1:55 PM, Alex Coplan wrote:
> > Hi!
> >
> > This is a v2 patch of:
> > https://gcc.gnu.org/pipermail/gcc-patches/2024-August/659968.html
> > that addresses Jakub's feedback.
> >
> > FWIW, I tried to contrive a testcase where convert_from_reference kicks
> > in and we get called with an ANNOTATE_EXPR in maybe_convert_cond, but to
> > no avail.
>
> Yes, the convert_from_reference shouldn't have any effect here, that should
> have happened already when processing the condition expression.
>
> > However, I did see cases (both in hand-written testcases and
> > in the testsuite, e.g. g++.dg/ext/pr114409-2.C) where the subsequent
> > call to condition_conversion would change the type (e.g. from int to
> > bool), which shows the need for updating the types in the ANNOTATE_EXPR
> > chain -- thanks for pointing that out, Jakub!
> >
> > Personally, I feel the handling of the flags (in this patch, as per
> > Jakub's suggestion) is a bit of a premature optimization. It seems
> > cleaner (and safer) to me just to re-build the annotations if needed
> > (i.e. in the case that the type changed). You could even have a nice
> > abstraction that encapsulates the stripping and re-building of
> > ANNOTATE_EXPRs, so that it doesn't clutter the caller quite so much.
>
> I'm sympathetic that the optimization is not very significant, but neither
> is updating the flags. You could also factor it out for the same less
> clutter in the caller?
Good point, I'll see if I can't factor things out with the in-place update
approach.
>
> > + /* If the type of *CONDP changed (e.g. due to
> > convert_from_reference) then
>
> As discussed, this is much more likely to be from condition_conversion.
>
> > + the flags may have changed too. The logic in the loop below relies on
> > + the flags only being changed in the following directions (if at all):
> > + TREE_SIDE_EFFECTS : 0 -> 1
> > + TREE_READONLY : 1 -> 0
> > + thus avoiding re-computing the flags from scratch (e.g. via build3), so
> > + let's verify that this precondition holds. */
>
> Is there any case where an ANNOTATE_EXPR can have different
> READONLY/SIDE_EFFECTS flags from its operand? It would be simpler to just
> copy the flags and not bother with the checking.
Looking at the calls to build3 (ANNOTATE_EXPR, ...) in cp/semantics.cc,
it looks like the other two operands of ANNOTATE_EXPRs are only ever
INTEGER_CSTs (the code in tree-cfg.cc:replace_loop_annotate_in_block
corroborates this).
I think an INTEGER_CST C will always have:
TREE_SIDE_EFFECTS (C) = TREE_READONLY (C) = 0
and since the TREE_READONLY flags are conjunctive and TREE_SIDE_EFFECTS
flags are disjunctive, for an ANNOTATE_EXPR A we will necessarily have:
- TREE_READONLY (A) = 0
- TREE_SIDE_EFFECTS (A) = TREE_SIDE_EFFECTS (TREE_OPERAND (A, 0))
so indeed I think this can be simplified significantly, since the above
means we needn't update TREE_READONLY, and TREE_SIDE_EFFECTS can be set
to that of the updated operand (without the checking).
I'll adjust the patch to account for this and try to factor things out
as suggested above.
Thanks a lot for the review.
Alex
>
> > +#define CHECK_FLAG_CHANGE(prop, value)\
> > + gcc_checking_assert (prop (orig_inner) == prop (*condp) || prop
> > (*condp) == value)
> > + CHECK_FLAG_CHANGE (TREE_SIDE_EFFECTS, 1);
> > + CHECK_FLAG_CHANGE (TREE_READONLY, 0);
> > +#undef CHECK_FLAG_CHANGE
> > + for (tree c = cond; c != *condp; c = TREE_OPERAND (c, 0))
> > + {
> > + gcc_checking_assert (TREE_CODE (c) == ANNOTATE_EXPR);
> > + TREE_TYPE (c) = TREE_TYPE (*condp);
> > + TREE_SIDE_EFFECTS (c) |= TREE_SIDE_EFFECTS (*condp);
> > + TREE_READONLY (c) &= TREE_READONLY (*condp);
> > + }
>
>
