| Message ID | 0e964ac9-0e58-33c1-c0ab-24b7f1c60be3@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000/test: Adjust some cases due to O2 vect [PR102658] | expand |
Hi! On Mon, Oct 11, 2021 at 10:47:00AM +0800, Kewen.Lin wrote: > As PR102658 shows, commit r12-4240 enables vectorization at O2, > some cases need to be adjusted accordingly for rs6000 port. > > - For target specific test cases, this adds -fno-tree-vectorize > to retain original test points, otherwise vectorization can > make some expected scalar instructions gone or generate some > unexpected instructions for vector construction. Ah good choice. > - 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. But you are just following established practice, so :-) > - 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*-*-* } } } I don't know if powerpc*-*-* is the correct choice in all these cases. Sometimes it might have to be powerpc*-*-linux* or similar. We'll find out :-) (An xfail causes XPASS if the test does *not* fail). > +/* Now O2 enables vectorization by default, which generates unexpected float > + conversion for vector construction, so simply disable it. */ It is good to see these comments. I love puzzles, but not in the testsuite! :-) Okay for trunk. Thanks! Segher
On 10/11/21 9:30 AM, Segher Boessenkool wrote: > Hi! > > On Mon, Oct 11, 2021 at 10:47:00AM +0800, Kewen.Lin wrote: >> As PR102658 shows, commit r12-4240 enables vectorization at O2, >> some cases need to be adjusted accordingly for rs6000 port. >> >> - For target specific test cases, this adds -fno-tree-vectorize >> to retain original test points, otherwise vectorization can >> make some expected scalar instructions gone or generate some >> unexpected instructions for vector construction. > > Ah good choice. > >> - 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. That they don't tells us that that the warnings need work (they were written with an assumption that doesn't hold anymore). 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. > > But you are just following established practice, so :-) > >> - 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. 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). 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 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. Martin > > I don't know if powerpc*-*-* is the correct choice in all these cases. > Sometimes it might have to be powerpc*-*-linux* or similar. We'll find > out :-) > > (An xfail causes XPASS if the test does *not* fail). > >> +/* Now O2 enables vectorization by default, which generates unexpected float >> + conversion for vector construction, so simply disable it. */ > > It is good to see these comments. I love puzzles, but not in the > testsuite! :-) > > Okay for trunk. Thanks! > > > Segher >
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. > 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. > 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? > >>- 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. > 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! Thanks, Segher
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. Martin
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. 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). 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. for pr102697, it looks like the testcase is not well written. > > Martin
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? Martin
Hi! On Tue, Oct 12, 2021 at 09:49:19AM -0600, Martin Sebor wrote: > 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? There should only be an xfail if we do not expect to be able to fix the bug causing this any time soon. There shouldn't be one here, not yet anyway. Other than that: yes, and one you have such a selector, just dg-require it (or its inverse) for this test, don't xfail the test (if this is expected and correct behaviour). Segher
On 10/12/21 10:18 AM, Segher Boessenkool wrote: > Hi! > > On Tue, Oct 12, 2021 at 09:49:19AM -0600, Martin Sebor wrote: >> 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? > > There should only be an xfail if we do not expect to be able to fix the > bug causing this any time soon. There shouldn't be one here, not yet > anyway. > > Other than that: yes, and one you have such a selector, just dg-require > it (or its inverse) for this test, don't xfail the test (if this is > expected and correct behaviour). My sense is that fixing all the fallout from the vectorization change is going to be delicate and time-consuming work. With the end of stage 1 just about a month away I'm not too optimistic how much of it I'll be able to get it done before then. Depending on how intrusive the fixes turn out to be it may or may not be suitable in stage 3. Based on pr102706 that Jeff reported for the regressions in his automated tester, it also sounds like the test failures are spread out across a multitude of targets. In addition, it doesn't look like the targets are all the same in all the tests. Enumerating the targets that correspond to each test failure would be like playing the proverbial Whac-A-Mole. That makes me think we do need some such selector rather soon. The failing test cases are a subset of all the cases exercised by the tests. We don't want to conditionally enable/disable the whole tests just for the few failing cases (if that's what you were suggesting by dg-require). So we need to apply the selector to individual dg-warning and dg-bogus directives in these tests. Martin
On 10/12/2021 11:15 AM, Martin Sebor via Gcc-patches wrote: > On 10/12/21 10:18 AM, Segher Boessenkool wrote: >> Hi! >> >> On Tue, Oct 12, 2021 at 09:49:19AM -0600, Martin Sebor wrote: >>> 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? >> >> There should only be an xfail if we do not expect to be able to fix the >> bug causing this any time soon. There shouldn't be one here, not yet >> anyway. >> >> Other than that: yes, and one you have such a selector, just dg-require >> it (or its inverse) for this test, don't xfail the test (if this is >> expected and correct behaviour). > > My sense is that fixing all the fallout from the vectorization > change is going to be delicate and time-consuming work. With > the end of stage 1 just about a month away I'm not too optimistic > how much of it I'll be able to get it done before then. Depending > on how intrusive the fixes turn out to be it may or may not be > suitable in stage 3. > > Based on pr102706 that Jeff reported for the regressions in his > automated tester, it also sounds like the test failures are spread > out across a multitude of targets. In addition, it doesn't look > like the targets are all the same in all the tests. Enumerating > the targets that correspond to each test failure would be like > playing the proverbial Whac-A-Mole. There'll be some degree of whac-a-mole. But it likely isn't every target. I'm still evaluating that when I have a few minutes to look at a given target. jeff
On Tue, Oct 12, 2021 at 11:15:51AM -0600, Martin Sebor wrote: > On 10/12/21 10:18 AM, Segher Boessenkool wrote: > >On Tue, Oct 12, 2021 at 09:49:19AM -0600, Martin Sebor wrote: > >>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? > > > >There should only be an xfail if we do not expect to be able to fix the > >bug causing this any time soon. There shouldn't be one here, not yet > >anyway. > > > >Other than that: yes, and one you have such a selector, just dg-require > >it (or its inverse) for this test, don't xfail the test (if this is > >expected and correct behaviour). > > My sense is that fixing all the fallout from the vectorization > change is going to be delicate and time-consuming work. With > the end of stage 1 just about a month away I'm not too optimistic > how much of it I'll be able to get it done before then. Depending > on how intrusive the fixes turn out to be it may or may not be > suitable in stage 3. Some it will be suitable for stage4, even (testsuite-only changes for example). > Based on pr102706 that Jeff reported for the regressions in his > automated tester, it also sounds like the test failures are spread > out across a multitude of targets. In addition, it doesn't look > like the targets are all the same in all the tests. Enumerating > the targets that correspond to each test failure would be like > playing the proverbial Whac-A-Mole. > > That makes me think we do need some such selector rather soon. Yes. > The failing test cases are a subset of all the cases exercised > by the tests. We don't want to conditionally enable/disable > the whole tests just for the few failing cases (if that's what > you were suggesting by dg-require). I mean that the tests should not be done on targets where those tests do not make sense. > So we need to apply > the selector to individual dg-warning and dg-bogus directives > in these tests. Some of those tests should not be run with -fvectorize at all, imo. You *want* to limit things a lot, for detail tests. Segher
On Mon, Oct 11, 2021 at 02:07:49PM -0600, Martin Sebor wrote: > On 10/11/21 11:43 AM, Segher Boessenkool wrote: > >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. It was approved though :-) I don't know all history behind it. > 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). Yeah. > 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. There is still a month of stage 1 to go, and we are getting >50 new fails every day. Maybe once that dies down we can report anything :-( Segher
On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <msebor@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. 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
On Wed, Oct 13, 2021 at 11:34 AM Hongtao Liu <crazylht@gmail.com> wrote: > > On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <msebor@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
on 2021/10/13 下午2:29, Hongtao Liu via Gcc-patches wrote: > On Wed, Oct 13, 2021 at 11:34 AM Hongtao Liu <crazylht@gmail.com> wrote: >> >> On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <msebor@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. Nice! Thanks for doing this. It seems we can have one general proc check_vect_slp_store_usage, and pass the different arguments to it according to v2qi, v4qi and v2hi. And I assume all these kinds of test cases are simple, it won't have the possibility that this pre-test says slp-ed while the actual case says no when there are some other stmts affecting the profitability evaluation. Otherwise, we might have to force unlimited cost model for both. BR, Kewen >> 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 > > >
On 10/13/21 1:43 AM, Kewen.Lin wrote: > on 2021/10/13 下午2:29, Hongtao Liu via Gcc-patches wrote: >> On Wed, Oct 13, 2021 at 11:34 AM Hongtao Liu <crazylht@gmail.com> wrote: >>> >>> On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <msebor@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. > > Nice! Thanks for doing this. It seems we can have one general proc > check_vect_slp_store_usage, and pass the different arguments to it > according to v2qi, v4qi and v2hi. Yes, thanks for your help! If this approach is sufficiently reliable it's great. My own thought was to check the output of -Q --help=optimizers looking for vectorization being enabled: $ gcc -O2 -S -Wall -Q --help=optimizers -xc - </dev/null | grep vector -ftree-loop-vectorize [enabled] -ftree-slp-vectorize [enabled] -ftree-vectorize [disabled] Whereas with GCC 11: -ftree-loop-vectorize [disabled] -ftree-slp-vectorize [disabled] -ftree-vectorize Martin > > And I assume all these kinds of test cases are simple, it won't have > the possibility that this pre-test says slp-ed while the actual case > says no when there are some other stmts affecting the profitability > evaluation. Otherwise, we might have to force unlimited cost model > for both. > > BR, > Kewen > >>> 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 >> >> >> >
diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c index 7d29b5f48c7..5d83caddc4e 100644 --- a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c +++ b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c @@ -221,10 +221,10 @@ void ga1_1 (void) a1_1.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" } a1_1.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" } - 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*-*-* } } } a.a[0] = 0; - a.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } - a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } + a.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } + a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } sink (&a); } @@ -320,10 +320,10 @@ void ga1i_1 (void) a1i_1.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" } a1i_1.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" } - 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*-*-* } } } a.a[0] = 1; - a.a[1] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } - a.a[2] = 3; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } + a.a[1] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } + a.a[2] = 3; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } sink (&a); } diff --git a/gcc/testsuite/g++.dg/warn/Wuninitialized-13.C b/gcc/testsuite/g++.dg/warn/Wuninitialized-13.C index 210e74c3c3b..4ad897a6486 100644 --- a/gcc/testsuite/g++.dg/warn/Wuninitialized-13.C +++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-13.C @@ -5,7 +5,7 @@ struct shared_count { shared_count () { } shared_count (shared_count &r) - : pi (r.pi) { } // { dg-warning "\\\[-Wuninitialized" "" { xfail { i?86-*-* x86_64-*-* } } } + : pi (r.pi) { } // { dg-warning "\\\[-Wuninitialized" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } int pi; }; diff --git a/gcc/testsuite/gcc.dg/Warray-parameter-3.c b/gcc/testsuite/gcc.dg/Warray-parameter-3.c index e8a269c85c6..f7404be8742 100644 --- a/gcc/testsuite/gcc.dg/Warray-parameter-3.c +++ b/gcc/testsuite/gcc.dg/Warray-parameter-3.c @@ -77,7 +77,7 @@ gia3 (int a[3]) __attribute__ ((noipa)) void gcas3 (char a[static 3]) { - a[0] = 0; a[1] = 1; a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* } } } + a[0] = 0; a[1] = 1; a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* } } } a[3] = 3; // { dg-warning "\\\[-Warray-bounds" } } diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-21.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-21.c index d88bde9c740..2db6a52b22b 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-21.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-21.c @@ -23,10 +23,10 @@ void test_store_zero_length (int i) { char a[3]; struct S0 *p = (struct S0*)a; - p->a = 0; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* } } } + p->a = 0; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* } } } p->b[0] = 0; p->b[1] = 1; // { dg-bogus "\\\[-Wstringop-overflow" } - p->b[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } + p->b[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } p->b[i] = 2; sink (p); } @@ -50,10 +50,10 @@ void test_store_flexarray (int i) { char a[3]; struct Sx *p = (struct Sx*)a; - p->a = 0; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* } } } + p->a = 0; // { dg-warning "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* } } } p->b[0] = 0; p->b[1] = 1; // { dg-bogus "\\\[-Wstringop-overflow" } - p->b[2] = 1; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } } + p->b[2] = 1; // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } p->b[i] = 2; sink (p); } diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-68.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-68.c index 09df0004991..02afe486f8a 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-68.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-68.c @@ -58,10 +58,10 @@ void warn_comp_lit_zero (void) void warn_comp_lit (void) { *(AC2*)a1 = Ac2; // { dg-warning "writing 2 bytes into a region of size 1" "pr101475" { xfail *-*-* } } - *(AC4*)a2 = Ac4; // { dg-warning "writing 4 bytes into a region of size 2" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* } } } } - *(AC4*)a3 = Ac4; // { dg-warning "writing 4 bytes into a region of size 3" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* } } } } - *(AC8*)a4 = Ac8; // { dg-warning "writing 8 bytes into a region of size 4" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* } } } } - *(AC8*)a7 = Ac8; // { dg-warning "writing 8 bytes into a region of size 7" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* } } } } + *(AC4*)a2 = Ac4; // { dg-warning "writing 4 bytes into a region of size 2" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* powerpc*-*-* } } } } + *(AC4*)a3 = Ac4; // { dg-warning "writing 4 bytes into a region of size 3" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* powerpc*-*-* } } } } + *(AC8*)a4 = Ac8; // { dg-warning "writing 8 bytes into a region of size 4" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* powerpc*-*-* } } } } + *(AC8*)a7 = Ac8; // { dg-warning "writing 8 bytes into a region of size 7" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* powerpc*-*-* } } } } *(AC16*)a15 = Ac16; // { dg-warning "writing 16 bytes into a region of size 15" "pr101475" { xfail { ! { i?86-*-* x86_64-*-* } } } } } diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-76.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-76.c index 0c7b53ccc0b..e66794d59c9 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-76.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-76.c @@ -27,10 +27,10 @@ void max_a3_a5 (int i) by its own warning independently of -Wstringop-overflow. */ char *d = MAX (p, q); - d[2] = 0; // { dg-warning "writing 4 bytes into a region of size 3" "" { target { i?86-*-* x86_64-*-* } } } + d[2] = 0; // { dg-warning "writing 4 bytes into a region of size 3" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* } } } d[3] = 0; d[4] = 0; - d[5] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* } } } + d[5] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } } @@ -44,10 +44,10 @@ void max_b6_b4 (int i) char *q = b4 + i; char *d = MAX (p, q); - d[3] = 0; // { dg-warning "writing 4 bytes into a region of size 3" "" { target { i?86-*-* x86_64-*-* } } } + d[3] = 0; // { dg-warning "writing 4 bytes into a region of size 3" "" { target { i?86-*-* x86_64-*-* powerpc*-*-* } } } d[4] = 0; d[5] = 0; - d[6] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* } } } + d[6] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } } @@ -82,7 +82,7 @@ void max_d8_p (char *q, int i) struct A3_5 { char a3[3]; // { dg-message "at offset 3 into destination object 'a3' of size 3" "pr??????" { xfail *-*-* } } - char a5[5]; // { dg-message "at offset 5 into destination object 'a5' of size 5" "note" { xfail { i?86-*-* x86_64-*-* } } } + char a5[5]; // { dg-message "at offset 5 into destination object 'a5' of size 5" "note" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } }; void max_A3_A5 (int i, struct A3_5 *pa3_5) @@ -95,14 +95,14 @@ void max_A3_A5 (int i, struct A3_5 *pa3_5) d[2] = 0; d[3] = 0; // { dg-warning "writing 1 byte into a region of size 0" "pr??????" { xfail *-*-* } } d[4] = 0; - d[5] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* } } } + d[5] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } } struct B4_B6 { char b4[4]; - char b6[6]; // { dg-message "at offset \[^a-zA-Z\n\r\]*6\[^a-zA-Z0-9\]* into destination object 'b6' of size 6" "note" { xfail { i?86-*-* x86_64-*-* } } } + char b6[6]; // { dg-message "at offset \[^a-zA-Z\n\r\]*6\[^a-zA-Z0-9\]* into destination object 'b6' of size 6" "note" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } }; void max_B6_B4 (int i, struct B4_B6 *pb4_b6) @@ -114,7 +114,7 @@ void max_B6_B4 (int i, struct B4_B6 *pb4_b6) d[3] = 0; d[4] = 0; d[5] = 0; - d[6] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* } } } + d[6] = 0; // { dg-warning "writing 1 byte into a region of size 0" "" { xfail { i?86-*-* x86_64-*-* powerpc*-*-* } } } } diff --git a/gcc/testsuite/gcc.target/powerpc/dform-1.c b/gcc/testsuite/gcc.target/powerpc/dform-1.c index fac39230fd6..1a0b0cf4c34 100644 --- a/gcc/testsuite/gcc.target/powerpc/dform-1.c +++ b/gcc/testsuite/gcc.target/powerpc/dform-1.c @@ -1,6 +1,8 @@ /* { dg-do compile { target { powerpc*-*-* && lp64 } } } */ /* { dg-require-effective-target powerpc_p9vector_ok } */ -/* { dg-options "-mpower9-vector -O2" } */ +/* Now O2 enables vectorization by default, which makes expected scalar + loads gone, so simply disable it. */ +/* { dg-options "-mpower9-vector -O2 -fno-tree-vectorize" } */ #ifndef TYPE #define TYPE double diff --git a/gcc/testsuite/gcc.target/powerpc/dform-2.c b/gcc/testsuite/gcc.target/powerpc/dform-2.c index 994733071e7..cc91f55b82d 100644 --- a/gcc/testsuite/gcc.target/powerpc/dform-2.c +++ b/gcc/testsuite/gcc.target/powerpc/dform-2.c @@ -1,6 +1,8 @@ /* { dg-do compile { target { powerpc*-*-* && lp64 } } } */ /* { dg-require-effective-target powerpc_p9vector_ok } */ -/* { dg-options "-mpower9-vector -O2" } */ +/* Now O2 enables vectorization by default, which generates unexpected float + conversion for vector construction, so simply disable it. */ +/* { dg-options "-mpower9-vector -O2 -fno-tree-vectorize" } */ #ifndef TYPE #define TYPE float diff --git a/gcc/testsuite/gcc.target/powerpc/pr80510-2.c b/gcc/testsuite/gcc.target/powerpc/pr80510-2.c index f85e005be64..d041d967c8b 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr80510-2.c +++ b/gcc/testsuite/gcc.target/powerpc/pr80510-2.c @@ -1,7 +1,9 @@ /* { dg-do compile { target { powerpc*-*-* } } } */ /* { dg-skip-if "" { powerpc*-*-darwin* } } */ /* { dg-require-effective-target powerpc_p8vector_ok } */ -/* { dg-options "-mdejagnu-cpu=power8 -O2" } */ +/* Now O2 enables vectorization by default, which generates unexpected VSR + to GPR movement for vector construction, so simply disable it. */ +/* { dg-options "-mdejagnu-cpu=power8 -O2 -fno-tree-vectorize" } */ /* Make sure that STXSSPX is generated for float scalars in Altivec registers on power7 instead of moving the value to a FPR register and doing a X-FORM