diff mbox series

[1/5] cp: Ensure ANNOTATE_EXPRs remain outermost expressions in conditions [PR116140]

Message ID ZrXguXMct7f30WN2@arm.com
State New
Headers show
Series Address std::find regression with RTL unrolling [PR116140] | expand

Commit Message

Alex Coplan Aug. 9, 2024, 9:26 a.m. UTC 
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.

Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?

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.
---
 gcc/cp/semantics.cc                           | 26 ++++++++++++-------
 .../g++.dg/ext/pragma-unroll-lambda.C         | 17 ++++++++++++
 2 files changed, 34 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ext/pragma-unroll-lambda.C

Comments

Jason Merrill Aug. 9, 2024, 3:01 p.m. UTC | #1 
On 8/9/24 5:26 AM, Alex Coplan wrote:
> 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.

I see that simplify_loop_decl_cond and finish_loop_cond use this same 
pattern.  OK.

> With this patch, we don't get any such warning and the loop gets unrolled as
> expected at -O2.
> 
> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
> 
> 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.
> ---
>   gcc/cp/semantics.cc                           | 26 ++++++++++++-------
>   .../g++.dg/ext/pragma-unroll-lambda.C         | 17 ++++++++++++
>   2 files changed, 34 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/ext/pragma-unroll-lambda.C
>
Jakub Jelinek Aug. 9, 2024, 3:08 p.m. UTC | #2 
On Fri, Aug 09, 2024 at 11:01:22AM -0400, Jason Merrill wrote:
> > 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.
> 
> I see that simplify_loop_decl_cond and finish_loop_cond use this same
> pattern.  OK.

I'm a little bit worried about the convert_from_reference in there.
Shouldn't TREE_TYPE of ANNOTATE_EXPR always match the TREE_TYPE of its
operand?  If it could be say REFERENCE_TYPE to BOOLEAN_TYPE,
convert_from_reference could change it but with the patch fail to adjust
the type.

	Jakub
Alex Coplan Aug. 9, 2024, 3:21 p.m. UTC | #3 
On 09/08/2024 17:08, Jakub Jelinek wrote:
> On Fri, Aug 09, 2024 at 11:01:22AM -0400, Jason Merrill wrote:
> > > 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.
> > 
> > I see that simplify_loop_decl_cond and finish_loop_cond use this same
> > pattern.  OK.
> 
> I'm a little bit worried about the convert_from_reference in there.
> Shouldn't TREE_TYPE of ANNOTATE_EXPR always match the TREE_TYPE of its
> operand?  If it could be say REFERENCE_TYPE to BOOLEAN_TYPE,
> convert_from_reference could change it but with the patch fail to adjust
> the type.

Hmm, good spot, I didn't realise that convert_from_reference could
change the type of the condition like this.

In that case I suppose the only thing to do is to construct a stack of
ANNOTATE_EXPRs on the way down and re-build the expressions (taking the
type of the inner expression, starting with the cond) on the way back
up.

I'll try something along those lines, then (unless there are further
comments suggesting otherwise!)

Thanks,
Alex

> 
> 	Jakub
>
Jakub Jelinek Aug. 9, 2024, 3:34 p.m. UTC | #4 
On Fri, Aug 09, 2024 at 04:21:14PM +0100, Alex Coplan wrote:
> Hmm, good spot, I didn't realise that convert_from_reference could
> change the type of the condition like this.
> 
> In that case I suppose the only thing to do is to construct a stack of
> ANNOTATE_EXPRs on the way down and re-build the expressions (taking the
> type of the inner expression, starting with the cond) on the way back
> up.

I think you don't need to rebuild them, just update their types.
Something along the lines of
  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);
   }

	Jakub
Alex Coplan Aug. 9, 2024, 3:46 p.m. UTC | #5 
On 09/08/2024 17:34, Jakub Jelinek wrote:
> On Fri, Aug 09, 2024 at 04:21:14PM +0100, Alex Coplan wrote:
> > Hmm, good spot, I didn't realise that convert_from_reference could
> > change the type of the condition like this.
> > 
> > In that case I suppose the only thing to do is to construct a stack of
> > ANNOTATE_EXPRs on the way down and re-build the expressions (taking the
> > type of the inner expression, starting with the cond) on the way back
> > up.
> 
> I think you don't need to rebuild them, just update their types.
> Something along the lines of
>   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);
>    }

I suppose I was just concerned about any other properties of
ANNOTATE_EXPRs that might be inherited from the operand (that could be
affected by such a change).

It looks like TREE_{READONLY,THIS_VOLATILE,SIDE_EFFECTS} are all set
in convert_from_reference and propagated by build3, so if those change
underneath us then only updating the type seems insufficient.

> 
> 	Jakub
>
Jakub Jelinek Aug. 9, 2024, 3:56 p.m. UTC | #6 
On Fri, Aug 09, 2024 at 04:46:55PM +0100, Alex Coplan wrote:
> On 09/08/2024 17:34, Jakub Jelinek wrote:
> > On Fri, Aug 09, 2024 at 04:21:14PM +0100, Alex Coplan wrote:
> > > Hmm, good spot, I didn't realise that convert_from_reference could
> > > change the type of the condition like this.
> > > 
> > > In that case I suppose the only thing to do is to construct a stack of
> > > ANNOTATE_EXPRs on the way down and re-build the expressions (taking the
> > > type of the inner expression, starting with the cond) on the way back
> > > up.
> > 
> > I think you don't need to rebuild them, just update their types.
> > Something along the lines of
> >   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);
> >    }
> 
> I suppose I was just concerned about any other properties of
> ANNOTATE_EXPRs that might be inherited from the operand (that could be
> affected by such a change).
> 
> It looks like TREE_{READONLY,THIS_VOLATILE,SIDE_EFFECTS} are all set
> in convert_from_reference and propagated by build3, so if those change
> underneath us then only updating the type seems insufficient.

I think TREE_THIS_VOLATILE isn't, that is only for references.
The others could change, but only in the !TREE_SIDE_EFFECTS ->
TREE_SIDE_EFFECTS or TREE_READONLY -> !TREE_READONLY direction
(the former if it was volatile bool &).
So you could also in the loop update it just in case,
      TREE_SIDE_EFFECTS (c) |= TREE_SIDE_EFFECTS (*condp);
      TREE_READONLY (c) &= TREE_READONLY (*condp);

	Jakub
Alex Coplan Aug. 9, 2024, 4:12 p.m. UTC | #7 
On 09/08/2024 17:56, Jakub Jelinek wrote:
> On Fri, Aug 09, 2024 at 04:46:55PM +0100, Alex Coplan wrote:
> > On 09/08/2024 17:34, Jakub Jelinek wrote:
> > > On Fri, Aug 09, 2024 at 04:21:14PM +0100, Alex Coplan wrote:
> > > > Hmm, good spot, I didn't realise that convert_from_reference could
> > > > change the type of the condition like this.
> > > > 
> > > > In that case I suppose the only thing to do is to construct a stack of
> > > > ANNOTATE_EXPRs on the way down and re-build the expressions (taking the
> > > > type of the inner expression, starting with the cond) on the way back
> > > > up.
> > > 
> > > I think you don't need to rebuild them, just update their types.
> > > Something along the lines of
> > >   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);
> > >    }
> > 
> > I suppose I was just concerned about any other properties of
> > ANNOTATE_EXPRs that might be inherited from the operand (that could be
> > affected by such a change).
> > 
> > It looks like TREE_{READONLY,THIS_VOLATILE,SIDE_EFFECTS} are all set
> > in convert_from_reference and propagated by build3, so if those change
> > underneath us then only updating the type seems insufficient.
> 
> I think TREE_THIS_VOLATILE isn't, that is only for references.
> The others could change, but only in the !TREE_SIDE_EFFECTS ->
> TREE_SIDE_EFFECTS or TREE_READONLY -> !TREE_READONLY direction
> (the former if it was volatile bool &).
> So you could also in the loop update it just in case,
>       TREE_SIDE_EFFECTS (c) |= TREE_SIDE_EFFECTS (*condp);
>       TREE_READONLY (c) &= TREE_READONLY (*condp);

Any reason not to just update those unconditionally?  I.e. just make
those normal assignments?  I'm assuming we'll only run the loop at all
in the case that TREE_TYPE (*condp) != TREE_TYPE (cond).

Alex

> 
> 	Jakub
>
Jakub Jelinek Aug. 9, 2024, 4:46 p.m. UTC | #8 
On Fri, Aug 09, 2024 at 05:12:00PM +0100, Alex Coplan wrote:
> > > I suppose I was just concerned about any other properties of
> > > ANNOTATE_EXPRs that might be inherited from the operand (that could be
> > > affected by such a change).
> > > 
> > > It looks like TREE_{READONLY,THIS_VOLATILE,SIDE_EFFECTS} are all set
> > > in convert_from_reference and propagated by build3, so if those change
> > > underneath us then only updating the type seems insufficient.
> > 
> > I think TREE_THIS_VOLATILE isn't, that is only for references.
> > The others could change, but only in the !TREE_SIDE_EFFECTS ->
> > TREE_SIDE_EFFECTS or TREE_READONLY -> !TREE_READONLY direction
> > (the former if it was volatile bool &).
> > So you could also in the loop update it just in case,
> >       TREE_SIDE_EFFECTS (c) |= TREE_SIDE_EFFECTS (*condp);
> >       TREE_READONLY (c) &= TREE_READONLY (*condp);
> 
> Any reason not to just update those unconditionally?  I.e. just make
> those normal assignments?  I'm assuming we'll only run the loop at all
> in the case that TREE_TYPE (*condp) != TREE_TYPE (cond).

ANNOTATE_EXPR has 3 arguments, not one, and the flags are dependent on all
of them.  If you look at the build3 construction of those flags,
TREE_SIDE_EFFECTS is ored from all the operands while TREE_READONLY is only
set if all the operands are.

	Jakub
diff mbox series

Patch

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index e58612660c9..8634a188458 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -966,25 +966,33 @@  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);
+
   /* 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);
+  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);
+}