public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH 2/2] c++: speculative constexpr and is_constant_evaluated [PR108243]
Date: Thu, 9 Feb 2023 15:36:35 -0800	[thread overview]
Message-ID: <e4c16c94-dce2-b36d-946b-62998c6965e6@redhat.com> (raw)
In-Reply-To: <b63670ca-2514-e720-3d6f-1be2b0119bbc@idea>

On 2/9/23 09:36, Patrick Palka wrote:
> On Sun, 5 Feb 2023, Jason Merrill wrote:
> 
>> On 2/3/23 15:51, Patrick Palka wrote:
>>> On Mon, 30 Jan 2023, Jason Merrill wrote:
>>>
>>>> On 1/27/23 17:02, Patrick Palka wrote:
>>>>> This PR illustrates that __builtin_is_constant_evaluated currently acts
>>>>> as an optimization barrier for our speculative constexpr evaluation,
>>>>> since we don't want to prematurely fold the builtin to false if the
>>>>> expression in question would be later manifestly constant evaluated (in
>>>>> which case it must be folded to true).
>>>>>
>>>>> This patch fixes this by permitting __builtin_is_constant_evaluated
>>>>> to get folded as false during cp_fold_function, since at that point
>>>>> we're sure we're doing manifestly constant evaluation.  To that end
>>>>> we add a flags parameter to cp_fold that controls what mce_value the
>>>>> CALL_EXPR case passes to maybe_constant_value.
>>>>>
>>>>> bootstrapped and rgetsted no x86_64-pc-linux-gnu, does this look OK for
>>>>> trunk?
>>>>>
>>>>> 	PR c++/108243
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> 	* cp-gimplify.cc (enum fold_flags): Define.
>>>>> 	(cp_fold_data::genericize): Replace this data member with ...
>>>>> 	(cp_fold_data::fold_flags): ... this.
>>>>> 	(cp_fold_r): Adjust cp_fold_data use and cp_fold_calls.
>>>>> 	(cp_fold_function): Likewise.
>>>>> 	(cp_fold_maybe_rvalue): Likewise.
>>>>> 	(cp_fully_fold_init): Likewise.
>>>>> 	(cp_fold): Add fold_flags parameter.  Don't cache if flags
>>>>> 	isn't empty.
>>>>> 	<case CALL_EXPR>: Pass mce_false to maybe_constant_value
>>>>> 	if if ff_genericize is set.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> 	* g++.dg/opt/pr108243.C: New test.
>>>>> ---
>>>>>     gcc/cp/cp-gimplify.cc               | 76
>>>>> ++++++++++++++++++-----------
>>>>>     gcc/testsuite/g++.dg/opt/pr108243.C | 29 +++++++++++
>>>>>     2 files changed, 76 insertions(+), 29 deletions(-)
>>>>>     create mode 100644 gcc/testsuite/g++.dg/opt/pr108243.C
>>>>>
>>>>> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
>>>>> index a35cedd05cc..d023a63768f 100644
>>>>> --- a/gcc/cp/cp-gimplify.cc
>>>>> +++ b/gcc/cp/cp-gimplify.cc
>>>>> @@ -43,12 +43,20 @@ along with GCC; see the file COPYING3.  If not see
>>>>>     #include "omp-general.h"
>>>>>     #include "opts.h"
>>>>>     +/* Flags for cp_fold and cp_fold_r.  */
>>>>> +
>>>>> +enum fold_flags {
>>>>> +  ff_none = 0,
>>>>> +  /* Whether we're being called from cp_fold_function.  */
>>>>> +  ff_genericize = 1 << 0,
>>>>> +};
>>>>> +
>>>>>     /* Forward declarations.  */
>>>>>       static tree cp_genericize_r (tree *, int *, void *);
>>>>>     static tree cp_fold_r (tree *, int *, void *);
>>>>>     static void cp_genericize_tree (tree*, bool);
>>>>> -static tree cp_fold (tree);
>>>>> +static tree cp_fold (tree, fold_flags);
>>>>>       /* Genericize a TRY_BLOCK.  */
>>>>>     @@ -996,9 +1004,8 @@ struct cp_genericize_data
>>>>>     struct cp_fold_data
>>>>>     {
>>>>>       hash_set<tree> pset;
>>>>> -  bool genericize; // called from cp_fold_function?
>>>>> -
>>>>> -  cp_fold_data (bool g): genericize (g) {}
>>>>> +  fold_flags flags;
>>>>> +  cp_fold_data (fold_flags flags): flags (flags) {}
>>>>>     };
>>>>>       static tree
>>>>> @@ -1039,7 +1046,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
>>>>> *data_)
>>>>>           break;
>>>>>         }
>>>>>     -  *stmt_p = stmt = cp_fold (*stmt_p);
>>>>> +  *stmt_p = stmt = cp_fold (*stmt_p, data->flags);
>>>>>         if (data->pset.add (stmt))
>>>>>         {
>>>>> @@ -1119,12 +1126,12 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees,
>>>>> void
>>>>> *data_)
>>>>>     	 here rather than in cp_genericize to avoid problems with the
>>>>> invisible
>>>>>     	 reference transition.  */
>>>>>         case INIT_EXPR:
>>>>> -      if (data->genericize)
>>>>> +      if (data->flags & ff_genericize)
>>>>>     	cp_genericize_init_expr (stmt_p);
>>>>>           break;
>>>>>           case TARGET_EXPR:
>>>>> -      if (data->genericize)
>>>>> +      if (data->flags & ff_genericize)
>>>>>     	cp_genericize_target_expr (stmt_p);
>>>>>             /* Folding might replace e.g. a COND_EXPR with a TARGET_EXPR;
>>>>> in
>>>>> @@ -1157,7 +1164,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
>>>>> *data_)
>>>>>     void
>>>>>     cp_fold_function (tree fndecl)
>>>>>     {
>>>>> -  cp_fold_data data (/*genericize*/true);
>>>>> +  cp_fold_data data (ff_genericize);
>>>>>       cp_walk_tree (&DECL_SAVED_TREE (fndecl), cp_fold_r, &data, NULL);
>>>>>     }
>>>>>     @@ -2375,7 +2382,7 @@ cp_fold_maybe_rvalue (tree x, bool rval)
>>>>>     {
>>>>>       while (true)
>>>>>         {
>>>>> -      x = cp_fold (x);
>>>>> +      x = cp_fold (x, ff_none);
>>>>>           if (rval)
>>>>>     	x = mark_rvalue_use (x);
>>>>>           if (rval && DECL_P (x)
>>>>> @@ -2434,7 +2441,7 @@ cp_fully_fold_init (tree x)
>>>>>       if (processing_template_decl)
>>>>>         return x;
>>>>>       x = cp_fully_fold (x);
>>>>> -  cp_fold_data data (/*genericize*/false);
>>>>> +  cp_fold_data data (ff_none);
>>>>>       cp_walk_tree (&x, cp_fold_r, &data, NULL);
>>>>>       return x;
>>>>>     }
>>>>> @@ -2469,7 +2476,7 @@ clear_fold_cache (void)
>>>>>         Function returns X or its folded variant.  */
>>>>>       static tree
>>>>> -cp_fold (tree x)
>>>>> +cp_fold (tree x, fold_flags flags)
>>>>>     {
>>>>>       tree op0, op1, op2, op3;
>>>>>       tree org_x = x, r = NULL_TREE;
>>>>> @@ -2490,8 +2497,11 @@ cp_fold (tree x)
>>>>>       if (fold_cache == NULL)
>>>>>         fold_cache = hash_map<tree, tree>::create_ggc (101);
>>>>>     -  if (tree *cached = fold_cache->get (x))
>>>>> -    return *cached;
>>>>> +  bool cache_p = (flags == ff_none);
>>>>> +
>>>>> +  if (cache_p)
>>>>> +    if (tree *cached = fold_cache->get (x))
>>>>> +      return *cached;
>>>>>         uid_sensitive_constexpr_evaluation_checker c;
>>>>>     @@ -2526,7 +2536,7 @@ cp_fold (tree x)
>>>>>     	     Don't create a new tree if op0 != TREE_OPERAND (x, 0),
>>>>> the
>>>>>     	     folding of the operand should be in the caches and if in
>>>>> cp_fold_r
>>>>>     	     it will modify it in place.  */
>>>>> -	  op0 = cp_fold (TREE_OPERAND (x, 0));
>>>>> +	  op0 = cp_fold (TREE_OPERAND (x, 0), flags);
>>>>>     	  if (op0 == error_mark_node)
>>>>>     	    x = error_mark_node;
>>>>>     	  break;
>>>>> @@ -2571,7 +2581,7 @@ cp_fold (tree x)
>>>>>     	{
>>>>>     	  tree p = maybe_undo_parenthesized_ref (x);
>>>>>     	  if (p != x)
>>>>> -	    return cp_fold (p);
>>>>> +	    return cp_fold (p, flags);
>>>>>     	}
>>>>>           goto unary;
>>>>>     @@ -2763,8 +2773,8 @@ cp_fold (tree x)
>>>>>         case COND_EXPR:
>>>>>           loc = EXPR_LOCATION (x);
>>>>>           op0 = cp_fold_rvalue (TREE_OPERAND (x, 0));
>>>>> -      op1 = cp_fold (TREE_OPERAND (x, 1));
>>>>> -      op2 = cp_fold (TREE_OPERAND (x, 2));
>>>>> +      op1 = cp_fold (TREE_OPERAND (x, 1), flags);
>>>>> +      op2 = cp_fold (TREE_OPERAND (x, 2), flags);
>>>>>             if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
>>>>>     	{
>>>>> @@ -2854,7 +2864,7 @@ cp_fold (tree x)
>>>>>     	      {
>>>>>     		if (!same_type_p (TREE_TYPE (x), TREE_TYPE (r)))
>>>>>     		  r = build_nop (TREE_TYPE (x), r);
>>>>> -		x = cp_fold (r);
>>>>> +		x = cp_fold (r, flags);
>>>>>     		break;
>>>>>     	      }
>>>>>     	  }
>>>>> @@ -2908,7 +2918,7 @@ cp_fold (tree x)
>>>>>     	int m = call_expr_nargs (x);
>>>>>     	for (int i = 0; i < m; i++)
>>>>>     	  {
>>>>> -	    r = cp_fold (CALL_EXPR_ARG (x, i));
>>>>> +	    r = cp_fold (CALL_EXPR_ARG (x, i), flags);
>>>>>     	    if (r != CALL_EXPR_ARG (x, i))
>>>>>     	      {
>>>>>     		if (r == error_mark_node)
>>>>> @@ -2931,7 +2941,7 @@ cp_fold (tree x)
>>>>>       	if (TREE_CODE (r) != CALL_EXPR)
>>>>>     	  {
>>>>> -	    x = cp_fold (r);
>>>>> +	    x = cp_fold (r, flags);
>>>>>     	    break;
>>>>>     	  }
>>>>>     @@ -2944,7 +2954,15 @@ cp_fold (tree x)
>>>>>     	   constant, but the call followed by an INDIRECT_REF is.  */
>>>>>     	if (callee && DECL_DECLARED_CONSTEXPR_P (callee)
>>>>>     	    && !flag_no_inline)
>>>>> -	  r = maybe_constant_value (x);
>>>>> +	  {
>>>>> +	    mce_value manifestly_const_eval = mce_unknown;
>>>>> +	    if (flags & ff_genericize)
>>>>> +	      /* At genericization time it's safe to fold
>>>>> +		 __builtin_is_constant_evaluated to false.  */
>>>>> +	      manifestly_const_eval = mce_false;
>>>>> +	    r = maybe_constant_value (x, /*decl=*/NULL_TREE,
>>>>> +				      manifestly_const_eval);
>>>>> +	  }
>>>>>     	optimize = sv;
>>>>>               if (TREE_CODE (r) != CALL_EXPR)
>>>>> @@ -2971,7 +2989,7 @@ cp_fold (tree x)
>>>>>     	vec<constructor_elt, va_gc> *nelts = NULL;
>>>>>     	FOR_EACH_VEC_SAFE_ELT (elts, i, p)
>>>>>     	  {
>>>>> -	    tree op = cp_fold (p->value);
>>>>> +	    tree op = cp_fold (p->value, flags);
>>>>>     	    if (op != p->value)
>>>>>     	      {
>>>>>     		if (op == error_mark_node)
>>>>> @@ -3002,7 +3020,7 @@ cp_fold (tree x)
>>>>>       	for (int i = 0; i < n; i++)
>>>>>     	  {
>>>>> -	    tree op = cp_fold (TREE_VEC_ELT (x, i));
>>>>> +	    tree op = cp_fold (TREE_VEC_ELT (x, i), flags);
>>>>>     	    if (op != TREE_VEC_ELT (x, i))
>>>>>     	      {
>>>>>     		if (!changed)
>>>>> @@ -3019,10 +3037,10 @@ cp_fold (tree x)
>>>>>         case ARRAY_RANGE_REF:
>>>>>             loc = EXPR_LOCATION (x);
>>>>> -      op0 = cp_fold (TREE_OPERAND (x, 0));
>>>>> -      op1 = cp_fold (TREE_OPERAND (x, 1));
>>>>> -      op2 = cp_fold (TREE_OPERAND (x, 2));
>>>>> -      op3 = cp_fold (TREE_OPERAND (x, 3));
>>>>> +      op0 = cp_fold (TREE_OPERAND (x, 0), flags);
>>>>> +      op1 = cp_fold (TREE_OPERAND (x, 1), flags);
>>>>> +      op2 = cp_fold (TREE_OPERAND (x, 2), flags);
>>>>> +      op3 = cp_fold (TREE_OPERAND (x, 3), flags);
>>>>>             if (op0 != TREE_OPERAND (x, 0)
>>>>>     	  || op1 != TREE_OPERAND (x, 1)
>>>>> @@ -3050,7 +3068,7 @@ cp_fold (tree x)
>>>>>           /* A SAVE_EXPR might contain e.g. (0 * i) + (0 * j), which,
>>>>> after
>>>>>     	 folding, evaluates to an invariant.  In that case no need to
>>>>> wrap
>>>>>     	 this folded tree with a SAVE_EXPR.  */
>>>>> -      r = cp_fold (TREE_OPERAND (x, 0));
>>>>> +      r = cp_fold (TREE_OPERAND (x, 0), flags);
>>>>>           if (tree_invariant_p (r))
>>>>>     	x = r;
>>>>>           break;
>>>>> @@ -3069,7 +3087,7 @@ cp_fold (tree x)
>>>>>           copy_warning (x, org_x);
>>>>>         }
>>>>>     -  if (!c.evaluation_restricted_p ())
>>>>> +  if (cache_p && !c.evaluation_restricted_p ())
>>>>>         {
>>>>>           fold_cache->put (org_x, x);
>>>>>           /* Prevent that we try to fold an already folded result again.
>>>>> */
>>>>> diff --git a/gcc/testsuite/g++.dg/opt/pr108243.C
>>>>> b/gcc/testsuite/g++.dg/opt/pr108243.C
>>>>> new file mode 100644
>>>>> index 00000000000..4c45dbba13c
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/g++.dg/opt/pr108243.C
>>>>> @@ -0,0 +1,29 @@
>>>>> +// PR c++/108243
>>>>> +// { dg-do compile { target c++11 } }
>>>>> +// { dg-additional-options "-O -fdump-tree-original" }
>>>>> +
>>>>> +constexpr int foo() {
>>>>> +  return __builtin_is_constant_evaluated() + 1;
>>>>> +}
>>>>> +
>>>>> +#if __cpp_if_consteval
>>>>> +constexpr int bar() {
>>>>> +  if consteval {
>>>>> +    return 5;
>>>>> +  } else {
>>>>> +    return 4;
>>>>> +  }
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +int p, q;
>>>>> +
>>>>> +int main() {
>>>>> +  p = foo();
>>>>> +#if __cpp_if_consteval
>>>>> +  q = bar();
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>> +// { dg-final { scan-tree-dump-not "= foo" "original" } }
>>>>> +// { dg-final { scan-tree-dump-not "= bar" "original" } }
>>>>
>>>> Let's also test a static initializer that can't be fully
>>>> constant-evaluated.
>>>
>>> D'oh, doing so revealed that cp_fold_function doesn't reach static
>>> initializers; that's taken care of by cp_fully_fold_init.  So it seems
>>> we need to make cp_fold when called from the latter entry point to also
>>> assume m_c_e is false.  We can't re-use ff_genericize here because that
>>> flag has additional effects in cp_fold_r, so it seems we need another
>>> flag that that only affects the manifestly constant-eval stuff; I called
>>> it ff_mce_false.  How does the following look?
>>>
>>> -- >8 --
>>>
>>> Subject: [PATCH 2/2] c++: speculative constexpr and is_constant_evaluated
>>>    [PR108243]
>>>
>>> This PR illustrates that __builtin_is_constant_evaluated currently acts
>>> as an optimization barrier for our speculative constexpr evaluation,
>>> since we don't want to prematurely fold the builtin to false if the
>>> expression in question would be later manifestly constant evaluated (in
>>> which case it must be folded to true).
>>>
>>> This patch fixes this by permitting __builtin_is_constant_evaluated
>>> to get folded as false during cp_fold_function and cp_fully_fold_init,
>>> since at these points we're sure we're done with manifestly constant
>>> evaluation.  To that end we add a flags parameter to cp_fold that
>>> controls whether we pass mce_false or mce_unknown to maybe_constant_value
>>> when folding a CALL_EXPR.
>>>
>>> 	PR c++/108243
>>> 	PR c++/97553
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* cp-gimplify.cc (enum fold_flags): Define.
>>> 	(cp_fold_data::genericize): Replace this data member with ...
>>> 	(cp_fold_data::fold_flags): ... this.
>>> 	(cp_fold_r): Adjust use of cp_fold_data and calls to cp_fold.
>>> 	(cp_fold_function): Likewise.
>>> 	(cp_fold_maybe_rvalue): Likewise.
>>> 	(cp_fully_fold_init): Likewise.
>>> 	(cp_fold): Add fold_flags parameter.  Don't cache if flags
>>> 	isn't empty.
>>> 	<case CALL_EXPR>: If ff_genericize is set, fold
>>> 	__builtin_is_constant_evaluated to false and pass mce_false to
>>> 	maybe_constant_value.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/opt/is_constant_evaluated1.C: New test.
>>> 	* g++.dg/opt/is_constant_evaluated2.C: New test.
>>> ---
>>>    gcc/cp/cp-gimplify.cc                         | 88 ++++++++++++-------
>>>    .../g++.dg/opt/is_constant_evaluated1.C       | 14 +++
>>>    .../g++.dg/opt/is_constant_evaluated2.C       | 32 +++++++
>>>    3 files changed, 104 insertions(+), 30 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/opt/is_constant_evaluated1.C
>>>    create mode 100644 gcc/testsuite/g++.dg/opt/is_constant_evaluated2.C
>>>
>>> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
>>> index 9929d29981a..590ed787997 100644
>>> --- a/gcc/cp/cp-gimplify.cc
>>> +++ b/gcc/cp/cp-gimplify.cc
>>> @@ -43,12 +43,26 @@ along with GCC; see the file COPYING3.  If not see
>>>    #include "omp-general.h"
>>>    #include "opts.h"
>>>    +/* Flags for cp_fold and cp_fold_r.  */
>>> +
>>> +enum fold_flags {
>>> +  ff_none = 0,
>>> +  /* Whether we're being called from cp_fold_function.  */
>>> +  ff_genericize = 1 << 0,
>>> +  /* Whether we're folding late enough that we could assume
>>> +     we're definitely not in a manifestly constant-evaluated
>>> +     context.  */
>>
>> It's not necessarily a matter of late enough; we could fold sooner and still
>> know that, as in cp_fully_fold_init.  We could do the same at other
>> full-expression points, but we don't because we want to delay folding as much
>> as possible.  So let's say "folding at a point where we know we're..."
>>
>>> +  ff_mce_false = 1 << 1,
>>> +};
>>> +
>>> +using fold_flags_t = int;
>>> +
>>>    /* Forward declarations.  */
>>>      static tree cp_genericize_r (tree *, int *, void *);
>>>    static tree cp_fold_r (tree *, int *, void *);
>>>    static void cp_genericize_tree (tree*, bool);
>>> -static tree cp_fold (tree);
>>> +static tree cp_fold (tree, fold_flags_t);
>>>      /* Genericize a TRY_BLOCK.  */
>>>    @@ -1012,9 +1026,8 @@ struct cp_genericize_data
>>>    struct cp_fold_data
>>>    {
>>>      hash_set<tree> pset;
>>> -  bool genericize; // called from cp_fold_function?
>>> -
>>> -  cp_fold_data (bool g): genericize (g) {}
>>> +  fold_flags_t flags;
>>> +  cp_fold_data (fold_flags_t flags): flags (flags) {}
>>>    };
>>>      static tree
>>> @@ -1055,7 +1068,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
>>> *data_)
>>>          break;
>>>        }
>>>    -  *stmt_p = stmt = cp_fold (*stmt_p);
>>> +  *stmt_p = stmt = cp_fold (*stmt_p, data->flags);
>>>        if (data->pset.add (stmt))
>>>        {
>>> @@ -1135,12 +1148,12 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
>>> *data_)
>>>    	 here rather than in cp_genericize to avoid problems with the
>>> invisible
>>>    	 reference transition.  */
>>>        case INIT_EXPR:
>>> -      if (data->genericize)
>>> +      if (data->flags & ff_genericize)
>>>    	cp_genericize_init_expr (stmt_p);
>>>          break;
>>>          case TARGET_EXPR:
>>> -      if (data->genericize)
>>> +      if (data->flags & ff_genericize)
>>>    	cp_genericize_target_expr (stmt_p);
>>>            /* Folding might replace e.g. a COND_EXPR with a TARGET_EXPR; in
>>> @@ -1173,7 +1186,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
>>> *data_)
>>>    void
>>>    cp_fold_function (tree fndecl)
>>>    {
>>> -  cp_fold_data data (/*genericize*/true);
>>> +  cp_fold_data data (ff_genericize | ff_mce_false);
>>
>> Here would be a good place for a comment about passing mce_false because all
>> manifestly-constant-evaluated expressions will have been constant-evaluated
>> already if possible.
>>
>>>      cp_walk_tree (&DECL_SAVED_TREE (fndecl), cp_fold_r, &data, NULL);
>>>    }
>>>    @@ -2391,7 +2404,7 @@ cp_fold_maybe_rvalue (tree x, bool rval)
>>>    {
>>>      while (true)
>>>        {
>>> -      x = cp_fold (x);
>>> +      x = cp_fold (x, ff_none);
>>>          if (rval)
>>>    	x = mark_rvalue_use (x);
>>>          if (rval && DECL_P (x)
>>> @@ -2450,7 +2463,7 @@ cp_fully_fold_init (tree x)
>>>      if (processing_template_decl)
>>>        return x;
>>>      x = cp_fully_fold (x);
>>> -  cp_fold_data data (/*genericize*/false);
>>> +  cp_fold_data data (ff_mce_false);
>>>      cp_walk_tree (&x, cp_fold_r, &data, NULL);
>>>      return x;
>>>    }
>>> @@ -2485,7 +2498,7 @@ clear_fold_cache (void)
>>>        Function returns X or its folded variant.  */
>>>      static tree
>>> -cp_fold (tree x)
>>> +cp_fold (tree x, fold_flags_t flags)
>>>    {
>>>      tree op0, op1, op2, op3;
>>>      tree org_x = x, r = NULL_TREE;
>>> @@ -2506,8 +2519,11 @@ cp_fold (tree x)
>>>      if (fold_cache == NULL)
>>>        fold_cache = hash_map<tree, tree>::create_ggc (101);
>>>    -  if (tree *cached = fold_cache->get (x))
>>> -    return *cached;
>>> +  bool cache_p = (flags == ff_none);
>>> +
>>> +  if (cache_p)
>>> +    if (tree *cached = fold_cache->get (x))
>>> +      return *cached;
>>>        uid_sensitive_constexpr_evaluation_checker c;
>>>    @@ -2542,7 +2558,7 @@ cp_fold (tree x)
>>>    	     Don't create a new tree if op0 != TREE_OPERAND (x, 0), the
>>>    	     folding of the operand should be in the caches and if in
>>> cp_fold_r
>>>    	     it will modify it in place.  */
>>> -	  op0 = cp_fold (TREE_OPERAND (x, 0));
>>> +	  op0 = cp_fold (TREE_OPERAND (x, 0), flags);
>>>    	  if (op0 == error_mark_node)
>>>    	    x = error_mark_node;
>>>    	  break;
>>> @@ -2587,7 +2603,7 @@ cp_fold (tree x)
>>>    	{
>>>    	  tree p = maybe_undo_parenthesized_ref (x);
>>>    	  if (p != x)
>>> -	    return cp_fold (p);
>>> +	    return cp_fold (p, flags);
>>>    	}
>>>          goto unary;
>>>    @@ -2779,8 +2795,8 @@ cp_fold (tree x)
>>>        case COND_EXPR:
>>>          loc = EXPR_LOCATION (x);
>>>          op0 = cp_fold_rvalue (TREE_OPERAND (x, 0));
>>> -      op1 = cp_fold (TREE_OPERAND (x, 1));
>>> -      op2 = cp_fold (TREE_OPERAND (x, 2));
>>> +      op1 = cp_fold (TREE_OPERAND (x, 1), flags);
>>> +      op2 = cp_fold (TREE_OPERAND (x, 2), flags);
>>>            if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
>>>    	{
>>> @@ -2870,7 +2886,7 @@ cp_fold (tree x)
>>>    	      {
>>>    		if (!same_type_p (TREE_TYPE (x), TREE_TYPE (r)))
>>>    		  r = build_nop (TREE_TYPE (x), r);
>>> -		x = cp_fold (r);
>>> +		x = cp_fold (r, flags);
>>>    		break;
>>>    	      }
>>>    	  }
>>> @@ -2890,8 +2906,12 @@ cp_fold (tree x)
>>>    	  {
>>>    	    switch (DECL_FE_FUNCTION_CODE (callee))
>>>    	      {
>>> -		/* Defer folding __builtin_is_constant_evaluated.  */
>>>    	      case CP_BUILT_IN_IS_CONSTANT_EVALUATED:
>>> +		/* Defer folding __builtin_is_constant_evaluated unless
>>> +		   we can assume this isn't a manifestly constant-evaluated
>>
>> s/can assume/know/
>>
>> OK with those comment changes.
> 
> Thanks a lot.  Unfortunately I think the patch has a significant problem
> that only just occurred to me -- disabling the cp_fold cache when the
> flag ff_mce_false is set effectively makes cp_fold_function and
> cp_fully_fold_init quadratic in the size of the expression (since
> cp_fold_r calls cp_fold on each subtree, and cp_fold when the cache is
> disabled will end up fully walking each subtree).  Note that the reason
> we must disable the cache is because cp_fold with ff_mce_false might
> give a different folded result than without that flag if the expression
> contains a suitable CALL_EXPR subexpression.

Good point.

> One approach to fix this complexity issue would be to parameterize the
> cache according to the flags that were passed to cp_fold, which would
> allow us to keep the cache enabled when ff_mce_false is set.  A downside
> to this approach is that the size of the cp_fold cache would essentially
> double since for each tree we'd now have two cache entries, one for
> flags=ff_none and another for flags=ff_mce_false.

We could also clear the cache before cp_fold_function since the two 
folds shouldn't overlap (much).

> Another approach would be to split out the trial constexpr evaluation
> part of cp_fold's CALL_EXPR handling, parameterize that, and call it
> directly from cp_fold_r.  With this approach we wouldn't perform as much
> folding, e.g.
> 
>    int n = 41 + !std::is_constant_evaluated();
> 
> would get folded to 1 + 41 rather than 42.  But I suspect this would
> give us 95% of the reapable benefits of the above approach.
> 
> I think I'm leaning towards this second approach, which the below patch
> implements instead.  What do you think?  Bootstrapped and regtested on
> x86_64-pc-linux-gnu.

That sounds reasonable, but...

> -- >8 --
> 
> Subject: [PATCH] c++: speculative constexpr and is_constant_evaluated
>   [PR108243]
> 
> This PR illustrates that __builtin_is_constant_evaluated currently acts
> as an optimization barrier for our speculative constexpr evaluation,
> since we don't want to prematurely fold the builtin to false before the
> expression in question undergoes constant evaluation as in a manifestly
> constant-evaluated context (in which case the builtin must instead be
> folded to true).
> 
> This patch fixes this by permitting __builtin_is_constant_evaluated
> to get folded to false from cp_fold_r, where we know we're done with
> proper constant evaluation (of manifestly constant-evaluated contexts).
> 
> 	PR c++/108243
> 	PR c++/97553
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-gimplify.cc
> 	(cp_fold_r): Remove redundant *stmt_p assignments.  After
> 	calling cp_fold, call maybe_fold_constexpr_call with mce_false.
> 	(cp_fold) <case CALL_EXPR>: Split out trial constexpr evaluation
> 	into ...
> 	(maybe_fold_constexpr_call): ... here.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/opt/is_constant_evaluated1.C: New test.
> 	* g++.dg/opt/is_constant_evaluated2.C: New test.
> ---
>   gcc/cp/cp-gimplify.cc                         | 55 +++++++++++++++----
>   .../g++.dg/opt/is_constant_evaluated1.C       | 20 +++++++
>   .../g++.dg/opt/is_constant_evaluated2.C       | 32 +++++++++++
>   3 files changed, 95 insertions(+), 12 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/opt/is_constant_evaluated1.C
>   create mode 100644 gcc/testsuite/g++.dg/opt/is_constant_evaluated2.C
> 
> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> index 9929d29981a..dca55056b2c 100644
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -49,6 +49,7 @@ static tree cp_genericize_r (tree *, int *, void *);
>   static tree cp_fold_r (tree *, int *, void *);
>   static void cp_genericize_tree (tree*, bool);
>   static tree cp_fold (tree);
> +static tree maybe_fold_constexpr_call (tree, mce_value);
>   
>   /* Genericize a TRY_BLOCK.  */
>   
> @@ -1034,7 +1035,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void *data_)
>   	    error_at (PTRMEM_CST_LOCATION (stmt),
>   		      "taking address of an immediate function %qD",
>   		      PTRMEM_CST_MEMBER (stmt));
> -	  stmt = *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> +	  stmt = build_zero_cst (TREE_TYPE (stmt));
>   	  break;
>   	}
>         break;
> @@ -1046,7 +1047,7 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void *data_)
>   	  error_at (EXPR_LOCATION (stmt),
>   		    "taking address of an immediate function %qD",
>   		    TREE_OPERAND (stmt, 0));
> -	  stmt = *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> +	  stmt = build_zero_cst (TREE_TYPE (stmt));
>   	  break;
>   	}
>         break;
> @@ -1055,7 +1056,17 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void *data_)
>         break;
>       }
>   
> -  *stmt_p = stmt = cp_fold (*stmt_p);
> +  stmt = cp_fold (stmt);
> +
> +  if (TREE_CODE (stmt) == CALL_EXPR)
> +    /* Since cp_fold_r is called (from cp_genericize, cp_fold_function
> +       and cp_fully_fold_init) only after the overall expression has been
> +       considered for constant-evaluation, we can by now safely fold any
> +       remaining __builtin_is_constant_evaluated calls to false, so try
> +       constexpr expansion with mce_false.  */
> +    stmt = maybe_fold_constexpr_call (stmt, mce_false);
> +
> +  *stmt_p = stmt;
>   
>     if (data->pset.add (stmt))
>       {
> @@ -2952,15 +2963,10 @@ cp_fold (tree x)
>   	  }
>   
>   	optimize = nw;
> -
> -	/* Invoke maybe_constant_value for functions declared
> -	   constexpr and not called with AGGR_INIT_EXPRs.
> -	   TODO:
> -	   Do constexpr expansion of expressions where the call itself is not
> -	   constant, but the call followed by an INDIRECT_REF is.  */
> -	if (callee && DECL_DECLARED_CONSTEXPR_P (callee)
> -	    && !flag_no_inline)
> -	  r = maybe_constant_value (x);
> +	/* Pass mce_unknown to defer folding __builtin_is_constant_evaluated
> +	   since we don't know if we're in a manifestly constant-evaluated
> +	   context that hasn't yet been constant-evaluated.  */
> +	r = maybe_fold_constexpr_call (x, mce_unknown);

It seems unfortunate to try to fold both here and in cp_fold_r.

Does this patch still fold __builtin_is_constant_evaluated() even though 
it no longer touches the cp_fold builtin handling?

>   	optimize = sv;
>   
>           if (TREE_CODE (r) != CALL_EXPR)
> @@ -3096,6 +3102,31 @@ cp_fold (tree x)
>     return x;
>   }
>   
> +/* If the CALL_EXPR X calls a constexpr function, try expanding it via
> +   constexpr evaluation.  Returns the expanded result or X if constexpr
> +   evaluation wasn't possible.
> +
> +   TODO: Do constexpr expansion of expressions where the call itself
> +   is not constant, but the call followed by an INDIRECT_REF is.  */
> +
> +static tree
> +maybe_fold_constexpr_call (tree x, mce_value manifestly_const_eval)
> +{
> +  if (flag_no_inline)
> +    return x;
> +  tree callee = get_callee_fndecl (x);
> +  if (!callee)
> +    return x;
> +  if (DECL_DECLARED_CONSTEXPR_P (callee))
> +    {
> +      tree r = maybe_constant_value (x, /*decl=*/NULL_TREE,
> +				     manifestly_const_eval);
> +      if (TREE_CODE (r) != CALL_EXPR)
> +	return r;
> +    }
> +  return x;
> +}
> +
>   /* Look up "hot", "cold", "likely" or "unlikely" in attribute list LIST.  */
>   
>   tree
> diff --git a/gcc/testsuite/g++.dg/opt/is_constant_evaluated1.C b/gcc/testsuite/g++.dg/opt/is_constant_evaluated1.C
> new file mode 100644
> index 00000000000..2123f20e3e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/opt/is_constant_evaluated1.C
> @@ -0,0 +1,20 @@
> +// PR c++/108243
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-O -fdump-tree-original" }
> +
> +struct A {
> +  constexpr A(int n, int m) : n(n), m(m) { }
> +  int n, m;
> +};
> +
> +constexpr int foo(int n) {
> +  return n + !__builtin_is_constant_evaluated();
> +}
> +
> +A* f(int n) {
> +  static A a = {n, foo(41)};
> +  return &a;
> +}
> +
> +// { dg-final { scan-tree-dump "42" "original" } }
> +// { dg-final { scan-tree-dump-not "foo \\(41\\)" "original" } }
> diff --git a/gcc/testsuite/g++.dg/opt/is_constant_evaluated2.C b/gcc/testsuite/g++.dg/opt/is_constant_evaluated2.C
> new file mode 100644
> index 00000000000..ed964e20a7a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/opt/is_constant_evaluated2.C
> @@ -0,0 +1,32 @@
> +// PR c++/97553
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-O -fdump-tree-original" }
> +
> +constexpr int foo() {
> +  return __builtin_is_constant_evaluated() + 1;
> +}
> +
> +#if __cpp_if_consteval
> +constexpr int bar() {
> +  if consteval {
> +    return 5;
> +  } else {
> +    return 4;
> +  }
> +}
> +#endif
> +
> +int p, q;
> +
> +int main() {
> +  p = foo();
> +#if __cpp_if_consteval
> +  q = bar();
> +#endif
> +}
> +
> +// { dg-final { scan-tree-dump "p = 1" "original" } }
> +// { dg-final { scan-tree-dump-not "= foo" "original" } }
> +
> +// { dg-final { scan-tree-dump "q = 4" "original" { target c++23 } } }
> +// { dg-final { scan-tree-dump-not "= bar" "original" { target c++23 } } }


  reply	other threads:[~2023-02-09 23:36 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-27 22:02 [PATCH 1/2] c++: make manifestly_const_eval tri-state Patrick Palka
2023-01-27 22:02 ` [PATCH 2/2] c++: speculative constexpr and is_constant_evaluated [PR108243] Patrick Palka
2023-01-27 22:05   ` Patrick Palka
2023-01-30 20:05   ` Jason Merrill
2023-02-03 20:51     ` Patrick Palka
2023-02-03 20:57       ` Patrick Palka
2023-02-05 20:11       ` Jason Merrill
2023-02-09 17:36         ` Patrick Palka
2023-02-09 23:36           ` Jason Merrill [this message]
2023-02-10  1:32             ` Patrick Palka
2023-02-10 14:48               ` Patrick Palka
2023-02-10 16:51                 ` Patrick Palka
2023-02-14 23:02                   ` Jason Merrill
2023-01-30 20:02 ` [PATCH 1/2] c++: make manifestly_const_eval tri-state Jason Merrill
2023-02-03 21:21   ` Patrick Palka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e4c16c94-dce2-b36d-946b-62998c6965e6@redhat.com \
    --to=jason@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=ppalka@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).