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?
+ /* 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.
+#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);
+ }
