From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH v3] c++: Move consteval folding to cp_fold_r
Date: Tue, 12 Sep 2023 17:26:25 -0400 [thread overview]
Message-ID: <b694a250-45a5-19bc-c905-49df0fbc3c56@redhat.com> (raw)
In-Reply-To: <ZPtmwqs+Aywe6ZAR@redhat.com>
On 9/8/23 14:24, Marek Polacek wrote:
> On Thu, Sep 07, 2023 at 02:32:51PM -0400, Jason Merrill wrote:
>> 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:
>>>>>>> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628747.html>
>>>>>>> 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.
>
> Ack. I was still living in pre-P2564 world.
>
>>>> 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.
>
> Correct. For instance, in:
>
> consteval int bar (int i) { if (i != 1) throw 1; return 0; }
>
> constexpr int
> foo (bool b)
> {
> return b ? bar (3) : 2;
> }
>
> static_assert (foo (false) == 2);
>
> we should complain only once. I've implemented your suggestion to set
> *non_constant_p instead of giving an error for mce_unknown.
>
>>> 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?
>
> Yes.
>
>>> +/* 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?
>
> Absolutely.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> -- >8 --
> In the review of P2564:
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628747.html>
> 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
> uninstantiated templates. We still have to handle things like
> "false ? foo () : 1". To that end, I've added cp_fold_immediate, called
> on dead branches in cxx_eval_conditional_expression.
>
> You'll see that I've reintroduced ADDR_EXPR_DENOTES_CALL_P here. This
> is to detect
>
> *(&foo)) ()
> (s.*&S::foo) ()
>
> which were deemed ill-formed.
>
> gcc/cp/ChangeLog:
>
> * call.cc (build_over_call): Set ADDR_EXPR_DENOTES_CALL_P. Don't handle
> immediate_invocation_p here.
> * constexpr.cc (in_immediate_context): New overload.
> (cxx_eval_call_expression): Use mce_true for DECL_IMMEDIATE_FUNCTION_P.
> (cxx_eval_conditional_expression): Call cp_fold_immediate.
> * cp-gimplify.cc (maybe_replace_decl): Make static.
> (cp_fold_r): Expand immediate invocations.
> (cp_fold_immediate_r): New.
> (cp_fold_immediate): New.
> * cp-tree.h (ADDR_EXPR_DENOTES_CALL_P): Define.
> (cp_fold_immediate): Declare.
> * tree.cc (bot_replace): Don't handle immediate invocations here.
>
> libstdc++-v3/ChangeLog:
>
> * testsuite/20_util/allocator/105975.cc: Add dg-error.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp23/consteval-if2.C: Add xfail.
> * g++.dg/cpp2a/consteval-memfn1.C: Adjust.
> * g++.dg/cpp2a/consteval11.C: Remove dg-message.
> * g++.dg/cpp2a/consteval3.C: Remove dg-message and dg-error.
> * g++.dg/cpp2a/consteval9.C: Remove dg-message.
> * g++.dg/cpp2a/consteval32.C: New test.
> * g++.dg/cpp2a/consteval33.C: New test.
> * g++.dg/cpp2a/consteval34.C: New test.
> * g++.dg/cpp2a/consteval35.C: New test.
> ---
> gcc/cp/call.cc | 40 +------
> gcc/cp/constexpr.cc | 23 +++-
> gcc/cp/cp-gimplify.cc | 108 ++++++++++++++----
> gcc/cp/cp-tree.h | 42 ++++---
> gcc/cp/tree.cc | 23 +---
> gcc/testsuite/g++.dg/cpp23/consteval-if2.C | 3 +-
> gcc/testsuite/g++.dg/cpp2a/consteval-memfn1.C | 7 ++
> gcc/testsuite/g++.dg/cpp2a/consteval11.C | 33 +++---
> gcc/testsuite/g++.dg/cpp2a/consteval3.C | 3 +-
> gcc/testsuite/g++.dg/cpp2a/consteval32.C | 4 +
> gcc/testsuite/g++.dg/cpp2a/consteval33.C | 34 ++++++
> gcc/testsuite/g++.dg/cpp2a/consteval34.C | 18 +++
> gcc/testsuite/g++.dg/cpp2a/consteval35.C | 10 ++
> gcc/testsuite/g++.dg/cpp2a/consteval9.C | 3 +-
> .../testsuite/20_util/allocator/105975.cc | 2 +-
> 15 files changed, 234 insertions(+), 119 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/consteval32.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/consteval33.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/consteval34.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/consteval35.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 0ca4370deab..8c077aa22fe 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -3837,12 +3841,25 @@ cxx_eval_conditional_expression (const constexpr_ctx *ctx, tree t,
> boolean_type_node);
> }
> /* Don't VERIFY_CONSTANT the other operands. */
> - if (integer_zerop (val))
> + const bool zero_p = integer_zerop (val);
> + if (zero_p)
> val = TREE_OPERAND (t, 2);
> else
> val = TREE_OPERAND (t, 1);
> if (TREE_CODE (t) == IF_STMT && !val)
> val = void_node;
> +
> + if (!in_immediate_context ()
> + /* P2564: a subexpression of a manifestly constant-evaluated expression
> + or conversion is an immediate function context. */
> + && ctx->manifestly_const_eval != mce_true
I might check this first as a tiny optimization.
> + && cp_fold_immediate (&TREE_OPERAND (t, zero_p ? 1 : 2),
> + ctx->manifestly_const_eval))
> + {
> + *non_constant_p = true;
> + return t;
> + }
> +
> /* A TARGET_EXPR may be nested inside another TARGET_EXPR, but still
> serve as the initializer for the same object as the outer TARGET_EXPR,
> as in
> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> index 206e791fcfd..9901513461c 100644
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -1000,7 +1000,7 @@ cp_genericize_target_expr (tree *stmt_p)
> replacement when cp_folding TARGET_EXPR to preserve the invariant that
> AGGR_INIT_EXPR_SLOT agrees with the enclosing TARGET_EXPR_SLOT. */
>
> -bool
> +static bool
> maybe_replace_decl (tree *tp, tree decl, tree replacement)
> {
> if (!*tp || !VOID_TYPE_P (TREE_TYPE (*tp)))
> @@ -1029,44 +1029,73 @@ struct cp_genericize_data
> bool handle_invisiref_parm_p;
> };
>
> -/* Perform any pre-gimplification folding of C++ front end trees to
> - GENERIC.
> - Note: The folding of non-omp cases is something to move into
> - the middle-end. As for now we have most foldings only on GENERIC
> - in fold-const, we need to perform this before transformation to
> - GIMPLE-form. */
> +/* A subroutine of cp_fold_r to handle immediate functions. */
>
> static tree
> -cp_fold_r (tree *stmt_p, int *walk_subtrees, void *data_)
> +cp_fold_immediate_r (tree *stmt_p, int *walk_subtrees, void *data_)
> {
> - cp_fold_data *data = (cp_fold_data*)data_;
> + auto data = static_cast<cp_fold_data *>(data_);
> tree stmt = *stmt_p;
> - enum tree_code code = TREE_CODE (stmt);
> + const tsubst_flags_t complain = (data->flags == ff_none ? tf_none : tf_error);
>
> - switch (code)
> + /* No need to look into types or unevaluated operands.
> + NB: This affects cp_fold_r as well. */
> + if (TYPE_P (stmt) || unevaluated_p (TREE_CODE (stmt)))
> {
> + *walk_subtrees = 0;
> + return NULL_TREE;
> + }
> +
> + switch (TREE_CODE (stmt))
> + {
> + /* Unfortunately we must handle code like
> + false ? bar () : 42
> + where we have to check bar too. */
> + case COND_EXPR:
> + if (cp_fold_immediate_r (&TREE_OPERAND (stmt, 1), walk_subtrees, data))
> + return error_mark_node;
> + if (TREE_OPERAND (stmt, 2)
> + && cp_fold_immediate_r (&TREE_OPERAND (stmt, 2), walk_subtrees, data))
> + return error_mark_node;
Is this necessary? Doesn't walk_tree already walk into the arms of
COND_EXPR?
> + break;
> +
> case PTRMEM_CST:
> if (TREE_CODE (PTRMEM_CST_MEMBER (stmt)) == FUNCTION_DECL
> && DECL_IMMEDIATE_FUNCTION_P (PTRMEM_CST_MEMBER (stmt)))
> {
> - if (!data->pset.add (stmt))
> + if (!data->pset.add (stmt) && (complain & tf_error))
> 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));
It looks like this will overwrite *stmt_p even if we didn't give an error.
> - break;
> + return error_mark_node;
> }
> break;
>
> + /* Expand immediate invocations. */
> + case CALL_EXPR:
> + case AGGR_INIT_EXPR:
> + 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, complain);
Likewise.
> + if (stmt == error_mark_node)
> + return error_mark_node;
> + }
> + break;
> +
> case ADDR_EXPR:
> if (TREE_CODE (TREE_OPERAND (stmt, 0)) == FUNCTION_DECL
> && DECL_IMMEDIATE_FUNCTION_P (TREE_OPERAND (stmt, 0)))
> {
> - error_at (EXPR_LOCATION (stmt),
> - "taking address of an immediate function %qD",
> - TREE_OPERAND (stmt, 0));
> + if (complain & tf_error)
> + 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));
Likewise.
Jason
next prev parent reply other threads:[~2023-09-12 21:26 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-01 17:23 [PATCH] " Marek Polacek
2023-09-01 17:36 ` Marek Polacek
2023-09-05 14:52 ` Jason Merrill
2023-09-05 19:59 ` Marek Polacek
2023-09-05 20:36 ` Jason Merrill
2023-09-07 15:23 ` [PATCH v2] " Marek Polacek
2023-09-07 18:32 ` Jason Merrill
2023-09-08 18:24 ` [PATCH v3] " Marek Polacek
2023-09-12 21:26 ` Jason Merrill [this message]
2023-09-13 20:56 ` [PATCH v4] " Marek Polacek
2023-09-13 21:57 ` Jason Merrill
2023-09-14 0:02 ` [PATCH v5] " Marek Polacek
2023-09-15 18:08 ` Jason Merrill
2023-09-15 20:32 ` [PATCH v6] " Marek Polacek
2023-09-16 20:22 ` Jason Merrill
2023-09-18 21:42 ` [PATCH v7] " Marek Polacek
2023-09-19 1:36 ` Jason Merrill
2023-09-19 13:01 ` Marek Polacek
2023-09-19 13:20 ` Jason Merrill
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=b694a250-45a5-19bc-c905-49df0fbc3c56@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=polacek@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).