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 6741B385842D for ; Tue, 7 Sep 2021 13:23:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6741B385842D Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-177-bAaugvD6OcCZgXvwfIIiLA-1; Tue, 07 Sep 2021 09:23:21 -0400 X-MC-Unique: bAaugvD6OcCZgXvwfIIiLA-1 Received: by mail-wr1-f70.google.com with SMTP id p10-20020a5d68ca000000b001552bf8b9daso2104634wrw.22 for ; Tue, 07 Sep 2021 06:23:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=kCC6hNNS75Y4F3MDrsakzrYZb1RsE2Khc0Ww4yQCWLU=; b=b6MP3orADEnvSuvXbxER6KpzYALURv7CILpNILM4r5h5xDIQ/oOvRsLlubwFmRz2KS 6aBsisj0TAa5P7mfYgnj9uaWXI+x9ZCUosajDf1E62CuhcjNmSdJxA0gGSy9dY8LWV+c 1KCYPS+3jZc4L6ZH/lFN/TVSsXvfI57LbYHnBKfzYxqgU+oxPK57xbbPhJiVon6VMDi8 SC2YLFkssE6PrU9NKwY/lTBu2qVwPnBF4FIVkmL+LrMmWTQ5j5NWolYqp+TKJHJu/qfv eWxPB3wP7J+KKEueuF1DAekiWSMkNFACYja7+22LRHU9izW+gvsNgtSD0uE4lKkQ9xBD 0Krg== X-Gm-Message-State: AOAM5319lJKyCF55099obPoVESEMilZSQnVtGUvVIlLB4v9UxktniqLw 6U91tHXTokFvoZY/ms8lWWaO1/84tcKoxsX/3nnWlHKEOgxK7F9l7LM/6yy3tcqwqiXGatMwUcV DVAGE752VS2av0yDq0g== X-Received: by 2002:adf:f84d:: with SMTP id d13mr19123578wrq.292.1631021000657; Tue, 07 Sep 2021 06:23:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJycaeUONZqBNJbnb7lsopqs8fqxsut+T+YWucQQ2qUOtxA90Dn5wGAyY685H0ON3FsMqxWxuA== X-Received: by 2002:adf:f84d:: with SMTP id d13mr19123553wrq.292.1631021000459; Tue, 07 Sep 2021 06:23:20 -0700 (PDT) Received: from abulafia.quesejoda.com ([139.47.33.227]) by smtp.gmail.com with ESMTPSA id 19sm2358801wmo.39.2021.09.07.06.23.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Sep 2021 06:23:19 -0700 (PDT) Subject: Re: [PATCH] Abstract PHI and forwarder block checks in jump threader. To: Richard Biener Cc: GCC patches , Jeff Law References: <20210903135832.735852-1-aldyh@redhat.com> <6D546CE0-4156-4C8F-B407-E7DCEA13FBF4@gmail.com> From: Aldy Hernandez Message-ID: <2e8290b5-bd89-1fb6-5bb2-bbd133c2d251@redhat.com> Date: Tue, 7 Sep 2021 15:23:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <6D546CE0-4156-4C8F-B407-E7DCEA13FBF4@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="------------A290328857884714260E97A8" Content-Language: en-US X-Spam-Status: No, score=-13.2 required=5.0 tests=BAYES_00, 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: Tue, 07 Sep 2021 13:23:24 -0000 This is a multi-part message in MIME format. --------------A290328857884714260E97A8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 9/7/21 2:59 PM, Richard Biener wrote: > On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez wrote: >> >> >> On 9/6/21 9:19 AM, Richard Biener wrote: >>> On Fri, Sep 3, 2021 at 3:59 PM Aldy Hernandez via Gcc-patches >>> wrote: >>>> >>>> This patch abstracts out a couple common idioms in the forward >>>> threader that I found useful while navigating the code base. >>>> >>>> Tested on x86-64 Linux. >>>> >>>> OK? >>>> >>>> gcc/ChangeLog: >>>> >>>> * tree-ssa-threadedge.c (has_phis_p): New. >>>> (forwarder_block_p): New. >>>> (potentially_threadable_block): Call forwarder_block_p. >>>> (jump_threader::thread_around_empty_blocks): Call has_phis_p. >>>> (jump_threader::thread_through_normal_block): Call >>>> forwarder_block_p. >>>> --- >>>> gcc/tree-ssa-threadedge.c | 25 +++++++++++++++++++------ >>>> 1 file changed, 19 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c >>>> index e57f6d3e39c..3db54a199fd 100644 >>>> --- a/gcc/tree-ssa-threadedge.c >>>> +++ b/gcc/tree-ssa-threadedge.c >>>> @@ -95,6 +95,21 @@ jump_threader::thread_through_all_blocks (bool may_peel_loop_headers) >>>> return m_registry->thread_through_all_blocks (may_peel_loop_headers); >>>> } >>>> >>>> +static inline bool >>>> +has_phis_p (basic_block bb) >>>> +{ >>>> + return !gsi_end_p (gsi_start_phis (bb)); >>> >>> gimple_seq_empty_p (phi_nodes (bb)) shoud be cheaper. Do virtual PHIs >>> count as PHIs for you? >> >> I don't know. The goal was to abstract some common idioms without >> changing existing behavior, but if my abstractions confuse other >> readers, perhaps I should revert my patch. >> >> FWIW, my initial motivation here was to merge the path profitability >> code between the forward and backward threaders. It seems the forward >> threader is more permissive than the backward threader, even though the >> latter can thread more paths than it's allowed (per profitable_path_p). >> >>> >>>> +} >>>> + >>>> +/* Return TRUE for a forwarder block which is defined as having PHIs >>>> + but no instructions. */ >>>> + >>>> +static bool >>>> +forwarder_block_p (basic_block bb) >>> >>> There exists a function with exactly the same signature in cfgrtl.h, likewise >>> several similar implementations might exist elsewhere. >> >> Ughh, that's definitely not good. >> >>> >>> Your definition is also quite odd, not matching what one would expect >>> (the PHI requirement). The tree-cfgcleanup.c variant has >>> tree_forwarder_block_p which is explicit about this. >>> >>> Btw, gsi_start_nondebug_bb does not ignore labels. >> >> Would a name like empty_block_with_phis_p be more appropriate? > > I think so. That said, my main concern ist the clash with the same named function. Agreed. OK for trunk? Aldy --------------A290328857884714260E97A8 Content-Type: text/x-patch; charset=UTF-8; name="p.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="p.patch" commit 77ac56456d5db150d6a71eaca918f19d2b478f82 Author: Aldy Hernandez Date: Tue Sep 7 15:20:23 2021 +0200 Rename forwarder_block_p in treading code to empty_block_with_phis_p. gcc/ChangeLog: * tree-ssa-threadedge.c (forwarder_block_p): Rename to... (empty_block_with_phis_p): ...this. (potentially_threadable_block): Same. (jump_threader::thread_through_normal_block): Same. diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c index 3db54a199fd..3c7cdc58b93 100644 --- a/gcc/tree-ssa-threadedge.c +++ b/gcc/tree-ssa-threadedge.c @@ -101,11 +101,10 @@ has_phis_p (basic_block bb) return !gsi_end_p (gsi_start_phis (bb)); } -/* Return TRUE for a forwarder block which is defined as having PHIs - but no instructions. */ +/* Return TRUE for a block with PHIs but no statements. */ static bool -forwarder_block_p (basic_block bb) +empty_block_with_phis_p (basic_block bb) { return gsi_end_p (gsi_start_nondebug_bb (bb)) && has_phis_p (bb); } @@ -123,7 +122,7 @@ potentially_threadable_block (basic_block bb) to the loop header. We want to thread through them as we can sometimes thread to the loop exit, which is obviously profitable. The interesting case here is when the block has PHIs. */ - if (forwarder_block_p (bb)) + if (empty_block_with_phis_p (bb)) return true; /* If BB has a single successor or a single predecessor, then @@ -1008,7 +1007,7 @@ jump_threader::thread_through_normal_block (vec *path, { /* First case. The statement simply doesn't have any instructions, but does have PHIs. */ - if (forwarder_block_p (e->dest)) + if (empty_block_with_phis_p (e->dest)) return 0; /* Second case. */ --------------A290328857884714260E97A8--