On Wed, Oct 13, 2021 at 11:34 AM Hongtao Liu <crazy...@gmail.com> wrote:
>
> On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <mse...@gmail.com> 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
> > > <gcc-patches@gcc.gnu.org> 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 <vector\(2\) char>}
+ # 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
