Re: [PATCH v2] c++: Move consteval folding to cp_fold_r

[Jason Merrill via Gcc-patches]

On 9/7/23 11:23, Marek Polacek wrote:

On Tue, Sep 05, 2023 at 04:36:34PM -0400, Jason Merrill wrote:

On 9/5/23 15:59, Marek Polacek wrote:

On Tue, Sep 05, 2023 at 10:52:04AM -0400, Jason Merrill wrote:

On 9/1/23 13:23, Marek Polacek wrote:

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --

In the review of P2564:

it turned out that in order to correctly handle an example in the paper,
we should stop doing immediate evaluation in build_over_call and
bot_replace, and instead do it in cp_fold_r.  This patch does that.

Another benefit is that this is a pretty significant simplification, at
least in my opinion.  Also, this fixes the c++/110997 ICE (but the test
doesn't compile yet).

The main drawback seems to be that cp_fold_r doesn't process as much
code as we did before: uninstantiated templates


That's acceptable, it's an optional diagnostic.


and things like "false ? foo () : 1".


This is a problem.  Maybe we want cp_fold_r to recurse into the arms of a
COND_EXPR before folding them away?  Maybe only if we know we've seen an
immediate function?


Unfortunately we had already thrown the dead branch away when we got to
cp_fold_r.  I wonder if we have to adjust cxx_eval_conditional_expression
to call cp_fold_r on the dead branch too,


Hmm, I guess so.


perhaps with a new ff_ flag to skip the whole second switch in cp_fold_r?


Or factor out the immediate function handling to a separate walk function
that cp_fold_r also calls?


I did that.
  

But then it's possible that the in_immediate_context checks have to stay.


We can just not do the walk in immediate (or mce_true) context, like we
currently avoid calling cp_fold_function.


Right.  Unfortunately I have to check even when mce_true, consider

   consteval int bar (int i) { if (i != 1) throw 1; return 0; }
   constexpr int a = 0 ? bar(3) : 3;


I disagree; the call is in a manifestly constant-evaluated expression, 
and so is now considered an immediate function context, and we should 
accept that example.



For mce_unknown I guess we'd want
to set *non_constant_p instead of giving an error.


I did not do this because I haven't found a case where it would make
a difference.


I think it will given the above comment.


diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 0ca4370deab..397d5c7ec3f 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -2311,6 +2311,29 @@ cxx_dynamic_cast_fn_p (tree fndecl)
  && CP_DECL_CONTEXT (fndecl) == abi_node);
  }
  
+/* Return true if we are in the body of a consteval function. > +   This is in addition to in_immediate_context because that

+   uses current_function_decl which may not be available.  CTX is
+   the current constexpr context.  */
+
+static bool
+in_immediate_context (const constexpr_ctx *ctx)
+{
+  if (in_immediate_context ())
+return true;


Can't we check for mce_true here instead of looking at the call chain?


+/* A wrapper around cp_fold_immediate_r.  */
+
+void
+cp_fold_immediate (tree *tp)
+{


Maybe return early if consteval isn't supported in the active standard?

Jason

[PATCH v2] c++: Move consteval folding to cp_fold_r

[Marek Polacek via Gcc-patches]

On Tue, Sep 05, 2023 at 04:36:34PM -0400, Jason Merrill wrote:
> On 9/5/23 15:59, Marek Polacek wrote:
> > On Tue, Sep 05, 2023 at 10:52:04AM -0400, Jason Merrill wrote:
> > > On 9/1/23 13:23, Marek Polacek wrote:
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > 
> > > > -- >8 --
> > > > 
> > > > In the review of P2564:
> > > > 
> > > > it turned out that in order to correctly handle an example in the paper,
> > > > we should stop doing immediate evaluation in build_over_call and
> > > > bot_replace, and instead do it in cp_fold_r.  This patch does that.
> > > > 
> > > > Another benefit is that this is a pretty significant simplification, at
> > > > least in my opinion.  Also, this fixes the c++/110997 ICE (but the test
> > > > doesn't compile yet).
> > > > 
> > > > The main drawback seems to be that cp_fold_r doesn't process as much
> > > > code as we did before: uninstantiated templates
> > > 
> > > That's acceptable, it's an optional diagnostic.
> > > 
> > > > and things like "false ? foo () : 1".
> > > 
> > > This is a problem.  Maybe we want cp_fold_r to recurse into the arms of a
> > > COND_EXPR before folding them away?  Maybe only if we know we've seen an
> > > immediate function?
> > 
> > Unfortunately we had already thrown the dead branch away when we got to
> > cp_fold_r.  I wonder if we have to adjust cxx_eval_conditional_expression
> > to call cp_fold_r on the dead branch too,
> 
> Hmm, I guess so.
> 
> > perhaps with a new ff_ flag to skip the whole second switch in cp_fold_r?
> 
> Or factor out the immediate function handling to a separate walk function
> that cp_fold_r also calls?

I did that.
 
> > But then it's possible that the in_immediate_context checks have to stay.
> 
> We can just not do the walk in immediate (or mce_true) context, like we
> currently avoid calling cp_fold_function.

Right.  Unfortunately I have to check even when mce_true, consider

  consteval int bar (int i) { if (i != 1) throw 1; return 0; }
  constexpr int a = 0 ? bar(3) : 3;

> For mce_unknown I guess we'd want
> to set *non_constant_p instead of giving an error.

I did not do this because I haven't found a case where it would make
a difference.

> This is a problem.  Maybe we want cp_fold_r to recurse into the arms of a
> COND_EXPR before folding them away?  Maybe only if we know we've seen an
> immediate function?

Hopefully resolved now.
 
> > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> > index 8bd5c4a47f8..af4f98b1fe1 100644
> > --- a/gcc/cp/constexpr.cc
> > +++ b/gcc/cp/constexpr.cc
> > @@ -3135,6 +3135,11 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, 
> > tree t,
> >   unsigned save_heap_alloc_count = ctx->global->heap_vars.length ();
> >   unsigned save_heap_dealloc_count = ctx->global->heap_dealloc_count;
> > + /* Make sure we fold std::is_constant_evaluated to true in an
> > +immediate function.  */
> > + if (immediate_invocation_p (fun))
> 
> I think this should just check DECL_IMMEDIATE_FUNCTION_P, the context
> doesn't matter.

Fixed.  And now I don't need to export immediate_invocation_p.
 
> > +   call_ctx.manifestly_const_eval = mce_true;
> > +
> > diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> > index 206e791fcfd..29132aad158 100644
> > --- a/gcc/cp/cp-gimplify.cc
> > +++ b/gcc/cp/cp-gimplify.cc
> > @@ -1058,9 +1058,21 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void 
> > *data_)
> > }
> > break;
> > +/* Expand immediate invocations.  */
> > +case CALL_EXPR:
> > +case AGGR_INIT_EXPR:
> > +  if (!in_immediate_context ())
> 
> As you mentioned in your followup, we shouldn't need to check this because
> we don't call cp_fold_r in immediate context.

Fixed.

> > +   if (tree fn = cp_get_callee (stmt))
> > + if (TREE_CODE (fn) != ADDR_EXPR || ADDR_EXPR_DENOTES_CALL_P (fn))
> > +   if (tree fndecl = cp_get_fndecl_from_callee (fn, /*fold*/false))
> > + if (DECL_IMMEDIATE_FUNCTION_P (fndecl))
> > +   *stmt_p = stmt = cxx_constant_value (stmt);
> > +  break;
> > +
> >   case ADDR_EXPR:
> > if (TREE_CODE (TREE_OPERAND (stmt, 0)) == FUNCTION_DECL
> > - && DECL_IMMEDIATE_FUNCTION_P (TREE_OPERAND (stmt, 0)))
> > + && DECL_IMMEDIATE_FUNCTION_P (TREE_OPERAND (stmt, 0))
> > + && !in_immediate_context ())
> 
> Likewise.

Fixed.
 
> > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> > index 799183dc646..7dfb6de2da3 100644
> > --- a/gcc/cp/tree.cc
> > +++ b/gcc/cp/tree.cc
> > @@ -3254,7 +3254,7 @@ bot_manip (tree* tp, int* walk_subtrees, void* data_)
> >  variables.  */
> >   static tree
> > -bot_replace (tree* t, int* walk_subtrees, void* data_)
> > +bot_replace (tree* t, int*, void* data_)
> 
> Generally we keep the parameter name as a comment like
> int */*walk_subtrees*/

I reaaally mislike that but ok, changed.

> > diff --git a/gcc/testsuite/g++.dg/cpp23/consteval-if