Re: [PATCH v4] c++: implement P2564, consteval needs to propagate up [PR107687]
On 11/6/23 17:34, Marek Polacek wrote:
On Fri, Nov 03, 2023 at 01:51:07PM -0400, Jason Merrill wrote:
On 11/2/23 11:28, Marek Polacek wrote:
On Sat, Oct 14, 2023 at 12:56:11AM -0400, Jason Merrill wrote:
On 10/10/23 13:20, Marek Polacek wrote:
I suppose some
functions cannot possibly be promoted because they don't contain
any CALL_EXPRs. So we may be able to rule them out while doing
cp_fold_r early.
Yes. Or, the only immediate-escalating functions referenced have already
been checked.
It looks like you haven't pursued this yet? One implementation thought:
Oops, I'd forgotten to address that.
maybe_store_cfun... could stop skipping immediate_escalating_function_p
(current_function_decl), and after we're done folding if the current
function isn't in the hash_set we can go ahead and set
DECL_ESCALATION_CHECKED_P?
Clever, I see what you mean. IOW, we store c_f_d iff the function contains
an i-e expr. If not, it can't possibly become consteval. I've added that
into cp_fold_function, and it seems to work well...
...except it revealed a different problem: cp_fold_r -> cp_fold will, since
https://gcc.gnu.org/pipermail/gcc-patches/2016-March/443993.html, remove
UNARY_PLUS_EXPR, leading us into this problem:
// stmt = +id(i)
cp_fold (...);
// stmt = id(i)
and the subsequent tree walk walks the CALL_EXPR's operands, so
cp_fold_immediate_r will never see the CALL_EXPR, so we miss an i-e expr.
Perhaps a better solution than the kludge I added would be to only call
cp_fold_immediate_r after cp_fold. Or potentially before /and/ after if
cp_fold changes the expression?
Or walk everything with cp_fold_immediate_r before walking again with
cp_fold_r?
It also seems odd that the ADDR_EXPR case calls vec_safe_push
(deferred_escalating_exprs, while the CALL_EXPR case calls
maybe_store_cfun_for_late_checking, why the different handling?
maybe_store_cfun_for_late_checking saves current_function_decl
so that we can check:
void g (int i) {
fn (i); // error if fn promotes to consteval
}
Yes, but why don't we want the same handling for ADDR_EXPR?
The handling can't be exactly the same due to global vars like
auto p1 = ;
...but it's wrong to only save the ADDR_EXPR if it's enclosed in
a function, because the ADDR_EXPR could be inside a consteval if
block, in which case I think we're not supposed to error. Tested
in consteval-prop20.C. Thanks,
And we don't need the !current_function_decl handling for CALL_EXPR?
The only significant difference I see between and f() for escalation
is that the latter might be an immediate invocation. Once we've
determined that it's not, so we are in fact looking at an
immediate-escalating expression, I'd expect the promotion handling to be
identical.
+ /* Whether cp_fold_immediate_r is looking for immediate-escalating
+ expressions. */
Isn't that always what it's doing?
The uses of ff_escalating in maybe_explain_promoted_consteval and
maybe_escalate_decl_and_cfun seem to have different purposes that I'm
having trouble following.
For the former, it seems to control returning the offending expression
rather than error_mark_node. Why don't we always do that?
For the latter, it seems to control recursion, which seems redundant
with the recursion in that latter function itself. And the use of the
flag seems redundant with at_eof.
+/* Remember that the current function declaration contains a call to
+ a function that might be promoted to consteval later. */
+
+static void
+maybe_store_cfun_for_late_checking ()
This name could say more about escalation? Maybe
...for_escalation_checking?
Or, better, merge this with maybe_store_immediate_escalating_fn?
+/* Figure out if DECL should be promoted to consteval and if so, maybe also
+ promote the function we are in currently. CALL is the CALL_EXPR of DECL.
+ EVALP is where we may store the result of cxx_constant_value so that we
+ don't have to evaluate the same tree again in cp_fold_immediate_r. */
+
+static void
+maybe_escalate_decl_and_cfun (tree decl, tree call, tree *evalp)
+{
+ if (cp_unevaluated_operand)
+return;
+
+ /* What we're calling is not a consteval function but it may become
+ one. This requires recursing; DECL may be promoted to consteval
+ because it contains an escalating expression E, but E itself may
+ have to be promoted first, etc. */
+ if (unchecked_immediate_escalating_function_p (decl))
+{
+ cp_fold_data data (ff_escalating, decl);
+ cp_walk_tree (_SAVED_TREE (decl), cp_fold_immediate_r,
+ , nullptr);
+ DECL_ESCALATION_CHECKED_P (decl) = true;
Why recurse both here and in cp_fold_immediate_r?
+}
+
+ /* In turn, maybe promote the function we find ourselves in... */
+ if (DECL_IMMEDIATE_FUNCTION_P (decl)
+ /* ...but not if the call to DECL was constant; that is the
+"an immediate invocation that is not a constant expression"
+case. We do this
[PATCH v4] c++: implement P2564, consteval needs to propagate up [PR107687]
On Fri, Nov 03, 2023 at 01:51:07PM -0400, Jason Merrill wrote:
> On 11/2/23 11:28, Marek Polacek wrote:
> > On Sat, Oct 14, 2023 at 12:56:11AM -0400, Jason Merrill wrote:
> > > On 10/10/23 13:20, Marek Polacek wrote:
> > > > On Tue, Aug 29, 2023 at 03:26:46PM -0400, Jason Merrill wrote:
> > > > > On 8/23/23 15:49, Marek Polacek wrote:
> > > > > > +struct A {
> > > > > > + int x;
> > > > > > + int y = id(x);
> > > > > > +};
> > > > > > +
> > > > > > +template
> > > > > > +constexpr int k(int) { // k is not an immediate
> > > > > > function because A(42) is a
> > > > > > + return A(42).y; // constant expression and thus
> > > > > > not immediate-escalating
> > > > > > +}
> > > > >
> > > > > Needs use(s) of k to test the comment.
> > > >
> > > > True, and that revealed what I think is a bug in the standard.
> > > > In the test I'm saying:
> > > >
> > > > // ??? [expr.const]#example-9 says:
> > > > // k is not an immediate function because A(42) is a
> > > > // constant expression and thus not immediate-escalating
> > > > // But I think the call to id(x) is *not* a constant expression and thus
> > > > // it is an immediate-escalating expression. Therefore k *is*
> > > > // an immediate function. So we get the error below. clang++ agrees.
> > > > id(x) is not a constant expression because x isn't constant.
> > >
> > > Not when considering id(x) by itself, but in the evaluation of A(42), the
> > > member x has just been initialized to constant 42. And A(42) is
> > > constant-evaluated because "An aggregate initialization is an immediate
> > > invocation if it evaluates a default member initializer that has a
> > > subexpression that is an immediate-escalating expression."
> > >
> > > I assume clang doesn't handle this passage properly yet because it was
> > > added
> > > during core review of the paper, for parity between aggregate
> > > initialization
> > > and constructor escalation.
> > >
> > > This can be a follow-up patch.
> >
> > I see. So the fix will be to, for the aggregate initialization case, pass
> > the whole A(42).y thing to cxx_constant_eval, not just id(x).
>
> Well, A(42) is the immediate invocation, the .y is not part of it.
Ah right, like the comment above says.
> > > > I suppose some
> > > > functions cannot possibly be promoted because they don't contain
> > > > any CALL_EXPRs. So we may be able to rule them out while doing
> > > > cp_fold_r early.
> > >
> > > Yes. Or, the only immediate-escalating functions referenced have already
> > > been checked.
>
> It looks like you haven't pursued this yet? One implementation thought:
Oops, I'd forgotten to address that.
> maybe_store_cfun... could stop skipping immediate_escalating_function_p
> (current_function_decl), and after we're done folding if the current
> function isn't in the hash_set we can go ahead and set
> DECL_ESCALATION_CHECKED_P?
Clever, I see what you mean. IOW, we store c_f_d iff the function contains
an i-e expr. If not, it can't possibly become consteval. I've added that
into cp_fold_function, and it seems to work well...
...except it revealed a different problem: cp_fold_r -> cp_fold will, since
https://gcc.gnu.org/pipermail/gcc-patches/2016-March/443993.html, remove
UNARY_PLUS_EXPR, leading us into this problem:
// stmt = +id(i)
cp_fold (...);
// stmt = id(i)
and the subsequent tree walk walks the CALL_EXPR's operands, so
cp_fold_immediate_r will never see the CALL_EXPR, so we miss an i-e expr.
Perhaps a better solution than the kludge I added would be to only call
cp_fold_immediate_r after cp_fold. Or potentially before /and/ after if
cp_fold changes the expression?
> > > We can also do some escalation during constexpr evaluation: all the
> > > functions involved need to be instantiated for the evaluation, and if we
> > > encounter an immediate-escalating expression while evaluating a call to an
> > > immediate-escalating function, we can promote it then. Though we can't
> > > necessarily mark it as not needing promotion, as there might be i-e exprs
> > > in
> > > branches that the particular evaluation doesn't take.
> >
> > I've tried but I didn't get anywhere. The patch was basically
> >
> > --- a/gcc/cp/constexpr.cc
> > +++ b/gcc/cp/constexpr.cc
> > @@ -2983,7 +2983,13 @@ cxx_eval_call_expression (const constexpr_ctx *ctx,
> > tree t,
> > } fb (new_call.bindings);
> >
> > if (*non_constant_p)
> > -return t;
> > +{
> > + if (cxx_dialect >= cxx20
> > + && ctx->manifestly_const_eval == mce_false
> > + && DECL_IMMEDIATE_FUNCTION_P (fun))
> > + maybe_promote_function_to_consteval (current_function_decl);
> > + return t;
> > +}
> >
> > /* We can't defer instantiating the function any longer. */
> > if (!DECL_INITIAL (fun)
> >
> > but since I have to check mce_false, it didn't do anything useful
> > in practice (that is, it wouldn't escalate anything in my tests).
>
> Makes sense.
>
> > > > @@ -55,6
