Message ID | patch-18218-tamar@arm.com |
---|---|
State | New |
Headers | show |
Series | AArch64: relax cbranch tests to accepted inverted branches [PR113502] | expand |
Tamar Christina <tamar.christina@arm.com> writes: > Hi All, > > Recently something in the midend had started inverting the branches by inverting > the condition and the branches. > > While this is fine, it makes it hard to actually test. In RTL I disable > scheduling and BB reordering to prevent this. But in GIMPLE there seems to be > nothing I can do. __builtin_expect seems to have no impact on the change since > I suspect this is happening during expand where conditions can be flipped > regardless of probability during compare_and_branch. > > Since the mid-end has plenty of correctness tests, this weakens the backend > tests to just check that a correct looking sequence is emitted. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/testsuite/ChangeLog: > > PR testsuite/113502 > * gcc.target/aarch64/sve/vect-early-break-cbranch.c: Ignore exact branch. > * gcc.target/aarch64/vect-early-break-cbranch.c: Likewise. OK I guess, since I agree that the "polarity" of the branch isn't really the thing that we're trying to test. But the fact that even __builtin_expect doesn't work seems like a bug. Do we have a PR for that? Might be worth filing one (for GCC 15+) if we don't. Thanks, Richard > > --- inline copy of patch -- > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c b/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > index d15053553f94e7dce3540e21f0c1f0d39ea4f289..d7cef1105410be04ed67d1d3b800746267f205a8 100644 > --- a/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > +++ b/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > @@ -9,7 +9,7 @@ int b[N] = {0}; > ** ... > ** cmpgt p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any \.L[0-9]+ > +** b.(any|none) \.L[0-9]+ > ** ... > */ > void f1 () > @@ -26,7 +26,7 @@ void f1 () > ** ... > ** cmpge p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any \.L[0-9]+ > +** b.(any|none) \.L[0-9]+ > ** ... > */ > void f2 () > @@ -43,7 +43,7 @@ void f2 () > ** ... > ** cmpeq p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any \.L[0-9]+ > +** b.(any|none) \.L[0-9]+ > ** ... > */ > void f3 () > @@ -60,7 +60,7 @@ void f3 () > ** ... > ** cmpne p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any \.L[0-9]+ > +** b.(any|none) \.L[0-9]+ > ** ... > */ > void f4 () > @@ -77,7 +77,7 @@ void f4 () > ** ... > ** cmplt p[0-9]+.s, p7/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any .L[0-9]+ > +** b.(any|none) .L[0-9]+ > ** ... > */ > void f5 () > @@ -94,7 +94,7 @@ void f5 () > ** ... > ** cmple p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > ** ptest p[0-9]+, p[0-9]+.b > -** b.any \.L[0-9]+ > +** b.(any|none) \.L[0-9]+ > ** ... > */ > void f6 () > diff --git a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > index a5e7b94827dd70240d754a834f1d11750a9c27a9..673b781eb6d092f6311409797b20a971f4fae247 100644 > --- a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > +++ b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > @@ -15,7 +15,7 @@ int b[N] = {0}; > ** cmgt v[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f1 () > @@ -34,7 +34,7 @@ void f1 () > ** cmge v[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f2 () > @@ -53,7 +53,7 @@ void f2 () > ** cmeq v[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f3 () > @@ -72,7 +72,7 @@ void f3 () > ** cmtst v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f4 () > @@ -91,7 +91,7 @@ void f4 () > ** cmlt v[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f5 () > @@ -110,7 +110,7 @@ void f5 () > ** cmle v[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmov x[0-9]+, d[0-9]+ > -** cbnz x[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f6 ()
On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Tamar Christina <tamar.christina@arm.com> writes: > > Hi All, > > > > Recently something in the midend had started inverting the branches by inverting > > the condition and the branches. > > > > While this is fine, it makes it hard to actually test. In RTL I disable > > scheduling and BB reordering to prevent this. But in GIMPLE there seems to be > > nothing I can do. __builtin_expect seems to have no impact on the change since > > I suspect this is happening during expand where conditions can be flipped > > regardless of probability during compare_and_branch. > > > > Since the mid-end has plenty of correctness tests, this weakens the backend > > tests to just check that a correct looking sequence is emitted. > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/testsuite/ChangeLog: > > > > PR testsuite/113502 > > * gcc.target/aarch64/sve/vect-early-break-cbranch.c: Ignore exact branch. > > * gcc.target/aarch64/vect-early-break-cbranch.c: Likewise. > > OK I guess, since I agree that the "polarity" of the branch isn't really the > thing that we're trying to test. But the fact that even __builtin_expect > doesn't work seems like a bug. Do we have a PR for that? Might be worth > filing one (for GCC 15+) if we don't. But that only should affect which edge is fallthru, not the "polarity" of the branch, no? > Thanks, > Richard > > > > > --- inline copy of patch -- > > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c b/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > > index d15053553f94e7dce3540e21f0c1f0d39ea4f289..d7cef1105410be04ed67d1d3b800746267f205a8 100644 > > --- a/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > > +++ b/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c > > @@ -9,7 +9,7 @@ int b[N] = {0}; > > ** ... > > ** cmpgt p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any \.L[0-9]+ > > +** b.(any|none) \.L[0-9]+ > > ** ... > > */ > > void f1 () > > @@ -26,7 +26,7 @@ void f1 () > > ** ... > > ** cmpge p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any \.L[0-9]+ > > +** b.(any|none) \.L[0-9]+ > > ** ... > > */ > > void f2 () > > @@ -43,7 +43,7 @@ void f2 () > > ** ... > > ** cmpeq p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any \.L[0-9]+ > > +** b.(any|none) \.L[0-9]+ > > ** ... > > */ > > void f3 () > > @@ -60,7 +60,7 @@ void f3 () > > ** ... > > ** cmpne p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any \.L[0-9]+ > > +** b.(any|none) \.L[0-9]+ > > ** ... > > */ > > void f4 () > > @@ -77,7 +77,7 @@ void f4 () > > ** ... > > ** cmplt p[0-9]+.s, p7/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any .L[0-9]+ > > +** b.(any|none) .L[0-9]+ > > ** ... > > */ > > void f5 () > > @@ -94,7 +94,7 @@ void f5 () > > ** ... > > ** cmple p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 > > ** ptest p[0-9]+, p[0-9]+.b > > -** b.any \.L[0-9]+ > > +** b.(any|none) \.L[0-9]+ > > ** ... > > */ > > void f6 () > > diff --git a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > > index a5e7b94827dd70240d754a834f1d11750a9c27a9..673b781eb6d092f6311409797b20a971f4fae247 100644 > > --- a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > > +++ b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c > > @@ -15,7 +15,7 @@ int b[N] = {0}; > > ** cmgt v[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f1 () > > @@ -34,7 +34,7 @@ void f1 () > > ** cmge v[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f2 () > > @@ -53,7 +53,7 @@ void f2 () > > ** cmeq v[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f3 () > > @@ -72,7 +72,7 @@ void f3 () > > ** cmtst v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f4 () > > @@ -91,7 +91,7 @@ void f4 () > > ** cmlt v[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f5 () > > @@ -110,7 +110,7 @@ void f5 () > > ** cmle v[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmov x[0-9]+, d[0-9]+ > > -** cbnz x[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f6 ()
Richard Biener <richard.guenther@gmail.com> writes: > On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Tamar Christina <tamar.christina@arm.com> writes: >> > Hi All, >> > >> > Recently something in the midend had started inverting the branches by inverting >> > the condition and the branches. >> > >> > While this is fine, it makes it hard to actually test. In RTL I disable >> > scheduling and BB reordering to prevent this. But in GIMPLE there seems to be >> > nothing I can do. __builtin_expect seems to have no impact on the change since >> > I suspect this is happening during expand where conditions can be flipped >> > regardless of probability during compare_and_branch. >> > >> > Since the mid-end has plenty of correctness tests, this weakens the backend >> > tests to just check that a correct looking sequence is emitted. >> > >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> > >> > Ok for master? >> > >> > Thanks, >> > Tamar >> > >> > gcc/testsuite/ChangeLog: >> > >> > PR testsuite/113502 >> > * gcc.target/aarch64/sve/vect-early-break-cbranch.c: Ignore exact branch. >> > * gcc.target/aarch64/vect-early-break-cbranch.c: Likewise. >> >> OK I guess, since I agree that the "polarity" of the branch isn't really the >> thing that we're trying to test. But the fact that even __builtin_expect >> doesn't work seems like a bug. Do we have a PR for that? Might be worth >> filing one (for GCC 15+) if we don't. > > But that only should affect which edge is fallthru, not the "polarity" > of the branch, no? Right, that's what I meant by "polarity". I should have used a proper term, sorry. The conditions in the patch are inverses of each other, but it seems like the choice between them should be predictable if the branch is very likely or very unlikely. Thanks, Richard
On Tue, Jan 30, 2024 at 11:40 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Richard Biener <richard.guenther@gmail.com> writes: > > On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Tamar Christina <tamar.christina@arm.com> writes: > >> > Hi All, > >> > > >> > Recently something in the midend had started inverting the branches by inverting > >> > the condition and the branches. > >> > > >> > While this is fine, it makes it hard to actually test. In RTL I disable > >> > scheduling and BB reordering to prevent this. But in GIMPLE there seems to be > >> > nothing I can do. __builtin_expect seems to have no impact on the change since > >> > I suspect this is happening during expand where conditions can be flipped > >> > regardless of probability during compare_and_branch. > >> > > >> > Since the mid-end has plenty of correctness tests, this weakens the backend > >> > tests to just check that a correct looking sequence is emitted. > >> > > >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > >> > > >> > Ok for master? > >> > > >> > Thanks, > >> > Tamar > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > PR testsuite/113502 > >> > * gcc.target/aarch64/sve/vect-early-break-cbranch.c: Ignore exact branch. > >> > * gcc.target/aarch64/vect-early-break-cbranch.c: Likewise. > >> > >> OK I guess, since I agree that the "polarity" of the branch isn't really the > >> thing that we're trying to test. But the fact that even __builtin_expect > >> doesn't work seems like a bug. Do we have a PR for that? Might be worth > >> filing one (for GCC 15+) if we don't. > > > > But that only should affect which edge is fallthru, not the "polarity" > > of the branch, no? > > Right, that's what I meant by "polarity". I should have used a proper > term, sorry. The conditions in the patch are inverses of each other, > but it seems like the choice between them should be predictable if > the branch is very likely or very unlikely. I'm not sure, esp. if you disable BB reordering which might be the one flipping the polarity to keep fallthru/backedge in-order. Richard. > Thanks, > Richard >
--- a/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c +++ b/gcc/testsuite/gcc.target/aarch64/sve/vect-early-break-cbranch.c @@ -9,7 +9,7 @@ int b[N] = {0}; ** ... ** cmpgt p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any \.L[0-9]+ +** b.(any|none) \.L[0-9]+ ** ... */ void f1 () @@ -26,7 +26,7 @@ void f1 () ** ... ** cmpge p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any \.L[0-9]+ +** b.(any|none) \.L[0-9]+ ** ... */ void f2 () @@ -43,7 +43,7 @@ void f2 () ** ... ** cmpeq p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any \.L[0-9]+ +** b.(any|none) \.L[0-9]+ ** ... */ void f3 () @@ -60,7 +60,7 @@ void f3 () ** ... ** cmpne p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any \.L[0-9]+ +** b.(any|none) \.L[0-9]+ ** ... */ void f4 () @@ -77,7 +77,7 @@ void f4 () ** ... ** cmplt p[0-9]+.s, p7/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any .L[0-9]+ +** b.(any|none) .L[0-9]+ ** ... */ void f5 () @@ -94,7 +94,7 @@ void f5 () ** ... ** cmple p[0-9]+.s, p[0-9]+/z, z[0-9]+.s, #0 ** ptest p[0-9]+, p[0-9]+.b -** b.any \.L[0-9]+ +** b.(any|none) \.L[0-9]+ ** ... */ void f6 () diff --git a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c index a5e7b94827dd70240d754a834f1d11750a9c27a9..673b781eb6d092f6311409797b20a971f4fae247 100644 --- a/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c +++ b/gcc/testsuite/gcc.target/aarch64/vect-early-break-cbranch.c @@ -15,7 +15,7 @@ int b[N] = {0}; ** cmgt v[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f1 () @@ -34,7 +34,7 @@ void f1 () ** cmge v[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f2 () @@ -53,7 +53,7 @@ void f2 () ** cmeq v[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f3 () @@ -72,7 +72,7 @@ void f3 () ** cmtst v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f4 () @@ -91,7 +91,7 @@ void f4 () ** cmlt v[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f5 () @@ -110,7 +110,7 @@ void f5 () ** cmle v[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmov x[0-9]+, d[0-9]+ -** cbnz x[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f6 ()