Re: [PATCH]AArch64: relax cbranch tests to accepted inverted branches [PR113502]
On Tue, Jan 30, 2024 at 11:40 AM Richard Sandiford wrote: > > Richard Biener writes: > > On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford > > wrote: > >> > >> Tamar Christina 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 >
Re: [PATCH]AArch64: relax cbranch tests to accepted inverted branches [PR113502]
Richard Biener writes: > On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford > wrote: >> >> Tamar Christina 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
Re: [PATCH]AArch64: relax cbranch tests to accepted inverted branches [PR113502]
On Mon, Jan 29, 2024 at 5:00 PM Richard Sandiford wrote: > > Tamar Christina 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}; > > ** cmgtv[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmovx[0-9]+, d[0-9]+ > > -** cbnzx[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f1 () > > @@ -34,7 +34,7 @@ void f1 () > > ** cmgev[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmovx[0-9]+, d[0-9]+ > > -** cbnzx[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f2 () > > @@ -53,7 +53,7 @@ void f2 () > > ** cmeqv[0-9]+.4s, v[0-9]+.4s, #0 > > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > ** fmovx[0-9]+, d[0-9]+ > > -** cbnzx[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 > > ** fmovx[0-9]+, d[0-9]+ > > -** cbnzx[0-9]+, \.L[0-9]+ > > +** cbn?z x[0-9]+, \.L[0-9]+ > > ** ... > > */ > > void f4 () > > @@ -91,7 +91,7 @@ void f4 () > > ** cmltv[0-9]+.4s, v[0-9]+.4s, #0 > > **
Re: [PATCH]AArch64: relax cbranch tests to accepted inverted branches [PR113502]
Tamar Christina 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}; > ** cmgtv[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f1 () > @@ -34,7 +34,7 @@ void f1 () > ** cmgev[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f2 () > @@ -53,7 +53,7 @@ void f2 () > ** cmeqv[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[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 > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f4 () > @@ -91,7 +91,7 @@ void f4 () > ** cmltv[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f5 () > @@ -110,7 +110,7 @@ void f5 () > ** cmlev[0-9]+.4s, v[0-9]+.4s, #0 > ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > ** fmovx[0-9]+, d[0-9]+ > -** cbnzx[0-9]+, \.L[0-9]+ > +** cbn?z x[0-9]+, \.L[0-9]+ > ** ... > */ > void f6 ()
[PATCH]AArch64: relax cbranch tests to accepted inverted branches [PR113502]
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. --- 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}; ** cmgtv[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmovx[0-9]+, d[0-9]+ -** cbnzx[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f1 () @@ -34,7 +34,7 @@ void f1 () ** cmgev[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmovx[0-9]+, d[0-9]+ -** cbnzx[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f2 () @@ -53,7 +53,7 @@ void f2 () ** cmeqv[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmovx[0-9]+, d[0-9]+ -** cbnzx[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 ** fmovx[0-9]+, d[0-9]+ -** cbnzx[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f4 () @@ -91,7 +91,7 @@ void f4 () ** cmltv[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmovx[0-9]+, d[0-9]+ -** cbnzx[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f5 () @@ -110,7 +110,7 @@ void f5 () ** cmlev[0-9]+.4s, v[0-9]+.4s, #0 ** umaxp v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s ** fmovx[0-9]+, d[0-9]+ -** cbnzx[0-9]+, \.L[0-9]+ +** cbn?z x[0-9]+, \.L[0-9]+ ** ... */ void f6 () -- 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,