Message ID | ZrpMgHaHIKvaqYuS@arm.com |
---|---|
State | New |
Headers | show |
Series | [v2] c++: Ensure ANNOTATE_EXPRs remain outermost expressions in conditions [PR116140] | expand |
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); > + }
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); > > + } > >
On Fri, Aug 16, 2024 at 11:38:03AM +0100, Alex Coplan wrote: > 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). As long as we don't add new ANNOTATE_EXPR kinds with non-constant arguments, but we don't have them right now. > I think an INTEGER_CST C will always have: > > TREE_SIDE_EFFECTS (C) = TREE_READONLY (C) = 0 That is true. > > 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 No. The TREE_READONLY computation is: read_only = 1; ... if (!TREE_READONLY (arg##N) \ && !CONSTANT_CLASS_P (arg##N)) \ (void) (read_only = 0); \ While INTEGER_CST isn't TREE_READONLY, it is CONSTANT_CLASS_P. > - TREE_SIDE_EFFECTS (A) = TREE_SIDE_EFFECTS (TREE_OPERAND (A, 0)) So, unless we add non-INTEGER_CST extra arguments to ANNOTATE_EXPR, TREE_READONLY (A) = TREE_READONLY (TREE_OPERAND (A, 0)) || CONSTANT_CLASS_P (TREE_OPERAND (A, 0)); Not really sure if the first argument will ever be say INTEGER_CST, #pragma GCC unroll 8 while (1) { if (something) break; } ? Jakub
On 16/08/2024 12:47, Jakub Jelinek wrote: > On Fri, Aug 16, 2024 at 11:38:03AM +0100, Alex Coplan wrote: > > 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). > > As long as we don't add new ANNOTATE_EXPR kinds with non-constant arguments, > but we don't have them right now. > > > I think an INTEGER_CST C will always have: > > > > TREE_SIDE_EFFECTS (C) = TREE_READONLY (C) = 0 > > That is true. > > > > 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 > > No. The TREE_READONLY computation is: > read_only = 1; > ... > if (!TREE_READONLY (arg##N) \ > && !CONSTANT_CLASS_P (arg##N)) \ > (void) (read_only = 0); \ > While INTEGER_CST isn't TREE_READONLY, it is CONSTANT_CLASS_P. > > > - TREE_SIDE_EFFECTS (A) = TREE_SIDE_EFFECTS (TREE_OPERAND (A, 0)) > > So, unless we add non-INTEGER_CST extra arguments to ANNOTATE_EXPR, > TREE_READONLY (A) = TREE_READONLY (TREE_OPERAND (A, 0)) > || CONSTANT_CLASS_P (TREE_OPERAND (A, 0)); Ah, right. I was going off memory of what we discussed so far and didn't look at what PROCESS_ARG actually does. Thanks. In any case, this avoids the need for the checking in the change of direction of the flags (although perhaps pushes the problem elsewhere in that now we arguably need to check that operands 1 and 2 of each ANNOTATE_EXPR is an INTEGER_CST, unless we want to just rely on that assumption unchecked). > Not really sure if the first argument will ever be say INTEGER_CST, > #pragma GCC unroll 8 > while (1) > { > if (something) > break; > } > ? > > Jakub >
On 8/16/24 7:20 AM, Alex Coplan wrote: > On 16/08/2024 12:47, Jakub Jelinek wrote: >> On Fri, Aug 16, 2024 at 11:38:03AM +0100, Alex Coplan wrote: >>> 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). >> >> As long as we don't add new ANNOTATE_EXPR kinds with non-constant arguments, >> but we don't have them right now. >> >>> I think an INTEGER_CST C will always have: >>> >>> TREE_SIDE_EFFECTS (C) = TREE_READONLY (C) = 0 >> >> That is true. >>> >>> 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 >> >> No. The TREE_READONLY computation is: >> read_only = 1; >> ... >> if (!TREE_READONLY (arg##N) \ >> && !CONSTANT_CLASS_P (arg##N)) \ >> (void) (read_only = 0); \ >> While INTEGER_CST isn't TREE_READONLY, it is CONSTANT_CLASS_P. >> >>> - TREE_SIDE_EFFECTS (A) = TREE_SIDE_EFFECTS (TREE_OPERAND (A, 0)) >> >> So, unless we add non-INTEGER_CST extra arguments to ANNOTATE_EXPR, >> TREE_READONLY (A) = TREE_READONLY (TREE_OPERAND (A, 0)) >> || CONSTANT_CLASS_P (TREE_OPERAND (A, 0)); > > Ah, right. I was going off memory of what we discussed so far and didn't look > at what PROCESS_ARG actually does. Thanks. > > In any case, this avoids the need for the checking in the change of > direction of the flags (although perhaps pushes the problem elsewhere in > that now we arguably need to check that operands 1 and 2 of each > ANNOTATE_EXPR is an INTEGER_CST, unless we want to just rely on that > assumption unchecked). Perhaps we want a recompute_expr_flags like the existing recompute_constructor_flags, so we don't need to duplicate PROCESS_ARG logic elsewhere. Jason
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index e58612660c9..e128061e93d 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -966,25 +966,62 @@ maybe_convert_cond (tree cond) if (type_dependent_expression_p (cond)) return cond; + /* If the condition has any ANNOTATE_EXPRs, those must remain the outermost + expressions of the condition. Walk through these, performing the condition + conversion in place on the inner expression. */ + tree *condp = &cond; + while (TREE_CODE (*condp) == ANNOTATE_EXPR) + condp = &TREE_OPERAND (*condp, 0); + + tree orig_inner = *condp; + /* For structured binding used in condition, the conversion needs to be evaluated before the individual variables are initialized in the std::tuple_{size,elemenet} case. cp_finish_decomp saved the conversion result in a TARGET_EXPR, pick it up from there. */ - if (DECL_DECOMPOSITION_P (cond) - && DECL_DECOMP_IS_BASE (cond) - && DECL_DECOMP_BASE (cond) - && TREE_CODE (DECL_DECOMP_BASE (cond)) == TARGET_EXPR) - cond = TARGET_EXPR_SLOT (DECL_DECOMP_BASE (cond)); + if (DECL_DECOMPOSITION_P (*condp) + && DECL_DECOMP_IS_BASE (*condp) + && DECL_DECOMP_BASE (*condp) + && TREE_CODE (DECL_DECOMP_BASE (*condp)) == TARGET_EXPR) + *condp = TARGET_EXPR_SLOT (DECL_DECOMP_BASE (*condp)); if (warn_sequence_point && !processing_template_decl) - verify_sequence_points (cond); + verify_sequence_points (*condp); - maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, + maybe_warn_unparenthesized_assignment (*condp, /*nested_p=*/false, tf_warning_or_error); /* Do the conversion. */ - cond = convert_from_reference (cond); - return condition_conversion (cond); + *condp = convert_from_reference (*condp); + *condp = condition_conversion (*condp); + + /* Either of the above two calls could have changed the type of *CONDP, in + which case the type (and possibly the flags) of any outer ANNOTATE_EXPRs + need updating. */ + if (TREE_TYPE (cond) != TREE_TYPE (*condp)) + { + /* If the type of *CONDP changed (e.g. due to convert_from_reference) then + 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. */ +#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); + } + } + + return cond; } /* Finish an expression-statement, whose EXPRESSION is as indicated. */ diff --git a/gcc/testsuite/g++.dg/ext/pragma-unroll-lambda.C b/gcc/testsuite/g++.dg/ext/pragma-unroll-lambda.C new file mode 100644 index 00000000000..f410f6d8d25 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/pragma-unroll-lambda.C @@ -0,0 +1,17 @@ +// { dg-do compile { target c++11 } } + +template<typename Iter, typename Pred> +inline Iter +my_find(Iter first, Iter last, Pred pred) +{ +#pragma GCC unroll 4 + while (first != last && !pred(*first)) + ++first; + return first; +} + +short *use_find(short *p) +{ + auto pred = [](short x) { return x == 42; }; + return my_find(p, p + 1024, pred); +}