From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id 1C0A0385DC37 for ; Wed, 31 Jan 2024 19:55:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1C0A0385DC37 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1C0A0385DC37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:470:142:3::10 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706730917; cv=none; b=GxUSYdmagt7TMzaULGW0u5r1Ds4QYJlbc677DbDU223X+DP78q7RAwykOL7CsZsij2xvKJCfsc0MYUdlL3Ir1RVAq7JJM2MyKos1q5IIUpp3ac7eGEeHxvhV/bh7HFGgG71WEuTMmZ6SG7cBxpSZk1KSm5zY3XDcMKIrDtt6ER0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706730917; c=relaxed/simple; bh=o01Ch9sPBwNLkobjrkSlsii5WPZZ2vPBZJQpPZaTJN0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=AKaU00i7HfwECpXA9Jf9/ph+LrkzHisYSwLOU3h8BObbPyCokuk7gXW6fxYYMaGkvVKCDaObStvCm4LDDdB6MrK3+eJL82dqbYd+QM9JD9gcNz6OymfrhCY6cMRgmiRDgivHoPVIo/qo8KE8oDVJGqpZAYGm6MtQmpYS9sR16DM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rVGfw-0001wO-1A for gcc-patches@gnu.org; Wed, 31 Jan 2024 14:55:13 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706730910; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=l4WMII0I6eIqykf+OB4vdVUdulhv/1i/XnqLHWjP0Wk=; b=B5kFdQLfgaBlJAjtaVzMhAK2Ozld5q6mWaMJIbbKLs8EizdD7fmrekPWRfFSRmWgZpsksY FWxrXgUV1Msfu8MZPf5BjD/rpGfD9pWc55/ba7OaTI0528+BGkzaHBHPM/rIuL+gJdz2Vj 1viYalS9Oe62+Bsiu5Bxu9hdYsit0/M= Received: from mail-ot1-f70.google.com (mail-ot1-f70.google.com [209.85.210.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-684-BFm4628xOmeolHvP7tC5dg-1; Wed, 31 Jan 2024 14:55:08 -0500 X-MC-Unique: BFm4628xOmeolHvP7tC5dg-1 Received: by mail-ot1-f70.google.com with SMTP id 46e09a7af769-6e119f68b24so130143a34.3 for ; Wed, 31 Jan 2024 11:55:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706730907; x=1707335707; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=l4WMII0I6eIqykf+OB4vdVUdulhv/1i/XnqLHWjP0Wk=; b=ZfrRgn9aX5KrCKdwXRnbUHhHgoFFohBnSFRbQIQrpv4iJ/X8zZhPxGxP1F26gZ6Lv/ cnSPNQxjbF8p23/DLQpp4G2Dv4UrpZ92a8r+CZRIId/WZ7CL8VOqXgv9c3PcnCA2l6l9 v8B9CvV7QEk4J76zkVORQN8BkKhyXL8x3hCnlbE9IbOtMelLvnOP6TpZjkakhpU/P9z/ bmQKHmSJcvif+2KhfaNXBYzSq9x/NcpSR18rq99HAI1sNIGn6tKfuQHI3aYl0qw2RfZY MXouKxhzXZO/6pbOZ5rhUXsjJvTLCEgJvYYYOYeUbcgpVfQfxWPT6drAGrQt3xwzl5dm 6d/Q== X-Gm-Message-State: AOJu0YybAAavOkK+WJYvB3nOx4wSibDQvjTVoIikDJ/ASbhU2jbdt7Ke E/la1dlwiD9z1pLVtzxie2TjI0/hJX8WlrYIU4I2Mf2ILjbdF5BMLt+KJFHAsVSrUaxy9iqWwjE eZMuAO4GubjMRxCFNRfa3t1GIjIKAGH3Kps46NCAx2JT1QA5sUQ== X-Received: by 2002:a05:6830:1e64:b0:6e0:c467:aab3 with SMTP id m4-20020a0568301e6400b006e0c467aab3mr187259otr.15.1706730907359; Wed, 31 Jan 2024 11:55:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IHfjXeK8LXz/0QClC8zqSyXYmNBLHYwli4rF8emLalj/M/W88yvEYa4AWUFh8Yk92akApaMrw== X-Received: by 2002:a05:6830:1e64:b0:6e0:c467:aab3 with SMTP id m4-20020a0568301e6400b006e0c467aab3mr187239otr.15.1706730906984; Wed, 31 Jan 2024 11:55:06 -0800 (PST) Received: from redhat.com (2603-7000-9500-34a5-0000-0000-0000-1db4.res6.spectrum.com. [2603:7000:9500:34a5::1db4]) by smtp.gmail.com with ESMTPSA id hh10-20020a05622a618a00b00428346b88bfsm5812458qtb.65.2024.01.31.11.55.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 31 Jan 2024 11:55:06 -0800 (PST) Date: Wed, 31 Jan 2024 14:55:04 -0500 From: Marek Polacek To: Andi Kleen Cc: gcc-patches@gnu.org Subject: Re: [PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324) Message-ID: References: <20240131021808.151575-1-ak@linux.intel.com> <20240131021808.151575-3-ak@linux.intel.com> MIME-Version: 1.0 In-Reply-To: <20240131021808.151575-3-ak@linux.intel.com> User-Agent: Mutt/2.2.12 (2023-09-09) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Received-SPF: pass client-ip=170.10.133.124; envelope-from=polacek@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9,DKIMWL_WL_HIGH=-1.292,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DKIM_VALID_EF=-0.1,RCVD_IN_DNSWL_NONE=-0.0001,RCVD_IN_MSPIKE_H5=0.001,RCVD_IN_MSPIKE_WL=0.001,RCVD_IN_SORBS_WEB=1.5,SPF_HELO_NONE=0.001,SPF_PASS=-0.001,T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_SORBS_WEB,SPF_HELO_PASS,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jan 30, 2024 at 06:17:15PM -0800, Andi Kleen wrote: > This patch implements a clang compatible [[musttail]] attribute for > returns. > > musttail is useful as an alternative to computed goto for interpreters. > With computed goto the interpreter function usually ends up very big > which causes problems with register allocation and other per function > optimizations not scaling. With musttail the interpreter can be instead > written as a sequence of smaller functions that call each other. To > avoid unbounded stack growth this requires forcing a sibling call, which > this attribute does. It guarantees an error if the call cannot be tail > called which allows the programmer to fix it instead of risking a stack > overflow. Unlike computed goto it is also type-safe. > > It turns out that David Malcolm had already implemented middle/backend > support for a musttail attribute back in 2016, but it wasn't exposed > to any frontend other than a special plugin. > > This patch adds a [[gnu::musttail]] attribute for C++ that can be added > to return statements. The return statement must be a direct call > (it does not follow dependencies), which is similar to what clang > implements. It then uses the existing must tail infrastructure. > > For compatibility it also detects clang::musttail FWIW, it's not clear to me we should do this. I don't see a precedent. > One problem is that tree-tailcall usually fails when optimization > is disabled, which implies the attribute only really works with > optimization on. But that seems to be a reasonable limitation. > > Passes bootstrap and full test I don't see a ChangeLog entry. > --- > gcc/cp/cp-tree.h | 4 ++-- > gcc/cp/parser.cc | 28 +++++++++++++++++++++++----- > gcc/cp/semantics.cc | 6 +++--- > gcc/cp/typeck.cc | 20 ++++++++++++++++++-- > 4 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 60e6dafc5494..bed52e860a00 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7763,7 +7763,7 @@ extern void finish_while_stmt (tree); > extern tree begin_do_stmt (void); > extern void finish_do_body (tree); > extern void finish_do_stmt (tree, tree, bool, tree, bool); > -extern tree finish_return_stmt (tree); > +extern tree finish_return_stmt (tree, bool = false); > extern tree begin_for_scope (tree *); > extern tree begin_for_stmt (tree, tree); > extern void finish_init_stmt (tree); > @@ -8275,7 +8275,7 @@ extern tree composite_pointer_type (const op_location_t &, > tsubst_flags_t); > extern tree merge_types (tree, tree); > extern tree strip_array_domain (tree); > -extern tree check_return_expr (tree, bool *, bool *); > +extern tree check_return_expr (tree, bool *, bool *, bool); > extern tree spaceship_type (tree, tsubst_flags_t = tf_warning_or_error); > extern tree genericize_spaceship (location_t, tree, tree, tree); > extern tree cp_build_binary_op (const op_location_t &, > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index 3748ccd49ff3..5a32804c0201 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -2462,7 +2462,7 @@ static tree cp_parser_perform_range_for_lookup > static tree cp_parser_range_for_member_function > (tree, tree); > static tree cp_parser_jump_statement > - (cp_parser *); > + (cp_parser *, bool = false); > static void cp_parser_declaration_statement > (cp_parser *); > > @@ -12719,9 +12719,27 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr, > NULL_TREE, false); > break; > > + case RID_RETURN: > + { > + bool musttail_p = false; > + std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc); > + if (lookup_attribute ("", "musttail", std_attrs)) > + { > + musttail_p = true; > + std_attrs = remove_attribute ("", "musttail", std_attrs); > + } > + // support this for compatibility > + if (lookup_attribute ("clang", "musttail", std_attrs)) > + { > + musttail_p = true; > + std_attrs = remove_attribute ("clang", "musttail", std_attrs); > + } Doing lookup_attribute unconditionally twice seems like a lot. You could do just lookup_attribute ("musttail", std_attrs) and then check get_attribute_namespace() == nullptr/gnu_identifier? It's not pretty that you have to remove_attribute but I guess we emit warnings otherwise? > + statement = cp_parser_jump_statement (parser, musttail_p); > + } > + break; > + > case RID_BREAK: > case RID_CONTINUE: > - case RID_RETURN: > case RID_CO_RETURN: > case RID_GOTO: > std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc); > @@ -14767,7 +14785,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl) > return false; > } > > -/* Parse a jump-statement. > +/* Parse a jump-statement. MUSTTAIL_P indicates a musttail attribute. Two spaces after a '.'. > > jump-statement: > break ; > @@ -14785,7 +14803,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl) > Returns the new BREAK_STMT, CONTINUE_STMT, RETURN_EXPR, or GOTO_EXPR. */ > > static tree > -cp_parser_jump_statement (cp_parser* parser) > +cp_parser_jump_statement (cp_parser* parser, bool musttail_p) > { > tree statement = error_mark_node; > cp_token *token; > @@ -14869,7 +14887,7 @@ cp_parser_jump_statement (cp_parser* parser) > else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt) > /* Don't deduce from a discarded return statement. */; > else > - statement = finish_return_stmt (expr); > + statement = finish_return_stmt (expr, musttail_p); > /* Look for the final `;'. */ > cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); > } > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 3299e2704465..a277f70ea0fd 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -1324,16 +1324,16 @@ finish_do_stmt (tree cond, tree do_stmt, bool ivdep, tree unroll, > } > > /* Finish a return-statement. The EXPRESSION returned, if any, is as > - indicated. */ > + indicated. MUSTTAIL_P indicates a mustcall attribute. */ > > tree > -finish_return_stmt (tree expr) > +finish_return_stmt (tree expr, bool musttail_p) > { > tree r; > bool no_warning; > bool dangling; > > - expr = check_return_expr (expr, &no_warning, &dangling); > + expr = check_return_expr (expr, &no_warning, &dangling, musttail_p); > > if (error_operand_p (expr) > || (flag_openmp && !check_omp_return ())) > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index a15eda3f5f8c..8c116e3b4f4c 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -11028,10 +11028,12 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p) > the DECL_RESULT for the function. Set *NO_WARNING to true if > code reaches end of non-void function warning shouldn't be issued > on this RETURN_EXPR. Set *DANGLING to true if code returns the > - address of a local variable. */ > + address of a local variable. MUSTTAIL_P indicates a musttail > + return. */ > > tree > -check_return_expr (tree retval, bool *no_warning, bool *dangling) > +check_return_expr (tree retval, bool *no_warning, bool *dangling, > + bool musttail_p) > { > tree result; > /* The type actually returned by the function. */ > @@ -11045,6 +11047,20 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > *no_warning = false; > *dangling = false; > > + if (musttail_p) > + { > + if (TREE_CODE (retval) == TARGET_EXPR > + && TREE_CODE (TARGET_EXPR_INITIAL (retval)) == CALL_EXPR) > + CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_INITIAL (retval)) = 1; > + else if (TREE_CODE (retval) != CALL_EXPR) > + { This won't handle [[gnu::musttail]] return (1, foo ()); which I guess is fine since clang++ doesn't accept that either, but musttail-invalid.c doesn't check the case (and it's C only). > + error_at (loc, "cannot tail-call: return value must be a call"); > + return error_mark_node; > + } > + else > + CALL_EXPR_MUST_TAIL_CALL (retval) = 1; > + } IMHO nicer it would be to do tree t = retval; if (TREE_CODE (t) == TARGET_EXPR) t = TARGET_EXPR_INITIAL (t); if (TREE_CODE (t) == CALL_EXPR) CALL_EXPR_MUST_TAIL_CALL (t) = true; else { error_at (loc, "cannot tail-call: return value must be a call"); return error_mark_node; } so that you can set CALL_EXPR_MUST_TAIL_CALL in one place. Marek