On 27/08/2024 10:55, Alex Coplan wrote: > Hi, > > This is a v3 that hopefully addresses the feedback from both Jason and > Jakub. v2 was posted here: > https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660191.html
Gentle ping on this C++ patch: https://gcc.gnu.org/pipermail/gcc-patches/2024-August/661559.html Jason, are you OK with this approach, or would you prefer to not make the INTEGER_CST assumption and do something along the lines of your last suggestion instead: > Perhaps we want a recompute_expr_flags like the existing > recompute_constructor_flags, so we don't need to duplicate PROCESS_ARG > logic elsewhere. ? Sorry, I'd missed that reply when I wrote the v3 patch. Thanks, Alex > > (Sorry for the delay in posting the re-spin, I was away last week.) > > In this version we refactor to introudce a helper class (annotate_saver) > which is much less invasive to the caller (maybe_convert_cond) and > should (at least in theory) be reuseable elsewhere. > > This version also relies on the assumption that operands 1 and 2 of > ANNOTATE_EXPRs are INTEGER_CSTs, which simplifies the flag updates > without having to rely on assumptions about the specific changes made > in maybe_convert_cond. > > 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 by first introducing a new helper > class (annotate_saver) to save and restore outer chains of > ANNOTATE_EXPRs and then using it in maybe_convert_cond. > > 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 (anotate_saver): New. Use it ... > (maybe_convert_cond): ... here, to 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. > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 5ab2076b673..b1a49b14238 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -951,6 +951,86 @@ maybe_warn_unparenthesized_assignment (tree t, bool > nested_p, > } > } > > +/* Helper class for saving/restoring ANNOTATE_EXPRs. For a tree node t, > users > + can construct one of these like so: > + > + annotate_saver s (&t); > + > + and t will be updated to have any annotations removed. The user can then > + transform t, and later restore the ANNOTATE_EXPRs with: > + > + t = s.restore (t). > + > + The intent is to ensure that any ANNOTATE_EXPRs remain the outermost > + expressions following any operations on t. */ > + > +class annotate_saver { > + /* The chain of saved annotations, if there were any. Otherwise null. */ > + tree m_annotations; > + > + /* If M_ANNOTATIONS is non-null, then M_INNER points to TREE_OPERAND (A, 0) > + for the innermost annotation A. */ > + tree *m_inner; > + > +public: > + annotate_saver (tree *); > + tree restore (tree); > +}; > + > +/* If *COND is an ANNOTATE_EXPR, walk through the chain of annotations, and > set > + *COND equal to the first non-ANNOTATE_EXPR (saving a pointer to the > + original chain of annotations for later use in restore). */ > + > +annotate_saver::annotate_saver (tree *cond) : m_annotations (nullptr) > +{ > + tree *t = cond; > + while (TREE_CODE (*t) == ANNOTATE_EXPR) > + t = &TREE_OPERAND (*t, 0); > + > + if (t != cond) > + { > + m_annotations = *cond; > + *cond = *t; > + m_inner = t; > + } > +} > + > +/* If we didn't strip any annotations on construction, return NEW_INNER > + unmodified. Otherwise, wrap the saved annotations around NEW_INNER > (updating > + the types and flags of the annotations if needed) and return the resulting > + expression. */ > + > +tree > +annotate_saver::restore (tree new_inner) > +{ > + if (!m_annotations) > + return new_inner; > + > + /* If the type of the inner expression changed, we need to update the types > + of all the ANNOTATE_EXPRs. We may need to update the flags too, but we > + assume they only change if the type of the inner expression changes. > + The flag update logic assumes that the other operands to the > + ANNOTATE_EXPRs are always INTEGER_CSTs. */ > + if (TREE_TYPE (new_inner) != TREE_TYPE (*m_inner)) > + { > + const bool new_readonly > + = TREE_READONLY (new_inner) || CONSTANT_CLASS_P (new_inner); > + > + for (tree c = m_annotations; c != *m_inner; c = TREE_OPERAND (c, 0)) > + { > + gcc_checking_assert (TREE_CODE (c) == ANNOTATE_EXPR > + && TREE_CODE (TREE_OPERAND (c, 1)) == INTEGER_CST > + && TREE_CODE (TREE_OPERAND (c, 2)) == > INTEGER_CST); > + TREE_TYPE (c) = TREE_TYPE (new_inner); > + TREE_SIDE_EFFECTS (c) = TREE_SIDE_EFFECTS (new_inner); > + TREE_READONLY (c) = new_readonly; > + } > + } > + > + *m_inner = new_inner; > + return m_annotations; > +} > + > /* COND is the condition-expression for an if, while, etc., > statement. Convert it to a boolean value, if appropriate. > In addition, verify sequence points if -Wsequence-point is enabled. */ > @@ -966,6 +1046,9 @@ maybe_convert_cond (tree cond) > if (type_dependent_expression_p (cond)) > return cond; > > + /* Strip any ANNOTATE_EXPRs from COND. */ > + annotate_saver annotations (&cond); > + > /* 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 > @@ -984,7 +1067,10 @@ maybe_convert_cond (tree cond) > > /* Do the conversion. */ > cond = convert_from_reference (cond); > - return condition_conversion (cond); > + cond = condition_conversion (cond); > + > + /* Restore any ANNOTATE_EXPRs around COND. */ > + return annotations.restore (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); > +}
