From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 3A80B3857800 for ; Wed, 15 Sep 2021 19:50:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3A80B3857800 Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-581--kBAdQCrN_Wpc3FdF6OL-Q-1; Wed, 15 Sep 2021 15:50:45 -0400 X-MC-Unique: -kBAdQCrN_Wpc3FdF6OL-Q-1 Received: by mail-qv1-f69.google.com with SMTP id l18-20020a056214039200b0037e4da8b408so4874668qvy.6 for ; Wed, 15 Sep 2021 12:50:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=hpvVTJxvp4oCm3yahKXkv0TH4RwTI/gkd+dvyzXdqno=; b=BTj5sJHS//3irlP+Jt3hQMSD18YNwtJd+1G329AVsXuSlE/O99ibbqy3xyoaM18wOw wi9ZXHBWjsxD7yQrPLwrrAwevU50O1zqDcRDRSUBLFb0NcQPsHbGTo5365wM+BCazQf3 7Pojpqx/pUon5w0dm4aRPVDVJx3RtsTchhPDj2TIfP2bW6k1+TE7ATQJQtwK0cyrBN5F mHZQekHjFx/ewql4Nt6yliKVWH05EIsnmrSYtmLAxOaqX484Po0wq99rmjeTnhq3lmui hCZfTjgf+lDT3PBi6gODVXX1P72M+JDFYjVlgbZEnP2gsYWe01zEHINctGsc25faNIRM XRvw== X-Gm-Message-State: AOAM5317Fqty/icxtT5JMtXtQQYn9nHLJi6ejOU4X/+1JqbZhiAkG+5o jdBHmA1nCkAxIhC2eVjyqaBZE3LHQC2wkfPatcy0KbefgTLrA7Jjd6O0ZqMayW0IYVpah39znpd R/RKUP1ooCZSw1iCHXQ== X-Received: by 2002:a05:620a:44c9:: with SMTP id y9mr1677823qkp.279.1631735444112; Wed, 15 Sep 2021 12:50:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzJ3nr619xdRz3bkFvbZBgwdyQIU0Ni4sRcgdANzNI4yfpWIQkxeWeia3jJceUV1Te0GygsYg== X-Received: by 2002:a05:620a:44c9:: with SMTP id y9mr1677798qkp.279.1631735443842; Wed, 15 Sep 2021 12:50:43 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id c67sm712818qke.113.2021.09.15.12.50.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Sep 2021 12:50:43 -0700 (PDT) Message-ID: <29bf5cd7-a74c-87e1-a4d4-4e5cc272d87e@redhat.com> Date: Wed, 15 Sep 2021 15:50:42 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 Subject: Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC]. To: Iain Sandoe Cc: GCC Patches References: <732D8CCD-ABAE-43EB-B73C-D32E4553D84F@sandoe.co.uk> <58a39ec9-cb56-c089-eaf2-3d43f317b272@redhat.com> <4233989E-BE46-48EC-A2D2-D541A5865B81@sandoe.co.uk> From: Jason Merrill In-Reply-To: <4233989E-BE46-48EC-A2D2-D541A5865B81@sandoe.co.uk> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, CTE_8BIT_MISMATCH, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Sep 2021 19:50:48 -0000 On 9/15/21 14:32, Iain Sandoe wrote: > Hi Jason, > >> On 15 Sep 2021, at 18:32, Jason Merrill wrote: >> >> On 9/14/21 11:36, Iain Sandoe wrote: >>> Hi >>> Some small code cleanups that allow us to have just one place that >>> we handle a statement with await expression(s) embedded. Also we >>> can reduce the work done to figure out whether a statement contains >>> any such expressions. >>> tested on x86_64,powerpc64le-linux x86_64-darwin >>> OK for master? >>> thanks >>> Iain >>> ----- >>> There is no need to make a MODIFY_EXPR for any of the condition >>> vars that we synthesize. >>> Expansion of co_return can be carried out independently of any >>> co_awaits that might be contained which simplifies this. >>> Where we are rewriting statements to handle await expression >>> logic, there is no need to carry out any analysis - we just need >>> to detect the presence of any co_await. >>> Signed-off-by: Iain Sandoe >>> gcc/cp/ChangeLog: >>> * coroutines.cc (await_statement_walker): Code cleanups. >>> --- >>> gcc/cp/coroutines.cc | 121 ++++++++++++++++++++----------------------- >>> 1 file changed, 56 insertions(+), 65 deletions(-) >>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>> index d2cc2e73c89..27556723b71 100644 >>> --- a/gcc/cp/coroutines.cc >>> +++ b/gcc/cp/coroutines.cc >>> @@ -3412,16 +3412,11 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) >>> return NULL_TREE; >>> } >>> - /* We have something to be handled as a single statement. */ >>> - bool has_cleanup_wrapper = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR; >>> - hash_set visited; >>> - awpts->saw_awaits = 0; >>> - hash_set truth_aoif_to_expand; >>> - awpts->truth_aoif_to_expand = &truth_aoif_to_expand; >>> - awpts->needs_truth_if_exp = false; >>> - awpts->has_awaiter_init = false; >>> + /* We have something to be handled as a single statement. We have to handle >>> + a few statements specially where await statements have to be moved out of >>> + constructs. */ >>> tree expr = *stmt; >>> - if (has_cleanup_wrapper) >>> + if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR) >>> expr = TREE_OPERAND (expr, 0); >>> STRIP_NOPS (expr); >>> @@ -3437,6 +3432,8 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) >>> transforms can be implemented. */ >>> case IF_STMT: >>> { >>> + tree *await_ptr; >>> + hash_set visited; >>> /* Transform 'if (cond with awaits) then stmt1 else stmt2' into >>> bool cond = cond with awaits. >>> if (cond) then stmt1 else stmt2. */ >>> @@ -3444,10 +3441,8 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) >>> /* We treat the condition as if it was a stand-alone statement, >>> to see if there are any await expressions which will be analyzed >>> and registered. */ >>> - if ((res = cp_walk_tree (&IF_COND (if_stmt), >>> - analyze_expression_awaits, d, &visited))) >>> - return res; >>> - if (!awpts->saw_awaits) >>> + if (!(cp_walk_tree (&IF_COND (if_stmt), >>> + find_any_await, &await_ptr, &visited))) >>> return NULL_TREE; /* Nothing special to do here. */ >>> gcc_checking_assert (!awpts->bind_stack->is_empty()); >>> @@ -3463,7 +3458,7 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) >>> /* We want to initialize the new variable with the expression >>> that contains the await(s) and potentially also needs to >>> have truth_if expressions expanded. */ >>> - tree new_s = build2_loc (sloc, MODIFY_EXPR, boolean_type_node, >>> + tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node, >>> newvar, cond_inner); >>> finish_expr_stmt (new_s); >>> IF_COND (if_stmt) = newvar; >>> @@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) >>> break; >>> case FOR_STMT: >>> { >>> + tree *await_ptr; >>> + hash_set visited; >>> /* for loops only need special treatment if the condition or the >>> iteration expression contain a co_await. */ >>> tree for_stmt = *stmt; >>> /* Sanity check. */ >>> - if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt), >>> - analyze_expression_awaits, d, &visited))) >>> - return res; >>> - gcc_checking_assert (!awpts->saw_awaits); >>> - >>> - if ((res = cp_walk_tree (&FOR_COND (for_stmt), >>> - analyze_expression_awaits, d, &visited))) >>> - return res; >>> - bool for_cond_await = awpts->saw_awaits != 0; >>> - unsigned save_awaits = awpts->saw_awaits; >>> - >>> - if ((res = cp_walk_tree (&FOR_EXPR (for_stmt), >>> - analyze_expression_awaits, d, &visited))) >>> - return res; >>> - bool for_expr_await = awpts->saw_awaits > save_awaits; >>> + gcc_checking_assert >>> + (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await, >>> + &await_ptr, &visited))); >> >> What's the rationale for this assert? [expr.await] seems to say explicitly that an await can appear in the initializer of an init-statement. > > Indeed (and we would not expect otherwise) > - but currently GCC appears to generate code for: > > for (loop_ind_var = init; … ; …) {} > > that looks like: > > loop_ind_var = init; > for (; … ; …) {} > > If that changes (and the init contains an await expr) then we’d need to apply that transform manually, so the assert is in place to check that the assumption about existing behaviour is met. Then the patch is OK with that rationale in a comment. Jason