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 1368A3858403 for ; Fri, 8 Oct 2021 10:49:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1368A3858403 Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-593-aa986mTTOmGGmjfv3iJDsA-1; Fri, 08 Oct 2021 06:49:32 -0400 X-MC-Unique: aa986mTTOmGGmjfv3iJDsA-1 Received: by mail-lf1-f69.google.com with SMTP id c6-20020a05651200c600b003fc6d39efa4so6808324lfp.12 for ; Fri, 08 Oct 2021 03:49:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mXjhbs7c7YuqbdBChhYpeNxVQf5xCk0yeBeaQLv6Cm4=; b=eslqGMqOzhX80nMjCktTop+RrTfCaYvfcgYh/Twe0Wd1qgBaAtrQF8wDjbF73aKaqA PzfWvdUfbmHiFZKKMnpFA37bgCg5xKPJJvlrb7BzhjarvRxzvuzJHkjhpS55hpSLjSM4 uoojCDDwypWIZwHgFEiLRP2A2SeyW3jWTrIwIsoHzMh0XEQuOaEjvj4fKvybP56w/uf5 uv7wKThLcaxL0CEY41omLt2poSFi/pVA23hrc36N8D7uFzTUVSlnMWUcgKIirO0LwGoR CZRl3u9Yl5ddBwD2+8yIhL69S0CimROmCrSHjLzPGjarZpKYoNwjKXcu2/oorV5ZGZy9 ZvPQ== X-Gm-Message-State: AOAM531oBO6G0vao6cZrEc0C3cpVORKT6xBGy+dM0iQopyQGppVdAM44 Yt5suE6weW4iRlCw4ULNjsI6y/i34U6WmctfBWbt+IWQhnsx2wZAWSDMejG4mMfiPWyA7Zb1Nei qBp/RIBBTe+j/xlj+rFvR8/yLyff5tLu+UA== X-Received: by 2002:a2e:2f1d:: with SMTP id v29mr2839499ljv.439.1633690171095; Fri, 08 Oct 2021 03:49:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyZ8SC4+0BjOhfL5FzDqLtB2KKCEK7viQNPSbTuqxBDGK3M/wYmORG7pknmWtsZERMHU4S83rWvUF2v91MDJuU= X-Received: by 2002:a2e:2f1d:: with SMTP id v29mr2839471ljv.439.1633690170799; Fri, 08 Oct 2021 03:49:30 -0700 (PDT) MIME-Version: 1.0 References: <20210916043305.1674303-1-hongtao.liu@intel.com> <4fe85f3d-b4fd-4419-04f0-c645e23e778d@gmail.com> <5703bdb6-dc01-4436-2403-35832619ebb4@gmail.com> <333oqn86-4p5r-20s7-pqs7-98816714s0sp@fhfr.qr> In-Reply-To: <333oqn86-4p5r-20s7-pqs7-98816714s0sp@fhfr.qr> From: Aldy Hernandez Date: Fri, 8 Oct 2021 12:49:19 +0200 Message-ID: Subject: Re: [PATCH] Enable auto-vectorization at O2 with very-cheap cost model. To: Richard Biener Cc: Hongtao Liu , Jakub Jelinek , Florian Weimer , Segher Boessenkool , Richard Sandiford , Premachandra.Mallappa@amd.com, GCC Patches , Bill Schmidt , liuhongt , Joseph Myers X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Fri, 08 Oct 2021 10:49:36 -0000 On Thu, Sep 23, 2021 at 8:32 AM Richard Biener via Gcc-patches wrote: > > On Thu, 23 Sep 2021, Hongtao Liu wrote: > > > On Thu, Sep 23, 2021 at 9:48 AM Hongtao Liu wrote: > > > > > > On Wed, Sep 22, 2021 at 10:21 PM Martin Sebor wrote: > > > > > > > > On 9/21/21 7:38 PM, Hongtao Liu wrote: > > > > > On Mon, Sep 20, 2021 at 4:13 AM Martin Sebor wrote: > > > > ... > > > > >>>>> diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c > > > > >>>>> index 1d79930cd58..9351f7e7a1a 100644 > > > > >>>>> --- a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c > > > > >>>>> +++ b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c > > > > >>>>> @@ -1,7 +1,7 @@ > > > > >>>>> /* PR middle-end/91458 - inconsistent warning for writing past the end > > > > >>>>> of an array member > > > > >>>>> { dg-do compile } > > > > >>>>> - { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf" } */ > > > > >>>>> + { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf -fno-tree-vectorize" } */ > > > > >>>> > > > > >>>> The testcase is large - what part requires this change? Given the > > > > >>>> testcase was added for inconsistent warnings do they now become > > > > >>>> inconsistent again as we enable vectorization at -O2? > > > > >>>> > > > > >>>> That said, the testcase adjustments need some explaining - I suppose > > > > >>>> you didn't just slap -fno-tree-vectorize to all of those changing > > > > >>>> behavior? > > > > >>>> > > > > >>> void ga1_ (void) > > > > >>> { > > > > >>> a1_.a[0] = 0; > > > > >>> a1_.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" } > > > > >>> a1_.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" } > > > > >>> > > > > >>> struct A1 a; > > > > >>> a.a[0] = 0; > > > > >>> a.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" } > > > > >>> a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" } > > > > >>> sink (&a); > > > > >>> } > > > > >>> > > > > >>> It's supposed to be 2 warning for a.a[1] = 1 and a.a[2] = 1 since > > > > >>> there are 2 accesses, but after enabling vectorization, there's only > > > > >>> one access, so one warning is missing which causes the failure. > > > > > > > > With the stores vectorized, is the warning on the correct line or > > > > does it point to the first store, the one that's in bounds, as > > > > it does with -O3? The latter would be a regression at -O2. > > > For the upper case, It points to the second store which is out of > > > bounds, the third store warning is missing. > > > > > > > > >> > > > > >> I would find it preferable to change the test code over disabling > > > > >> optimizations that are on by default. My concern is that the test > > > > >> would no longer exercise the default behavior. (The same goes for > > > > >> the -fno-ipa-icf option.) > > > > > Hmm, it's a middle-end test, for some backend, it may not do > > > > > vectorization(it depends on TARGET_VECTOR_MODE_SUPPORTED_P and > > > > > relative cost model). > > > > > > > > Yes, there are quite a few warning tests like that. Their main > > > > purpose is to verify that in common GCC invocations (i.e., without > > > > any special options) warnings are a) issued when expected and b) > > > > not issued when not expected. Otherwise, middle end warnings are > > > > known to have both false positives and false negatives in some > > > > invocations, depending on what optimizations are in effect. > > > > Indiscriminately disabling common optimizations for these large > > > > tests and invoking them under artificial conditions would > > > > compromise this goal and hide the problems. > > > > > > > > If enabling vectorization at -O2 causes regressions in the quality > > > > of diagnostics (as the test failure above indicates seems to be > > > > happening) we should investigate these and open bugs for them so > > > > they can be fixed. We can then tweak the specific failing test > > > > cases to avoid the failures until they are fixed. > > > There are indeed cases of false positives and false negatives > > > .i.e. > > > // Verify warning for access to a definition with an initializer that > > > // initializes the one-element array member. > > > struct A1 a1i_1 = { 0, { 1 } }; > > > > > > void ga1i_1 (void) > > > { > > > a1i_1.a[0] = 0; > > > a1i_1.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" } > > > a1i_1.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" } > > > > > > struct A1 a = { 0, { 1 } }; --- false positive here. > > > a.a[0] = 1; > > > a.a[1] = 2; // { dg-warning > > > "\\\[-Wstringop-overflow" } false negative here. > > > a.a[2] = 3; // { dg-warning > > > "\\\[-Wstringop-overflow" } false negative here. > > > sink (&a); > > > } > > Similar for > > * gcc.dg/Warray-bounds-51.c. > > * gcc.dg/Warray-parameter-3.c > > * gcc.dg/Wstringop-overflow-14.c > > * gcc.dg/Wstringop-overflow-21.c > > > > So there're 3 situations. > > 1. All accesses are out of bound, and after vectorization, there are > > some warnings missing. > > 2. Part of accesses are inbound, part of accesses are out of bound, > > and after vectorization, the warning goes from out of bound line to > > inbound line. > > 3. All access are out of bound, and after vectoriation, all warning > > are missing, and goes to a false-positive line. > > I remember some of the warning code explicitely excuses itself from > even trying to deal with vectorized loads/stores, that might need to > be revisited. It would also be useful to verify whether the line > info on the vectorized loads/stores is sensible (if you dump with > -lineno you get stmts with line numbers). > > It is of course impossible to preserve the location of all original > scalar accesses exactly - it _might_ be possible to create a new > location range (not sure how they are exactly represented), but then > if we vectorize say > > a[0] = b[0]; > a[1] = b[1]; > > we'd somehow get a multi-line location range that covers unrelated > bits (the intermediate load or store). > > So currently the vectorizer simply chooses the location of one of > the scalar loads or stores for the vectorized access (and it might do > so quite randomly). > > Note I don't think it's feasible to not vectorize out-of-bound loads > or stores for the same reason that you'll say you can't do the > warnings all before vectorizing because the might expose themselves > only later. > > So we simply have to cope with the reality that GCC is optimizing and > that the later we perform analysis for warnings the more the original > code is mangled. As we moved array-bound warnings before unrolling > so we should move this particular warning to before vectorization > [loop optimization]. Sorry I'm late to the party here... Couldn't we run warnings earlier? A while back, I converted the array bounds checker to the generic range_query API, so it could be used either with the vr_values object from the VRP pass, or with a ranger. There's no reason it should be tied to VRP. We could make it its own pass and move it somewhere more sensible. Aldy