| 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); +}
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. 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. However, I understand the desire to avoid re-allocating here (even if this should be a fairly uncommon case), hence this patch goes with the approach suggested by Jakub. I'm happy to be persuaded to do otherwise by C++ maintainers :) Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk? Thanks, Alex -- >8 -- For the testcase added with this patch, we would end up losing the: #pragma GCC unroll 4 and emitting "warning: ignoring loop annotation". That warning comes from tree-cfg.cc:replace_loop_annotate, and means that we failed to process the ANNOTATE_EXPR in tree-cfg.cc:replace_loop_annotate_in_block. That function walks backwards over the GIMPLE in an exiting BB for a loop, skipping over the final gcond, and looks for any ANNOTATE_EXPRS immediately preceding the gcond. The function documents the following pre-condition: /* [...] We assume that the annotations come immediately before the condition in BB, if any. */ now looking at the exiting BB of the loop, we have: <bb 8> : D.4524 = .ANNOTATE (iftmp.1, 1, 4); retval.0 = D.4524; if (retval.0 != 0) goto <bb 3>; [INV] else goto <bb 9>; [INV] and crucially there is an intervening assignment between the gcond and the preceding .ANNOTATE ifn call. To see where this comes from, we can look to the IR given by -fdump-tree-original: if (<<cleanup_point ANNOTATE_EXPR <first != last && !use_find(short int*)::<lambda(short int)>::operator() (&pred, *first), unroll 4>>>) goto <D.4518>; else goto <D.4516>; here the problem is that we've wrapped a CLEANUP_POINT_EXPR around the ANNOTATE_EXPR, meaning the ANNOTATE_EXPR is no longer the outermost expression in the condition. The CLEANUP_POINT_EXPR gets added by the following call chain: finish_while_stmt_cond -> maybe_convert_cond -> condition_conversion -> fold_build_cleanup_point_expr this patch chooses to fix the issue in maybe_convert_cond by walking through any ANNOTATE_EXPRs and doing any condition conversion on the inner expression, leaving the ANNOTATE_EXPRs (if any) as the outermost expressions in the condition. With this patch, we don't get any such warning and the loop gets unrolled as expected at -O2. gcc/cp/ChangeLog: PR libstdc++/116140 * semantics.cc (maybe_convert_cond): Ensure any ANNOTATE_EXPRs remain the outermost expression(s) of the condition. gcc/testsuite/ChangeLog: PR libstdc++/116140 * g++.dg/ext/pragma-unroll-lambda.C: New test. Co-Authored-By: Jakub Jelinek <jakub@redhat.com>