From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc35.google.com (mail-oo1-xc35.google.com [IPv6:2607:f8b0:4864:20::c35]) by sourceware.org (Postfix) with ESMTPS id AA747388982C for ; Fri, 25 Jun 2021 17:19:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AA747388982C Received: by mail-oo1-xc35.google.com with SMTP id k1-20020a0568200161b029024bef8a628bso2692738ood.7 for ; Fri, 25 Jun 2021 10:19:22 -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 :content-transfer-encoding; bh=HIrmi2WVLeXUVSZCVXP57N3z0VAXNAQWMzLSmfOcF2U=; b=HepFS4o5rE5ePT7P8lTaRebo+pMUtYY841mXsIBrcIJ9kqjP8BhMmh8YWTQZIBZTS+ 6chDPloTmzfoliSOkz71x/BZn/Vyl/mIbVzL1ZHoMKNDW40MgfE0lV3lT9vCGjQRTjVT uX3pWVhLmiRKbBKRLfsk73ialKD9xNDl/b/gAK/uCDIvgBBDAfHns8+x8mzB9htIGXKu /eWcN04bYps6PsZYtP1yUqKOHmLZpztM2rm+mtuCsCjjcHcS2wbi7hnm34ijZYcU74zh v8lSP3hrxnHCUDMIEp9UPwoAJBCh/KAQHBq2fxRd6SP2JmVBFTADLec3J1vnd3iYNp17 S8qA== X-Gm-Message-State: AOAM530VBWXoh8Iy76YCDTvkNM2Ry1ZU7CXMbXQo9dZCE2VpliNlDlFu NydyK431TaQPcrbv6Fs8Iew= X-Google-Smtp-Source: ABdhPJzBR/+nKOeLmnPpgsVpb1zaVp50vXF1jr9v6p2g7gi1aWg4pWMEh8dNxX3MrHoIjKZMLy41Jw== X-Received: by 2002:a4a:8d93:: with SMTP id i19mr8356069ook.64.1624641562075; Fri, 25 Jun 2021 10:19:22 -0700 (PDT) Received: from [192.168.0.41] (97-118-105-195.hlrn.qwest.net. [97.118.105.195]) by smtp.gmail.com with ESMTPSA id k26sm891563ook.0.2021.06.25.10.19.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Jun 2021 10:19:21 -0700 (PDT) Subject: Re: replacing the backwards threader and more To: Aldy Hernandez , Richard Biener , Jeff Law Cc: GCC Mailing List , Martin Sebor References: <07775b9d-b8eb-48cb-57ef-9cc278d38967@redhat.com> <550e1ffd-957c-f348-49b6-b980c072c307@redhat.com> <3ac5e4e1-405f-fd5a-cb36-433a93f77df4@gmail.com> <23a30c02-fa53-8734-88a8-72ec4d1e8211@redhat.com> From: Martin Sebor Message-ID: Date: Fri, 25 Jun 2021 11:19:20 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <23a30c02-fa53-8734-88a8-72ec4d1e8211@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Jun 2021 17:19:24 -0000 On 6/25/21 10:20 AM, Aldy Hernandez via Gcc wrote: > Hi folks. > > I'm done with benchmarking, testing and cleanups, so I'd like to post my > patchset for review.  However, before doing so, I'd like to address a > handful of meta-issues that may affect how I post these patches. > > Trapping on differences > ======================= > > Originally I wanted to contribute verification code that would trap if > the legacy code threaded any edges the new code couldn't (to be removed > after a week).  However, after having tested on various architectures > and only running once into a missing thread, I'm leaning towards > omitting the verification code, since it's fragile, time consuming, and > quite hacky. > > For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le. > There is only one case, across bootstrap and regression tests where the > verification code is ever tripped (discussed below). > > Performance > =========== > > I re-ran benchmarks as per our callgrind suite, and the penalty with the > current pipeline is 1.55% of overall compilation time.  As is being > discussed, we should be able to mitigate this significantly by removing > other threading passes. > > Failing testcases > ================= > > I have yet to run into incorrect code being generated, but I have had to > tweak a considerable number of tests.  I have verified every single > discrepancy and documented my changes in the testsuite when it merited > doing so.  However, there are a couple tests that trigger regressions > and I'd like to ask for guidance on how to address them. > > 1. gcc.c-torture/compile/pr83510.c > > I would like to XFAIL this. > > What happens here is that thread1 threads a switch statement such that > the various cases have been split into different independent blocks. One > of these blocks exposes an arr[i_27] access which is later propagated by > VRP to be arr[10].  This is an invalid access, but the array bounds code > doesn't know it is an unreachable path. The test has a bunch of loops that iterate over the 10 array elements. There have been bug reports about loop unrolling causing false positives -Warray-bounds (e.g., PR 92539, 92110, or 86341) so this could be the same issue. > > However, it is not until dom2 that we "know" that the value of the > switch index is such that the path to arr[10] is unreachable.  For that > matter, it is not until dom3 that we remove the unreachable path. If you do XFAIL it can you please isolate a small test case and open a bug and make it a -Warray-bounds blocker? > > 2. -Wfree-nonheap-object > > This warning is triggered while cleaning up an auto_vec.  I see that the > va_heap::release() inline is wrapped with a pragma ignore > "-Wfree-nonheap-object", but this is not sufficient because jump > threading may alter uses in such a way that may_emit_free_warning() will > warn on the *inlined* location, thus bypassing the pragma. > > I worked around this with a mere: > > > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp) >>    location_t loc = tree_inlined_location (exp); >> +  loc = EXPR_LOCATION (exp); > > but this causes a ton of Wfree-nonheap* tests to fail.  I think someone > more knowledgeable should address this (msebor??). This sounds like the same problem as PR 98871. Does the patch below fix it? https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572515.html If so, I suggest getting that patch in first to avoid testsuite failures. If it doesn't fix it I'll look into it before you commit your changes. > > 3. uninit-pred-9_b.c > > The uninit code is getting confused with the threading and the bogus > warning in line 24 is back.  I looked at the thread, and it is correct. > > I'm afraid all these warnings are quite fragile in the presence of more > aggressive optimizations, and I suspect it will only get worse. From my recent review of open -Wmaybe-uninitialized bugs (and the code) it does seem to be both fragile and getting worse. I've only found a few simple problems so far in the code but nothing that would make a dramatic difference so I can't say if it's possible to do much better, but I'm not done or ready to give up. If you XFAIL this too please open a bug for it and make it a blocker for -Wuninitialized? Martin > > 4. libphobos/src/std/net/isemail.d > > This is a D test where we don't actually fail, but we trigger the > verification code.  It is the only jump threading edge that the new code > fails to get over the old code, and it only happens on ppc64. > > It triggers because a BB4 -> BB5 is too expensive to thread, but a BBn > -> BB3 -> BB4 -> BB5 is considered safe to thread because BB3 is a latch > and it alters the profitability equation.  The reason we don't get it, > is that we assume that if a X->Y is unprofitable, it is not worth > looking at W->X->Y and so forth. > > Jeff had some fancy ideas on how to attack this.  Once such idea was to > stop looking back, but only for things we were absolutely sure would > never yield a profitable path.  I tried a subset of this, by allowing > further looks on this latch test, but my 1.55% overall performance > penalty turned into an 8.33% penalty.  Personally it looks way too > expensive for this one isolated case.  Besides, the test where this > clamping code originally came from still succeeds (commit > eab2541b860c48203115ac6dca3284e982015d2c). > > CONCLUSION > ========== > > That's basically it. > > If we agree the above things are not big issues, or can be addressed as > follow-ups, I'd like to start the ball rolling on the new threader. This > would allow more extensive testing of the code, and separate it a bit > from the other big changes coming up :). > > Aldy >