On 10/09/2024 10:29, Jason Merrill wrote:
> On 9/10/24 6:10 AM, Alex Coplan wrote:
> > 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.
>
> I still think that function would be nice to have, but the patch is OK as
> is.
Thanks, I've pushed the patch and the rest of the series as:
3fd07d4f04f libstdc++: Restore unrolling in std::find using pragma [PR116140]
9759f6299d9 lto: Stream has_unroll flag during LTO [PR116140]
31ff173c708 testsuite: Ensure ltrans dump files get cleaned up properly
[PR116140]
f97d86242b8 c++: Ensure ANNOTATE_EXPRs remain outermost expressions in
conditions [PR116140]
Alex
>
> > 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);
> > > +}
> >
>
