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 <ja...@redhat.com>
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);
+}
