| Message ID | Zs2ifl4ZIVxXq2SE@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] c++: Ensure ANNOTATE_EXPRs remain outermost expressions in conditions [PR116140] | expand |
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); > +}
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, > 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); >> +} >
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); > > > +} > > >
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); +}
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 (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.