From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x331.google.com (mail-ot1-x331.google.com [IPv6:2607:f8b0:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id A942E38515EE for ; Mon, 28 Jun 2021 23:19:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A942E38515EE Received: by mail-ot1-x331.google.com with SMTP id v6-20020a0568300906b0290464d9be9510so9185251ott.12 for ; Mon, 28 Jun 2021 16:19:42 -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=mNE+LCfdlpVCRs9Xmos6rkStIGr2ARWeCwP2ELF8C08=; b=KxSWgv5qHOZS7aHhhZbYueZyrq7gnmAGeQVEuMn4KImqNXnKTFIiN+A1ijQ5eciGW6 93ksrb00qO7/STllHN3hZdpKwuTeXkiJA/E0kbm3jJ89EWZJ4dt9hxZ3tx2gtz9J9skr FdO+T3IPVy648IIJ4m0WF12NNthPGbFEhCBJhRvq2S9L9EPleY15HT97Lz0TjQSky+K7 k/eGFLZzt77P03t6GT56d/ArWVzHmEEyG1bMhRLQw1SmF+cbM1MA1bjJxh8YR5cJ4Yrj xc6FE9ZtlsXmY3E6bWTsUxKhPJzbpAe+imUkg+8dK+2f2OpEiGPdFDORziaMfn/DTsIK TxZw== X-Gm-Message-State: AOAM533ziCiyRmZipeiDOrQv4o+pypD9s6uTCttFPQaBoIo81a6r//c+ WKf1oNTk0QqlV8eIGDm4iHI= X-Google-Smtp-Source: ABdhPJw6rKyKw/NMdo2LBnJQZc7MUpJZvnL2NvQKjawqMNEewqOSs/KOELEpJaJjXvC4yFSwVm5+RA== X-Received: by 2002:a05:6830:2478:: with SMTP id x56mr1773297otr.197.1624922382042; Mon, 28 Jun 2021 16:19:42 -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 s19sm2548806oic.16.2021.06.28.16.19.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 28 Jun 2021 16:19:41 -0700 (PDT) Subject: Re: [PATCH 0/2] Ranger-based backwards threader implementation. To: Aldy Hernandez , GCC patches Cc: Martin Sebor References: <20210628162147.631850-1-aldyh@redhat.com> From: Martin Sebor Message-ID: <958cb46e-81cb-4ec6-d0f5-35da70e12957@gmail.com> Date: Mon, 28 Jun 2021 17:19:39 -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: <20210628162147.631850-1-aldyh@redhat.com> Content-Type: multipart/mixed; boundary="------------75467E758D90DFFEC87D33C8" Content-Language: en-US X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, 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-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: Mon, 28 Jun 2021 23:19:44 -0000 This is a multi-part message in MIME format. --------------75467E758D90DFFEC87D33C8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 6/28/21 10:21 AM, Aldy Hernandez via Gcc-patches wrote: > This is the ranger-based backwards threader. It is divided into two > parts: the solver and the path discovery bits. > > The solver is generic enough, that it may be of use to other passes, > so it's been abstracted into its own separate class/file. Andrew and > I have already gone over it, so I don't think a review is necessary. > Besides, it's technically an extension of the ranger infrastructure. > > On the other hand, the path discovery bits could benefit from the > watchful eye of the jump threading experts. > > Documenting the solver in a [ranger-tech] post is on my TODO list, > as I think it would be useful as an example of GORI as a general > tool, outside the VRP world. > > As I have mentioned elsewhere, I have gone through each test and > documented the reasons why they were adjusted (when useful). The > reviewer(s) may benefit from looking at the test notes. > > I have added a --param=threader-mode={ranger,legacy} option, which I > hope to remove shortly after. It has been useful for diagnosing > issues in the past, though perhaps not so much now. I've left it > in case there's a remote interest in using it during stage1, but > removing it could be a huge cleanup to tree-ssa-threadbackward.c. > > If/when accepted, I will open 2-3 PRs with the XFAILed tests as > requested. I am still working on distilling a C counterpart for > the libphobos missing thread edge. It'll hopefully be ready by the > time the review is done. > > A version of this patchset with the verification code has > been tested on x86-64, ppc64, ppc64le, and aarch64 (all Linux). > > I am currently re-testing on x86-64 Linux, but will not re-test on the > rest of the architectures because...OMG aarch6 is so slow! I applied the series and ran a subset of tests and didn't see any failures, just the three XPASSes below. The Wfree-nonheap-object tests you mentioned in the other post all pass. Looks like you got past that problem? XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 32) XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 46) XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 60) A couple of comments on the tests below (I haven't looked at the meat of the patch): > > Thanks. > Aldy > > Aldy Hernandez (2): > Implement basic block path solver. > Backwards jump threader rewrite with ranger. > > gcc/Makefile.in | 6 + > gcc/flag-types.h | 7 + > gcc/params.opt | 17 + > .../g++.dg/debug/dwarf2/deallocator.C | 3 +- > gcc/testsuite/gcc.c-torture/compile/pr83510.c | 33 ++ > gcc/testsuite/gcc.dg/Wrestrict-22.c | 3 + The change here just adds the comment: +/* This looks like the threader caused the entire loop to collapse, and the + warning pass can't determine the arguments to memcpy. */ + Since the test passes I'm not sure I understand what the comment is trying to say. Is it still accurate and necessary? > gcc/testsuite/gcc.dg/loop-unswitch-2.c | 2 +- > gcc/testsuite/gcc.dg/old-style-asm-1.c | 5 +- > gcc/testsuite/gcc.dg/pr68317.c | 4 +- > gcc/testsuite/gcc.dg/pr97567-2.c | 2 +- > gcc/testsuite/gcc.dg/predict-9.c | 4 +- > gcc/testsuite/gcc.dg/shrink-wrap-loop.c | 53 ++ > gcc/testsuite/gcc.dg/sibcall-1.c | 10 + > .../gcc.dg/tree-ssa/builtin-sprintf-3.c | 5 +- I wonder if breaking up the test function into five, one for each of the tests it does, would be a better way to avoid the IL changes than disabling all the threading passes. Like in the attached patch. Martin > gcc/testsuite/gcc.dg/tree-ssa/pr21001.c | 1 + > gcc/testsuite/gcc.dg/tree-ssa/pr21294.c | 1 + > gcc/testsuite/gcc.dg/tree-ssa/pr21417.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr21458-2.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr21563.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr49039.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr61839_1.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr61839_3.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/pr77445-2.c | 2 +- > .../gcc.dg/tree-ssa/ranger-threader-1.c | 20 + > .../gcc.dg/tree-ssa/ranger-threader-2.c | 39 ++ > .../gcc.dg/tree-ssa/ranger-threader-3.c | 41 ++ > .../gcc.dg/tree-ssa/ranger-threader-4.c | 83 +++ > gcc/testsuite/gcc.dg/tree-ssa/split-path-4.c | 4 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-11.c | 2 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-12.c | 2 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-14.c | 1 + > .../gcc.dg/tree-ssa/ssa-dom-thread-18.c | 5 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-6.c | 4 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-7.c | 1 + > gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-48.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/ssa-thread-11.c | 1 + > gcc/testsuite/gcc.dg/tree-ssa/ssa-thread-12.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/ssa-thread-14.c | 1 + > gcc/testsuite/gcc.dg/tree-ssa/vrp02.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp03.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp05.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp06.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp07.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp09.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp19.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp20.c | 2 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp33.c | 2 +- > gcc/testsuite/gcc.dg/vect/bb-slp-16.c | 7 + > .../gcc.target/i386/avx2-vect-aggressive.c | 2 +- > gcc/tree-ssa-path-solver.cc | 310 ++++++++++++ > gcc/tree-ssa-path-solver.h | 85 ++++ > gcc/tree-ssa-threadbackward.c | 475 +++++++++++++++++- > gcc/tree-ssa-threadedge.c | 20 +- > gcc/tree-ssa-threadedge.h | 3 +- > gcc/tree-ssa-threadupdate.c | 12 +- > gcc/tree-ssa-threadupdate.h | 2 +- > .../libgomp.graphite/force-parallel-4.c | 1 + > .../libgomp.graphite/force-parallel-8.c | 2 + > 58 files changed, 1261 insertions(+), 54 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ranger-threader-1.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ranger-threader-2.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ranger-threader-3.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ranger-threader-4.c > create mode 100644 gcc/tree-ssa-path-solver.cc > create mode 100644 gcc/tree-ssa-path-solver.h > --------------75467E758D90DFFEC87D33C8 Content-Type: text/x-patch; charset=UTF-8; name="builtin-sprintf-3.c.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="builtin-sprintf-3.c.diff" diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-3.c index fae2a1b73ea..b2e005bc716 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-3.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-3.c @@ -3,7 +3,10 @@ that the sprintf return value (or value range) optimization is not performed for an unknown string. */ /* { dg-do compile } */ -/* { dg-options "-O2 -Wall -Werror -fdump-tree-optimized -fprintf-return-value" } */ +/* { dg-options "-O2 -Wall -Werror -fdump-tree-optimized" } */ + +/* Note: Threader will duplicate BBs such that there are multiple + string_*_fail calls on certain paths. */ #define INT_MAX __INT_MAX__ #define INT_MIN (-INT_MAX - 1) @@ -15,7 +18,7 @@ extern void string_lt_0_fail (); extern void string_eq_0_fail (); extern void string_gt_0_fail (); -void test_string (char *d, const char *s) +void test_string_eq_min (char *d, const char *s) { int n = __builtin_sprintf (d, "%-s", s); @@ -23,13 +26,36 @@ void test_string (char *d, const char *s) or INT_MAX. (This is a white box test based on knowing that the optimization computes its own values of the two constants.) */ if (n == INT_MIN) string_eq_min_fail (); +} + +void test_string_eq_max (char *d, const char *s) +{ + int n = __builtin_sprintf (d, "%-s", s); + if (n == INT_MAX) string_eq_max_fail (); +} + +void test_string_lt_0 (char *d, const char *s) +{ + int n = __builtin_sprintf (d, "%-s", s); /* The return value could be negative when strlen(s) is in excess of 4095 (the maximum number of bytes a single directive is required to handle). */ if (n < 0) string_lt_0_fail (); +} + +void test_string_eq_0 (char *d, const char *s) +{ + int n = __builtin_sprintf (d, "%-s", s); + if (n == 0) string_eq_0_fail (); +} + +void test_string_gt_0 (char *d, const char *s) +{ + int n = __builtin_sprintf (d, "%-s", s); + if (n > 0) string_gt_0_fail (); } --------------75467E758D90DFFEC87D33C8--