From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com [IPv6:2607:f8b0:4864:20::72c]) by sourceware.org (Postfix) with ESMTPS id 83CED385840C for ; Fri, 8 Oct 2021 23:43:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 83CED385840C Received: by mail-qk1-x72c.google.com with SMTP id g21so11109294qki.11 for ; Fri, 08 Oct 2021 16:43:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=zWF3IL/KlWqPwAWg/Utwy7yJJ0pHOl1mDBS7AaKs1V4=; b=ztaz6lpM8kvbdZNSp2QP4y1jllF+dnfFRCgr+IWxJv+dOn7ktyUyCeiZORCsTN94O0 m+vf8mBbqkNHZMSwZL8hDAiX/IynMAbxjlnt0NtF8cei8r4CUeHCn1FQ6OgwYhdfaJ6d x4N7vnYZ+AfhYhRCtCK5UMmYmpOFR4qxjPRynFTllTs52b24Xj806nBuzLx17wmrt+5x cO8TnWmEPa0im1KDaNljsA5apssjGkoZX7EVj13RKoOeiMJm8rL/vNkdhbMDiZoWcM6M bCem+D5F5orxi07MM4iqa8uT4QXpKJY5irTxGuGPIWDBx5oDzBqCzYSLY7f60XbQz382 AHJw== X-Gm-Message-State: AOAM531gZvt9afT/tB9DVjC4C+QHNk0ltronPWzooFSKtxx3pq9jaD6/ aOntkolR7ZUZSN6o+7uJZTI= X-Google-Smtp-Source: ABdhPJzvhO/PofUJmrm3tsfkjOL3K20YNb3vd78w2Yf/NkxlB8mpSce0jbHQascbAQxqvqYCprrC1Q== X-Received: by 2002:a37:60c3:: with SMTP id u186mr5353232qkb.462.1633736639063; Fri, 08 Oct 2021 16:43:59 -0700 (PDT) Received: from [192.168.0.41] (184-96-250-116.hlrn.qwest.net. [184.96.250.116]) by smtp.gmail.com with ESMTPSA id 66sm640818qke.118.2021.10.08.16.43.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 08 Oct 2021 16:43:58 -0700 (PDT) Subject: Re: [PATCH] Enable auto-vectorization at O2 with very-cheap cost model. To: Aldy Hernandez , Richard Biener Cc: Jakub Jelinek , Florian Weimer , Segher Boessenkool , Richard Sandiford , Premachandra.Mallappa@amd.com, GCC Patches , Bill Schmidt , liuhongt , Joseph Myers 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> From: Martin Sebor Message-ID: <150b0ba4-8542-beee-e064-8a07b1152226@gmail.com> Date: Fri, 8 Oct 2021 17:43:56 -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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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 23:44:01 -0000 On 10/8/21 4:49 AM, Aldy Hernandez via Gcc-patches wrote: > 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? Yes, we could. > > 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. The warning here (-Wstringop-overflow) comes from the strlen pass which runs after vectorization which is why we lose the accurate location for individual stores. VRP runs before it, so -Warray-bounds doesn't suffer from the same problem. Running the strlen subset of -Wstringop-overflow earlier will avoid the problem: https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580040.html It shouldn't even be necessary to move it -- -Warray-bounds should be enough. I think the only difference is that -Wstringop-overflow for plain char assignments respects subobject boundaries while -Warray-bounds does not (for accesses of any type). That's a hack to avoid some false positives so if we can find some other way to avoid those we can improve -Warray-bounds and get rid of -Wstringop-overflow for direct char stores (the warning was meant for built-in string functions but it has grown to encompass direct stores as well). That said, it is useful to distinguish out-of-bounds reads from out-of-bounds writes. -Wstringop-overflow and -Wstringop-overread make that possible. -Warray-bounds doesn't. So maybe we also need to enhance -Warry-bounds and add the corresponding modes. Martin > > Aldy >