| Message ID | 5464E265.6050008@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore <sandra@codesourcery.com> wrote: > This patch to the AArch64 back end adds a couple of additional bics patterns > to match code of the form > > if ((x & y) == x) ...; > > This is testing whether the bits set in x are a subset of the bits set in y; > or, that no bits in x are set that are not set in y. So, it is equivalent > to > > if ((x & ~y) == 0) ...; > > Presently this generates code like > and x21, x21, x20 > cmp x21, x20 > b.eq c0 <main+0xc0> > > and this patch allows it to be written more concisely as: > bics x21, x20, x21 > b.eq c0 <main+0xc0> > > Since the bics instruction sets the condition codes itself, no explicit > comparison is required and the result of the bics computation can be > discarded. > > Regression-tested on aarch64-linux-gnu. OK to commit? Is this not a duplicate of https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? regards Ramana > > -Sandra > > > 2014-11-12 Sandra Loosemore <sandra@codesourcery.com> > Chris Jones <chrisj@nvidia.com> > > gcc/ > * config/aarch64/aarch64.md (*and<mode>3_compare_op1): New. > (*and<mode>3_compare_op2): New. > > gcc/testsuite/ > * gcc.target/aarch64/bics_3.c: New. > >
On 13/11/14 17:05, Ramana Radhakrishnan wrote: > On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore > <sandra@codesourcery.com> wrote: >> This patch to the AArch64 back end adds a couple of additional bics patterns >> to match code of the form >> >> if ((x & y) == x) ...; >> >> This is testing whether the bits set in x are a subset of the bits set in y; >> or, that no bits in x are set that are not set in y. So, it is equivalent >> to >> >> if ((x & ~y) == 0) ...; >> >> Presently this generates code like >> and x21, x21, x20 >> cmp x21, x20 >> b.eq c0 <main+0xc0> >> >> and this patch allows it to be written more concisely as: >> bics x21, x20, x21 >> b.eq c0 <main+0xc0> >> >> Since the bics instruction sets the condition codes itself, no explicit >> comparison is required and the result of the bics computation can be >> discarded. >> >> Regression-tested on aarch64-linux-gnu. OK to commit? > > Is this not a duplicate of > https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? > > regards > Ramana > >> >> -Sandra >> >> >> 2014-11-12 Sandra Loosemore <sandra@codesourcery.com> >> Chris Jones <chrisj@nvidia.com> >> >> gcc/ >> * config/aarch64/aarch64.md (*and<mode>3_compare_op1): New. >> (*and<mode>3_compare_op2): New. >> >> gcc/testsuite/ >> * gcc.target/aarch64/bics_3.c: New. >> >> > I don't think so. However, I think it is something that should be caught in generic simplification code ie map ((a & b) == b) ==> ((~a & b) == 0), etc Bit-clear operations are not that uncommon. Furthermore, A may be a constant. R.
On 11/13/2014 10:27 AM, Richard Earnshaw wrote: > On 13/11/14 17:05, Ramana Radhakrishnan wrote: >> On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore >> <sandra@codesourcery.com> wrote: >>> This patch to the AArch64 back end adds a couple of additional bics patterns >>> to match code of the form >>> >>> if ((x & y) == x) ...; >>> >>> This is testing whether the bits set in x are a subset of the bits set in y; >>> or, that no bits in x are set that are not set in y. So, it is equivalent >>> to >>> >>> if ((x & ~y) == 0) ...; >>> >>> Presently this generates code like >>> and x21, x21, x20 >>> cmp x21, x20 >>> b.eq c0 <main+0xc0> >>> >>> and this patch allows it to be written more concisely as: >>> bics x21, x20, x21 >>> b.eq c0 <main+0xc0> >>> >>> Since the bics instruction sets the condition codes itself, no explicit >>> comparison is required and the result of the bics computation can be >>> discarded. >>> >>> Regression-tested on aarch64-linux-gnu. OK to commit? >> >> Is this not a duplicate of >> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? >> >> > I don't think so. However, I think it is something that should be > caught in generic simplification code > > ie map ((a & b) == b) ==> ((~a & b) == 0), etc > > Bit-clear operations are not that uncommon. Furthermore, A may be a > constant. Alex posted his patch when I already had Chris's in my regression test queue, but I've just confirmed that it does not fix the test case I included. I already thought a little about making this a generic simplification, but it seemed to me like it was only useful on targets that have a bit-clear instruction that happens to set condition codes, and that it would pessimize code on targets that don't have a bit-clear instruction at all (by inserting the extra complement operation). So to me it seemed reasonable to do it in the back end. -Sandra
On Thu, Nov 13, 2014 at 9:42 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > On 11/13/2014 10:27 AM, Richard Earnshaw wrote: >> >> On 13/11/14 17:05, Ramana Radhakrishnan wrote: >>> >>> On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore >>> <sandra@codesourcery.com> wrote: >>>> >>>> This patch to the AArch64 back end adds a couple of additional bics >>>> patterns >>>> to match code of the form >>>> >>>> if ((x & y) == x) ...; >>>> >>>> This is testing whether the bits set in x are a subset of the bits set >>>> in y; >>>> or, that no bits in x are set that are not set in y. So, it is >>>> equivalent >>>> to >>>> >>>> if ((x & ~y) == 0) ...; >>>> >>>> Presently this generates code like >>>> and x21, x21, x20 >>>> cmp x21, x20 >>>> b.eq c0 <main+0xc0> >>>> >>>> and this patch allows it to be written more concisely as: >>>> bics x21, x20, x21 >>>> b.eq c0 <main+0xc0> >>>> >>>> Since the bics instruction sets the condition codes itself, no explicit >>>> comparison is required and the result of the bics computation can be >>>> discarded. >>>> >>>> Regression-tested on aarch64-linux-gnu. OK to commit? >>> >>> >>> Is this not a duplicate of >>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? >>> >>> >> I don't think so. However, I think it is something that should be >> caught in generic simplification code >> >> ie map ((a & b) == b) ==> ((~a & b) == 0), etc >> >> Bit-clear operations are not that uncommon. Furthermore, A may be a >> constant. > > > Alex posted his patch when I already had Chris's in my regression test > queue, but I've just confirmed that it does not fix the test case I > included. > > I already thought a little about making this a generic simplification, but > it seemed to me like it was only useful on targets that have a bit-clear > instruction that happens to set condition codes, and that it would pessimize > code on targets that don't have a bit-clear instruction at all (by inserting > the extra complement operation). So to me it seemed reasonable to do it in > the back end. But can't you do this in simplify-rtx.c and allow for the cost model to do the correct thing? Thanks, Andrew > > -Sandra >
On 11/13/2014 10:47 AM, Andrew Pinski wrote: > On Thu, Nov 13, 2014 at 9:42 AM, Sandra Loosemore > <sandra@codesourcery.com> wrote: >> On 11/13/2014 10:27 AM, Richard Earnshaw wrote: >>> >>> On 13/11/14 17:05, Ramana Radhakrishnan wrote: >>>> >>>> On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore >>>> <sandra@codesourcery.com> wrote: >>>>> >>>>> This patch to the AArch64 back end adds a couple of additional bics >>>>> patterns >>>>> to match code of the form >>>>> >>>>> if ((x & y) == x) ...; >>>>> >>>>> This is testing whether the bits set in x are a subset of the bits set >>>>> in y; >>>>> or, that no bits in x are set that are not set in y. So, it is >>>>> equivalent >>>>> to >>>>> >>>>> if ((x & ~y) == 0) ...; >>>>> >>>>> Presently this generates code like >>>>> and x21, x21, x20 >>>>> cmp x21, x20 >>>>> b.eq c0 <main+0xc0> >>>>> >>>>> and this patch allows it to be written more concisely as: >>>>> bics x21, x20, x21 >>>>> b.eq c0 <main+0xc0> >>>>> >>>>> Since the bics instruction sets the condition codes itself, no explicit >>>>> comparison is required and the result of the bics computation can be >>>>> discarded. >>>>> >>>>> Regression-tested on aarch64-linux-gnu. OK to commit? >>>> >>>> >>>> Is this not a duplicate of >>>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? >>>> >>>> >>> I don't think so. However, I think it is something that should be >>> caught in generic simplification code >>> >>> ie map ((a & b) == b) ==> ((~a & b) == 0), etc >>> >>> Bit-clear operations are not that uncommon. Furthermore, A may be a >>> constant. >> >> >> Alex posted his patch when I already had Chris's in my regression test >> queue, but I've just confirmed that it does not fix the test case I >> included. >> >> I already thought a little about making this a generic simplification, but >> it seemed to me like it was only useful on targets that have a bit-clear >> instruction that happens to set condition codes, and that it would pessimize >> code on targets that don't have a bit-clear instruction at all (by inserting >> the extra complement operation). So to me it seemed reasonable to do it in >> the back end. > > But can't you do this in simplify-rtx.c and allow for the cost model > to do the correct thing? I could give that a shot, but it seems unlikely that I will be able to complete the patch rewrite and testing before we are in Stage 3. If I have something ready next week, will it be too late for consideration in GCC 5? -Sandra
On 13/11/14 17:42, Sandra Loosemore wrote: > On 11/13/2014 10:27 AM, Richard Earnshaw wrote: >> On 13/11/14 17:05, Ramana Radhakrishnan wrote: >>> On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore >>> <sandra@codesourcery.com> wrote: >>>> This patch to the AArch64 back end adds a couple of additional bics patterns >>>> to match code of the form >>>> >>>> if ((x & y) == x) ...; >>>> >>>> This is testing whether the bits set in x are a subset of the bits set in y; >>>> or, that no bits in x are set that are not set in y. So, it is equivalent >>>> to >>>> >>>> if ((x & ~y) == 0) ...; >>>> >>>> Presently this generates code like >>>> and x21, x21, x20 >>>> cmp x21, x20 >>>> b.eq c0 <main+0xc0> >>>> >>>> and this patch allows it to be written more concisely as: >>>> bics x21, x20, x21 >>>> b.eq c0 <main+0xc0> >>>> >>>> Since the bics instruction sets the condition codes itself, no explicit >>>> comparison is required and the result of the bics computation can be >>>> discarded. >>>> >>>> Regression-tested on aarch64-linux-gnu. OK to commit? >>> >>> Is this not a duplicate of >>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? >>> >>> >> I don't think so. However, I think it is something that should be >> caught in generic simplification code >> >> ie map ((a & b) == b) ==> ((~a & b) == 0), etc >> >> Bit-clear operations are not that uncommon. Furthermore, A may be a >> constant. > > Alex posted his patch when I already had Chris's in my regression test > queue, but I've just confirmed that it does not fix the test case I > included. Alex's patch is adding a proper pattern for the BICS instruction. I wouldn't expect it to directly achieve what you are trying to do - but I do think the compiler should transform what you have into the form that Alex has just added the patterns for. > > I already thought a little about making this a generic simplification, > but it seemed to me like it was only useful on targets that have a > bit-clear instruction that happens to set condition codes, and that it > would pessimize code on targets that don't have a bit-clear instruction > at all (by inserting the extra complement operation). So to me it > seemed reasonable to do it in the back end. I doubt that. I'd be surprised if any target had a direct ((A & B) == A) type comparison, so all you're doing is canonicalizing something that no target is likely to have into something some targets have. And ulitimately, if the pattern doesn't match, combine will just undo all the changes. Furthermore, as I previously said, if B is a constant then you're going to get a real optimization for that case that is widely applicable. Eg A & ~1 == A will simplify into A & 1 == 0 Many targets will have a flag-setting AND operation. R.
On 13/11/14 22:44, Sandra Loosemore wrote: > On 11/13/2014 10:47 AM, Andrew Pinski wrote: >> On Thu, Nov 13, 2014 at 9:42 AM, Sandra Loosemore >> <sandra@codesourcery.com> wrote: >>> On 11/13/2014 10:27 AM, Richard Earnshaw wrote: >>>> >>>> On 13/11/14 17:05, Ramana Radhakrishnan wrote: >>>>> >>>>> On Thu, Nov 13, 2014 at 4:55 PM, Sandra Loosemore >>>>> <sandra@codesourcery.com> wrote: >>>>>> >>>>>> This patch to the AArch64 back end adds a couple of additional bics >>>>>> patterns >>>>>> to match code of the form >>>>>> >>>>>> if ((x & y) == x) ...; >>>>>> >>>>>> This is testing whether the bits set in x are a subset of the bits set >>>>>> in y; >>>>>> or, that no bits in x are set that are not set in y. So, it is >>>>>> equivalent >>>>>> to >>>>>> >>>>>> if ((x & ~y) == 0) ...; >>>>>> >>>>>> Presently this generates code like >>>>>> and x21, x21, x20 >>>>>> cmp x21, x20 >>>>>> b.eq c0 <main+0xc0> >>>>>> >>>>>> and this patch allows it to be written more concisely as: >>>>>> bics x21, x20, x21 >>>>>> b.eq c0 <main+0xc0> >>>>>> >>>>>> Since the bics instruction sets the condition codes itself, no explicit >>>>>> comparison is required and the result of the bics computation can be >>>>>> discarded. >>>>>> >>>>>> Regression-tested on aarch64-linux-gnu. OK to commit? >>>>> >>>>> >>>>> Is this not a duplicate of >>>>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00943.html ? >>>>> >>>>> >>>> I don't think so. However, I think it is something that should be >>>> caught in generic simplification code >>>> >>>> ie map ((a & b) == b) ==> ((~a & b) == 0), etc >>>> >>>> Bit-clear operations are not that uncommon. Furthermore, A may be a >>>> constant. >>> >>> >>> Alex posted his patch when I already had Chris's in my regression test >>> queue, but I've just confirmed that it does not fix the test case I >>> included. >>> >>> I already thought a little about making this a generic simplification, but >>> it seemed to me like it was only useful on targets that have a bit-clear >>> instruction that happens to set condition codes, and that it would pessimize >>> code on targets that don't have a bit-clear instruction at all (by inserting >>> the extra complement operation). So to me it seemed reasonable to do it in >>> the back end. >> >> But can't you do this in simplify-rtx.c and allow for the cost model >> to do the correct thing? > > I could give that a shot, but it seems unlikely that I will be able to > complete the patch rewrite and testing before we are in Stage 3. If I > have something ready next week, will it be too late for consideration in > GCC 5? > You're following up on a previously submitted patch and you're not rewriting large chunks of the compiler. I don't think this is really that complicated so I'd be surprised if anyone strongly objected because it wasn't in final form before the end of stage1. I would recommend you try to get it at least re-submitted before the end of the year, though. R.
Index: gcc/config/aarch64/aarch64.md =================================================================== --- gcc/config/aarch64/aarch64.md (revision 217322) +++ gcc/config/aarch64/aarch64.md (working copy) @@ -2894,6 +2894,32 @@ [(set_attr "type" "logics_shift_imm")] ) +;; ((a & b) == a) ==> ((a & ~b) == 0) +(define_insn "*and<mode>3_compare_op1" + [(set (reg:CC CC_REGNUM) + (compare:CC + (and:GPI (match_operand:GPI 1 "register_operand" "r") + (match_operand:GPI 2 "register_operand" "r")) + (match_dup 1))) + (clobber (match_scratch:GPI 0 "=r"))] + "" + "bics\\t%<w>0, %<w>1, %<w>2" + [(set_attr "type" "logics_reg")] +) + +;; ((a & b) == b) ==> ((b & ~a) == 0) +(define_insn "*and<mode>3_compare_op2" + [(set (reg:CC CC_REGNUM) + (compare:CC + (and:GPI (match_operand:GPI 1 "register_operand" "r") + (match_operand:GPI 2 "register_operand" "r")) + (match_dup 2))) + (clobber (match_scratch:GPI 0 "=r"))] + "" + "bics\\t%<w>0, %<w>2, %<w>1" + [(set_attr "type" "logics_reg")] +) + (define_insn "clz<mode>2" [(set (match_operand:GPI 0 "register_operand" "=r") (clz:GPI (match_operand:GPI 1 "register_operand" "r")))] Index: gcc/testsuite/gcc.target/aarch64/bics_3.c =================================================================== --- gcc/testsuite/gcc.target/aarch64/bics_3.c (revision 0) +++ gcc/testsuite/gcc.target/aarch64/bics_3.c (revision 0) @@ -0,0 +1,87 @@ +/* { dg-do run } */ +/* { dg-options "-O2 --save-temps -fno-inline" } */ + +extern void abort (void); + +int +bics_si_test1 (int a, int b, int c) +{ + if ((a & b) == a) + return a; + else + return c; +} + +int +bics_si_test2 (int a, int b, int c) +{ + if ((a & b) == b) + return b; + else + return c; +} + +typedef long long s64; + +s64 +bics_di_test1 (s64 a, s64 b, s64 c) +{ + if ((a & b) == a) + return a; + else + return c; +} + +s64 +bics_di_test2 (s64 a, s64 b, s64 c) +{ + if ((a & b) == b) + return b; + else + return c; +} + +int +main () +{ + int x; + s64 y; + + x = bics_si_test1 (0xf00d, 0xf11f, 0); + if (x != 0xf00d) + abort (); + + x = bics_si_test1 (0xf11f, 0xf00d, 0); + if (x != 0) + abort (); + + x = bics_si_test2 (0xf00d, 0xf11f, 0); + if (x != 0) + abort (); + + x = bics_si_test2 (0xf11f, 0xf00d, 0); + if (x != 0xf00d) + abort (); + + y = bics_di_test1 (0x10001000f00dll, 0x12341000f00dll, 0ll); + if (y != 0x10001000f00dll) + abort (); + + y = bics_di_test1 (0x12341000f00dll, 0x10001000f00dll, 0ll); + if (y != 0) + abort (); + + y = bics_di_test2 (0x10001000f00dll, 0x12341000f00dll, 0ll); + if (y != 0) + abort (); + + y = bics_di_test2 (0x12341000f00dll, 0x10001000f00dll, 0ll); + if (y != 0x10001000f00dll) + abort (); + + return 0; +} + +/* { dg-final { scan-assembler-times "bics\tw\[0-9\]+, w\[0-9\]+, w\[0-9\]+" 2 } } */ +/* { dg-final { scan-assembler-times "bics\tx\[0-9\]+, x\[0-9\]+, x\[0-9\]+" 2 } } */ +/* { dg-final { cleanup-saved-temps } } */