From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk1-xa2b.google.com (mail-vk1-xa2b.google.com [IPv6:2607:f8b0:4864:20::a2b]) by sourceware.org (Postfix) with ESMTPS id ED444385842A for ; Wed, 13 Oct 2021 06:23:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ED444385842A Received: by mail-vk1-xa2b.google.com with SMTP id n201so1308335vkn.12 for ; Tue, 12 Oct 2021 23:23:16 -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=WZqvMtkvjnepqTKyIOhSxG16QQcoUc/BFWY8I12CzjA=; b=6FvQUofokjm68CVx8BJdMHPStkevOvmiPJyAOLLDJKhUtbbajD3He7pIaPCzaYs07Y QQJQxXpZbPzoqUJG1gmMfU/itIYin8Zs1t7xchUT/9q0uFvbgO11Ir/xnL05fghuTNZf EsKs5l+VuItj0hTEJROae1MOo6MvL79wgAmaTaWwcYolvdPkgPfsKbjLe+n88OIxL9EY FWZ4v5tmtupWcE7N1NMHpP9DmEgm+esYChERIgajnCXzWuDUSDfLU322kQApJ1wUwaA8 pVpFZJhg7RrMKn7GHvIPwewC6CdeVCxfNkE4LwSqkqxO1tbZfhWTjf6ZOE2ngaUPE4M1 OHCQ== X-Gm-Message-State: AOAM530G+Qs3PdOScqp8+zFMw8kiwfX7H371ULD2qA02f8GMj2O+dpDB ekPOIEG+u3vOISXQSuCJIhU5ZB7/oH4cA7VzrCY= X-Google-Smtp-Source: ABdhPJxOO2hl43wbE74bGaOZTTaQR1LYAWKTwwY3iHdjZXIcUYIrBEjlj3+xKXB54tVBZy6PTQ07K0vJ7EJXekXTs7c= X-Received: by 2002:a1f:3409:: with SMTP id b9mr31715629vka.21.1634106196257; Tue, 12 Oct 2021 23:23:16 -0700 (PDT) MIME-Version: 1.0 References: <0e964ac9-0e58-33c1-c0ab-24b7f1c60be3@linux.ibm.com> <20211011153050.GV10333@gate.crashing.org> <9bb2743f-cd23-5b7d-6d9d-9917e591377f@gmail.com> <20211011174302.GZ10333@gate.crashing.org> <5966f37c-a51e-593c-4ee2-05c3d04c89fc@gmail.com> In-Reply-To: From: Hongtao Liu Date: Wed, 13 Oct 2021 14:29:51 +0800 Message-ID: Subject: Re: [PATCH] rs6000/test: Adjust some cases due to O2 vect [PR102658] To: Martin Sebor Cc: Segher Boessenkool , Hongtao Liu , GCC Patches , Bill Schmidt , David Edelsohn Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, 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: Wed, 13 Oct 2021 06:23:19 -0000 On Wed, Oct 13, 2021 at 11:34 AM Hongtao Liu wrote: > > On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor wrote: > > > > On 10/11/21 8:31 PM, Hongtao Liu wrote: > > > On Tue, Oct 12, 2021 at 4:08 AM Martin Sebor via Gcc-patches > > > wrote: > > >> > > >> On 10/11/21 11:43 AM, Segher Boessenkool wrote: > > >>> On Mon, Oct 11, 2021 at 10:23:03AM -0600, Martin Sebor wrote: > > >>>> On 10/11/21 9:30 AM, Segher Boessenkool wrote: > > >>>>> On Mon, Oct 11, 2021 at 10:47:00AM +0800, Kewen.Lin wrote: > > >>>>>> - For generic test cases, it follows the existing suggested > > >>>>>> practice with necessary target/xfail selector. > > >>>>> > > >>>>> Not such a great choice. Many of those tests do not make sense with > > >>>>> vectorisation enabled. This should have been thought about, in some > > >>>>> cases resulting in not running the test with vectorisation enabled, and > > >>>>> in some cases duplicating the test, once with and once without > > >>>>> vectorisation. > > >>>> > > >>>> The tests detect bugs that are present both with and without > > >>>> vetctorization, so they should pass both ways. > > >>> > > >>> Then it should be tested both ways! This is my point. > > >> > > >> Agreed. (Most warnings are tested with just one set of options, > > >> but it's becoming apparent that the middle end ones should be > > >> exercised more extensively.) > > >> > > >>> > > >>>> That they don't > > >>>> tells us that that the warnings need work (they were written with > > >>>> an assumption that doesn't hold anymore). > > >>> > > >>> They were written in world A. In world B many things behave > > >>> differently. Transplanting the testcases from A to B without any extra > > >>> analysis will not test what the testcases wanted to test, and possibly > > >>> nothing at all anymore. > > >> > > >> Absolutely. > > >> > > >>> > > >>>> We need to track that > > >>>> work somehow, but simply xfailing them without making a record > > >>>> of what underlying problem the xfails correspond to isn't the best > > >>>> way. In my experience, what works well is opening a bug for each > > >>>> distinct limitation (if one doesn't already exist) and adding > > >>>> a reference to it as a comment to the xfail. > > >>> > > >>> Probably, yes. > > >>> > > >>>>> But you are just following established practice, so :-) > > >>> > > >>> I also am okay with this. If it was decided x86 does not have to deal > > >>> with these (generic!) problems, then why should we do other people's > > >>> work? > > >> > > >> I don't know that anything was decided. I think those changes > > >> were made in haste, and (as you noted in your review of these > > >> updates to them), were incomplete (missing comments referencing > > >> the underlying bugs or limitations). Now that we've noticed it > > >> we should try to fix it. I'm not expecting you (or Kwen) to do > > >> other people's work, but it would help to let them/us know that > > >> there is work for us to do. I only noticed the problem by luck. > > >> > > >>>>>> - struct A1 a = { 0, { 1 } }; // { dg-warning > > >>>>>> "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* } } } > > >>>>>> + struct A1 a = { 0, { 1 } }; // { dg-warning > > >>>>>> "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* > > >>>>>> } } } > > >>>> > > >>>> As I mentioned in the bug, when adding xfails for regressions > > >>>> please be sure to reference the bug that tracks the underlying > > >>>> root cause.] > > >>> > > >>> You are saying this to whoever added that x86 xfail I hope. > > >> > > >> In general it's an appeal to both authors and reviewers of such > > >> changes. Here, it's mostly for Hongtao who apparently added all > > >> these undocumented xfails. > > >> > > >>>> There may be multiple problems, and we need to > > >>>> identify what it is in each instance. As the author of > > >>>> the tests I can help with that but not if I'm not in the loop > > >>>> on these changes (it would seem prudent to get the author's > > >>>> thoughts on such sweeping changes to their work). > > >>> > > >>> Yup. > > >>> > > >>>> I discussed one of these failures with Hongtao in detail at > > >>>> the time autovectorization was being enabled and made the same > > >>>> request then but I didn't realize the problem was so pervasive. > > >>>> > > >>>> In addition, the target-specific conditionals in the xfails are > > >>>> going to be difficult to maintain. > > >>> > > >>> It is a cop-out. Especially because it makes no comment why it is > > >>> xfailed (which should *always* be explained!) > > >>> > > >>>> It might be okay for one or > > >>>> two in a single test but for so many we need a better solution > > >>>> than that. If autovectorization is only enabled for a subset > > >>>> of targets then a solution might be to add a new DejagGNU test > > >>>> for it and conditionalize the xfails on it. > > >>> > > >>> That, combined with duplicating these tests and still testing the > > >>> -fno-vectorization situation properly. Those tests tested something. > > >>> With vectorisation enabled they might no longer test that same thing, > > >>> especially if the test fails now! > > >> > > >> Right. The original autovectorization change was made either > > >> without a full analysis of its impact on the affected warnings, > > >> or its impact wasn't adequately captured (either in the xfails > > >> comments or by opening bugs for them). Now that we know about > > >> this we should try to fix it. The first step toward that is > > >> to review the xfailed test cases and for each add a comment with > > >> the bug that captures its root cause. > > >> > > >> Hongtao, please let me know if you are going to work on that. > > > I will make a copy of the tests to test the -fno-tree-vectorize > > > scenario(the original test). > > > For the xfails, they're analyzed and recorded in pr102462/pr102697, > > > sorry for not adding comments to them. > > > > Thanks for raising pr102697! It captures the essence of the bug > > that's masked by the vectorization of the invalid store. This is > > due to the hack I pointed to in the discussion below: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580172.html > > > > > The root causes for those xfails are divided into 2 categories: > > > > > > 1. All accesses are out of bound, and after vectorization, there are > > > some warnings missing.(Because there is only 1 access after > > > vectorization, 2 accesses w/o vectorization, and diagnostic is based > > > on access). > > > > If these involve -Wstringop-overflow for accesses that span > > multiple subobjects, as in writing past the end of one member > > and over the following member, then that would be due to > > pr102697 (the hack above). > > > > > 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. > > > > Right, this is the issue we talked about during the review of > > your patch, and I think is captured in the test case in comment > > #4 on pr102462. > > > > > > > > for pr102697, it looks like the testcase is not well written. > > > > The test case is correct. I've added my comments to the PR > > and confirmed it as a GCC 12 regression. (I may not have > > the time to fix it for GCC 12 but I will plan to get to it > > for GCC 13 unless someone beats me to it.) > > > > I think it might be helpful to open a bug just for case (2) > > and reference it in all the corresponding xfails. > > > > pr102462 talks about three distinct cases and mentions > > -Warray-bounds as well as -Wstringop-overflow. It's not clear > > from it exactly which of the three cases it's meant to be about. > > > > There is also an undocumented xfail in > > g++.dg/warn/Wuninitialized-13.C. It should get its own bug > > even if the essence of the problem is the same (the warning > > doesn't share an implementation with -Warray-bounds or > > -Wstringop-overflow so a fix will most likely need to be > > separate from one for the other bugs). > > > > Coming back to the xfail conditionals, do you think you'll > > be able to put together some target-supports magic so they > > don't have to enumerate all the affected targets? > > > Those failure testcases(exposed by x86 part)can be extracted and > categorized into 3 below testcases. > Question is can we check vectorization ability in > dg-require-effective-target for those testcase? > If we can, we can dynamically check whether each target supports this xfail. > How about +# Return true if vectorization of v2qi store is enabed. +# Return zero if the desirable pattern isn't found. +# It's used by Warray-bounds/Wstringop-overflow testcases which are +# regressed by O2 vectorization, refer to PR102697/PR102462/PR102706 +proc check_vect_slp_v2qi_store_usage { } { + global tool + + return [check_cached_effective_target slp_v2qi_store_usage { + set result [check_compile slp_v2qi_store_usage assembly { + char p[4] __attribute__ ((aligned (4))); + void + foo () + { + p[0] = 1; + p[1] = 2; + p[2] = 3; + } + } "-O2 -fopt-info-all" ] + + # Get compiler emitted messages and delete generated file. + set lines [lindex $result 0] + set output [lindex $result 1] + remote_file build delete $output + + set pattern1 {optimized: basic block part vectorized using [0-9]+ byte vectors} + set pattern2 {add new stmt: MEM } + # Capture the vectorized info of v2qi, set it to zero if not found. + if { ![regexp $pattern1 $lines whole val] + || ![regexp $pattern2 $lines whole val] } then { + set val 0 + } + + return $val + }] +} + +# Return the true if target support vectorization of v2qi store. +proc check_effective_target_vect_slp_v2qi_store { } { + return [expr { [check_vect_slp_v2qi_store_usage] != 0 }] +} similar for vect_slp_v4qi_store/vec_slp_v2hi_store, and add these target selector to xfail/target cases. > foo is used for vector(2) char, foo1 vector(4) char, foo2 vector(2) short. > > char p[4] __attribute__ ((aligned (4))); > void > foo () > { > p[0] = 1; > p[1] = 2; > p[2] = 3; > } > > void > foo1 () > { > p[0] = 0; > p[1] = 1; > p[2] = 2; > p[3] = 3; > } > > void > foo2 (short* q) > { > q[0] = 0; > q[1] = 1; > } > > Martin > > > > > -- > BR, > Hongtao -- BR, Hongtao