| Message ID | CA+=Sn1n0MJ5ifh5hz3P9jABGom2REi5Sc5A9=2J1Zsw+1QZKuw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > Right now we only produce ins when a zero_extract is used on the > right hand side. We can do better by adding some patterns which > combine for the ins instruction. This patch adds those patterns and a > testcase which shows a simple example where the code is improved. Sorry for the delay in reviewing this. Had you thought about trying to teach combine.c about this instead? It doesn't look like any of the patterns are really providing more information about the underlying instruction. At the moment the patch has things like: > +(define_insn "*insv<mode>_internal1" > + [(set (match_operand:GPR 0 "register_operand" "=d") > + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") > + (match_operand:GPR 2 "const_int_operand" "i")) > + (and:GPR (match_operand:GPR 3 "register_operand" "d") > + (match_operand:GPR 4 "const_int_operand" "i"))))] > + "ISA_HAS_EXT_INS && mips_bottom_bitmask_p (INTVAL (operands[4])) > + && INTVAL(operands[2]) == ~INTVAL(operands[4])" > +{ > + int len, pos; > + pos = mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); > + operands[2] = GEN_INT (pos); > + operands[4] = GEN_INT (len); > + return "<d>ins\t%0,%3,%2,%4"; > +} > + [(set_attr "type" "arith") > + (set_attr "mode" "<MODE>")]) but AFAIK there's nothing to guarantee that the bottom bitmask will be operand 2 rather than operand 4, so if we really do need do this via patterns, I think we'd need both orderrs. But if we do it this way, I assume every backend that provides an insv pattern will need to cut-&-paste these patterns too. Richard
On Sun, Aug 19, 2012 at 10:13 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: >> Right now we only produce ins when a zero_extract is used on the >> right hand side. We can do better by adding some patterns which >> combine for the ins instruction. This patch adds those patterns and a >> testcase which shows a simple example where the code is improved. > > Sorry for the delay in reviewing this. Had you thought about trying to > teach combine.c about this instead? It doesn't look like any of the > patterns are really providing more information about the underlying > instruction. combine.c has some code to do this already if one of the src register is the same as the dest register; that is what make_field_assignment does. Quickly looking at the code, the problem I doing it in make_field_assignment is there is no way to return that you need a copy of the value first unless I am missing something obvious. Now I agree we should be optimize this in combine rather than these manual patterns. Thanks, Andrew Pinski > > At the moment the patch has things like: > >> +(define_insn "*insv<mode>_internal1" >> + [(set (match_operand:GPR 0 "register_operand" "=d") >> + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") >> + (match_operand:GPR 2 "const_int_operand" "i")) >> + (and:GPR (match_operand:GPR 3 "register_operand" "d") >> + (match_operand:GPR 4 "const_int_operand" "i"))))] >> + "ISA_HAS_EXT_INS && mips_bottom_bitmask_p (INTVAL (operands[4])) >> + && INTVAL(operands[2]) == ~INTVAL(operands[4])" >> +{ >> + int len, pos; >> + pos = mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); >> + operands[2] = GEN_INT (pos); >> + operands[4] = GEN_INT (len); >> + return "<d>ins\t%0,%3,%2,%4"; >> +} >> + [(set_attr "type" "arith") >> + (set_attr "mode" "<MODE>")]) > > but AFAIK there's nothing to guarantee that the bottom bitmask > will be operand 2 rather than operand 4, so if we really do need > do this via patterns, I think we'd need both orderrs. > > But if we do it this way, I assume every backend that provides > an insv pattern will need to cut-&-paste these patterns too. > > Richard >
On Fri, Oct 5, 2012 at 8:43 PM, Andrew Pinski <andrew.pinski@caviumnetworks.com> wrote: > On Sun, Aug 19, 2012 at 10:13 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: >>> Right now we only produce ins when a zero_extract is used on the >>> right hand side. We can do better by adding some patterns which >>> combine for the ins instruction. This patch adds those patterns and a >>> testcase which shows a simple example where the code is improved. >> >> Sorry for the delay in reviewing this. Had you thought about trying to >> teach combine.c about this instead? It doesn't look like any of the >> patterns are really providing more information about the underlying >> instruction. > > combine.c has some code to do this already if one of the src register > is the same as the dest register; that is what make_field_assignment > does. Quickly looking at the code, the problem I doing it in > make_field_assignment is there is no way to return that you need a > copy of the value first unless I am missing something obvious. Now I > agree we should be optimize this in combine rather than these manual > patterns. I now have a patch which implements this in combine which allows the backend not need to change. I generate a SEQUENCE which then try_combine splits like we do for PARALLEL but keeping it in the correct order and allowing for the case where we are combing two instructions into two instructions. I hope to be able to post it later on Saturday. Thanks, Andrew Pinski > > Thanks, > Andrew Pinski > > >> >> At the moment the patch has things like: >> >>> +(define_insn "*insv<mode>_internal1" >>> + [(set (match_operand:GPR 0 "register_operand" "=d") >>> + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") >>> + (match_operand:GPR 2 "const_int_operand" "i")) >>> + (and:GPR (match_operand:GPR 3 "register_operand" "d") >>> + (match_operand:GPR 4 "const_int_operand" "i"))))] >>> + "ISA_HAS_EXT_INS && mips_bottom_bitmask_p (INTVAL (operands[4])) >>> + && INTVAL(operands[2]) == ~INTVAL(operands[4])" >>> +{ >>> + int len, pos; >>> + pos = mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); >>> + operands[2] = GEN_INT (pos); >>> + operands[4] = GEN_INT (len); >>> + return "<d>ins\t%0,%3,%2,%4"; >>> +} >>> + [(set_attr "type" "arith") >>> + (set_attr "mode" "<MODE>")]) >> >> but AFAIK there's nothing to guarantee that the bottom bitmask >> will be operand 2 rather than operand 4, so if we really do need >> do this via patterns, I think we'd need both orderrs. >> >> But if we do it this way, I assume every backend that provides >> an insv pattern will need to cut-&-paste these patterns too. >> >> Richard >>
Hi Andrew, Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > On Fri, Oct 5, 2012 at 8:43 PM, Andrew Pinski > <andrew.pinski@caviumnetworks.com> wrote: >> On Sun, Aug 19, 2012 at 10:13 AM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: >>>> Right now we only produce ins when a zero_extract is used on the >>>> right hand side. We can do better by adding some patterns which >>>> combine for the ins instruction. This patch adds those patterns and a >>>> testcase which shows a simple example where the code is improved. >>> >>> Sorry for the delay in reviewing this. Had you thought about trying to >>> teach combine.c about this instead? It doesn't look like any of the >>> patterns are really providing more information about the underlying >>> instruction. >> >> combine.c has some code to do this already if one of the src register >> is the same as the dest register; that is what make_field_assignment >> does. Quickly looking at the code, the problem I doing it in >> make_field_assignment is there is no way to return that you need a >> copy of the value first unless I am missing something obvious. Now I >> agree we should be optimize this in combine rather than these manual >> patterns. > > I now have a patch which implements this in combine which allows the > backend not need to change. I generate a SEQUENCE which then > try_combine splits like we do for PARALLEL but keeping it in the > correct order and allowing for the case where we are combing two > instructions into two instructions. > I hope to be able to post it later on Saturday. Just wondering, what's the status of this? Was worried that you might have posted it and I'd missed it. Richard
On Mon, 2012-11-05 at 19:19 +0000, Richard Sandiford wrote: > Hi Andrew, > > Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > > On Fri, Oct 5, 2012 at 8:43 PM, Andrew Pinski > > <andrew.pinski@caviumnetworks.com> wrote: > >> On Sun, Aug 19, 2012 at 10:13 AM, Richard Sandiford > >> <rdsandiford@googlemail.com> wrote: > >>> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > >>>> Right now we only produce ins when a zero_extract is used on the > >>>> right hand side. We can do better by adding some patterns which > >>>> combine for the ins instruction. This patch adds those patterns and a > >>>> testcase which shows a simple example where the code is improved. > >>> > >>> Sorry for the delay in reviewing this. Had you thought about trying to > >>> teach combine.c about this instead? It doesn't look like any of the > >>> patterns are really providing more information about the underlying > >>> instruction. > >> > >> combine.c has some code to do this already if one of the src register > >> is the same as the dest register; that is what make_field_assignment > >> does. Quickly looking at the code, the problem I doing it in > >> make_field_assignment is there is no way to return that you need a > >> copy of the value first unless I am missing something obvious. Now I > >> agree we should be optimize this in combine rather than these manual > >> patterns. > > > > I now have a patch which implements this in combine which allows the > > backend not need to change. I generate a SEQUENCE which then > > try_combine splits like we do for PARALLEL but keeping it in the > > correct order and allowing for the case where we are combing two > > instructions into two instructions. > > I hope to be able to post it later on Saturday. > > Just wondering, what's the status of this? Was worried that you might > have posted it and I'd missed it. I have not posted it yet. I am still cleaning up the code and making sure it does not cause regressions. Thanks, Andrew > > Richard >
On Mon, 2012-11-05 at 11:20 -0800, Andrew Pinski wrote: > On Mon, 2012-11-05 at 19:19 +0000, Richard Sandiford wrote: > > Hi Andrew, > > > > Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > > > On Fri, Oct 5, 2012 at 8:43 PM, Andrew Pinski > > > <andrew.pinski@caviumnetworks.com> wrote: > > >> On Sun, Aug 19, 2012 at 10:13 AM, Richard Sandiford > > >> <rdsandiford@googlemail.com> wrote: > > >>> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > > >>>> Right now we only produce ins when a zero_extract is used on the > > >>>> right hand side. We can do better by adding some patterns which > > >>>> combine for the ins instruction. This patch adds those patterns and a > > >>>> testcase which shows a simple example where the code is improved. > > >>> > > >>> Sorry for the delay in reviewing this. Had you thought about trying to > > >>> teach combine.c about this instead? It doesn't look like any of the > > >>> patterns are really providing more information about the underlying > > >>> instruction. > > >> > > >> combine.c has some code to do this already if one of the src register > > >> is the same as the dest register; that is what make_field_assignment > > >> does. Quickly looking at the code, the problem I doing it in > > >> make_field_assignment is there is no way to return that you need a > > >> copy of the value first unless I am missing something obvious. Now I > > >> agree we should be optimize this in combine rather than these manual > > >> patterns. > > > > > > I now have a patch which implements this in combine which allows the > > > backend not need to change. I generate a SEQUENCE which then > > > try_combine splits like we do for PARALLEL but keeping it in the > > > correct order and allowing for the case where we are combing two > > > instructions into two instructions. > > > I hope to be able to post it later on Saturday. > > > > Just wondering, what's the status of this? Was worried that you might > > have posted it and I'd missed it. > > I have not posted it yet. I am still cleaning up the code and making > sure it does not cause regressions. Hi Richard, Yesterday, I found out my approach is not ready for the rest of combine. So I have to go back to the drawing board of writing a patch. I will commit the testcase which shows the regression as the testsuite did not have a testcase for the issue before. Thanks, Andrew Pinski > > Thanks, > Andrew > > > > > Richard > > > > >
Index: testsuite/gcc.target/mips/ins-4.c =================================================================== --- testsuite/gcc.target/mips/ins-4.c (revision 0) +++ testsuite/gcc.target/mips/ins-4.c (revision 0) @@ -0,0 +1,22 @@ +/* { dg-options "-O2 isa_rev>=2 -mgp64" } */ +/* { dg-final { scan-assembler-times "ins\t" 2 } } */ +/* { dg-final { scan-assembler-not "or\t" } } */ +/* { dg-final { scan-assembler-not "cins\t" } } */ + +#define shift 0 +#define mask (0xfffffull<<shift) + +/* Check that simple ins are produced by manually doing + bitfield insertations (no shifts in this case). */ + +NOMIPS16 int f(int a, int b) +{ + a = (a&~mask) | ((b<<shift)&mask); + return a; +} + +NOMIPS16 long long fll(long long a, long long b) +{ + a = (a&~mask) | ((b<<shift)&mask); + return a; +} Index: config/mips/predicates.md =================================================================== --- config/mips/predicates.md (revision 190403) +++ config/mips/predicates.md (working copy) @@ -105,6 +105,22 @@ (define_predicate "low_bitmask_operand" (match_code "const_int") (match_test "low_bitmask_len (mode, INTVAL (op)) > 16"))) +(define_predicate "bitmask_operand" + (and (match_code "const_int") + (and (not (match_operand 0 "uns_arith_operand")) + (match_test "mips_bitmask_p (INTVAL (op))")))) + +(define_predicate "bottom_bitmask_operand" + (and (match_code "const_int") + (and (match_operand 0 "bitmask_operand") + (match_test "mips_bottom_bitmask_p (INTVAL (op))")))) + +(define_predicate "inverse_bitmask_operand" + (and (match_code "const_int") + (and (not (match_operand 0 "uns_arith_operand")) + (and (not (match_operand 0 "bottom_bitmask_operand")) + (match_test "mips_bitmask_p (~ INTVAL (op))"))))) + (define_predicate "and_reg_operand" (ior (match_operand 0 "register_operand") (and (not (match_test "TARGET_MIPS16")) Index: config/mips/mips.md =================================================================== --- config/mips/mips.md (revision 190403) +++ config/mips/mips.md (working copy) @@ -3903,6 +3903,112 @@ (define_insn "*cins<mode>" [(set_attr "type" "shift") (set_attr "mode" "<MODE>")]) +(define_insn "*insv<mode>_internal1" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") + (match_operand:GPR 2 "const_int_operand" "i")) + (and:GPR (match_operand:GPR 3 "register_operand" "d") + (match_operand:GPR 4 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS && mips_bottom_bitmask_p (INTVAL (operands[4])) + && INTVAL(operands[2]) == ~INTVAL(operands[4])" +{ + int len, pos; + pos = mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); + operands[2] = GEN_INT (pos); + operands[4] = GEN_INT (len); + return "<d>ins\t%0,%3,%2,%4"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + +(define_insn "*insv<mode>_internal2" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "d") + (match_operand:GPR 2 "const_int_operand" "i")) + (and:GPR (match_operand:GPR 3 "register_operand" "0") + (match_operand:GPR 4 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS && mips_bottom_bitmask_p (INTVAL (operands[2])) + && INTVAL(operands[2]) == ~INTVAL(operands[4])" +{ + int len, pos; + pos = mips_bitmask (INTVAL (operands[2]), &len, <MODE>mode); + operands[2] = GEN_INT (pos); + operands[4] = GEN_INT (len); + return "<d>ins\t%0,%1,%2,%4"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + +(define_insn "*insv<mode>_internal3" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") + (match_operand:GPR 2 "const_int_operand" "i")) + (and:GPR (ashift:GPR (match_operand:GPR 3 "register_operand" "d") + (match_operand:GPR 5 "const_int_operand" "i")) + (match_operand:GPR 4 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS + && mips_bitmask (INTVAL (operands[4]), NULL, VOIDmode) == INTVAL (operands[5]) + && INTVAL(operands[2]) == ~INTVAL(operands[4])" +{ + int len; + mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); + operands[4] = GEN_INT (len); + return "<d>ins\t%0,%3,%5,%4"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + +(define_insn "*insv<mode>_internal4" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (and:GPR (ashift:GPR (match_operand:GPR 3 "register_operand" "d") + (match_operand:GPR 5 "const_int_operand" "i")) + (match_operand:GPR 4 "const_int_operand" "i")) + (and:GPR (match_operand:GPR 1 "register_operand" "0") + (match_operand:GPR 2 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS + && mips_bitmask (INTVAL (operands[4]), NULL, VOIDmode) == INTVAL (operands[5]) + && INTVAL(operands[2]) == ~INTVAL(operands[4])" +{ + int len; + mips_bitmask (INTVAL (operands[4]), &len, <MODE>mode); + operands[4] = GEN_INT (len); + return "<d>ins\t%0,%3,%5,%4"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + +(define_insn "*insv<mode>_internal5" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (and:GPR (match_operand:GPR 1 "register_operand" "0") + (match_operand:GPR 2 "const_int_operand" "i")) + (ashift:GPR (match_operand:GPR 3 "register_operand" "d") + (match_operand:GPR 4 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS && mips_bitmask_ins_p (INTVAL (operands[2]), INTVAL (operands[4]), <MODE>mode)" +{ + int len; + mips_bitmask (~INTVAL (operands[2]), &len, <MODE>mode); + operands[2] = GEN_INT (len); + return "<d>ins\t%0,%3,%4,%2"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + +(define_insn "*insv<mode>_internal4" + [(set (match_operand:GPR 0 "register_operand" "=d") + (ior:GPR (ashift:GPR (match_operand:GPR 3 "register_operand" "d") + (match_operand:GPR 4 "const_int_operand" "i")) + (and:GPR (match_operand:GPR 1 "register_operand" "0") + (match_operand:GPR 2 "const_int_operand" "i"))))] + "ISA_HAS_EXT_INS && mips_bitmask_ins_p (INTVAL (operands[2]), INTVAL (operands[4]), <MODE>mode)" +{ + int len; + mips_bitmask (~INTVAL (operands[2]), &len, <MODE>mode); + operands[2] = GEN_INT (len); + return "<d>ins\t%0,%3,%4,%2"; +} + [(set_attr "type" "arith") + (set_attr "mode" "<MODE>")]) + ;; Unaligned word moves generated by the bit field patterns. ;; ;; As far as the rtl is concerned, both the left-part and right-part Index: config/mips/mips-protos.h =================================================================== --- config/mips/mips-protos.h (revision 190403) +++ config/mips/mips-protos.h (working copy) @@ -310,6 +310,12 @@ extern bool mips16e_save_restore_pattern extern bool mask_low_and_shift_p (enum machine_mode, rtx, rtx, int); extern int mask_low_and_shift_len (enum machine_mode, rtx, rtx); + +extern int mips_bitmask (unsigned HOST_WIDE_INT, int *, enum machine_mode); +extern bool mips_bitmask_p (unsigned HOST_WIDE_INT); +extern bool mips_bitmask_ins_p (unsigned HOST_WIDE_INT, int, enum machine_mode); +extern bool mips_bottom_bitmask_p (unsigned HOST_WIDE_INT); + extern bool and_operands_ok (enum machine_mode, rtx, rtx); union mips_gen_fn_ptrs Index: config/mips/mips.c =================================================================== --- config/mips/mips.c (revision 190403) +++ config/mips/mips.c (working copy) @@ -7414,6 +7414,69 @@ mask_low_and_shift_len (enum machine_mod shval = INTVAL (shift) & (GET_MODE_BITSIZE (mode) - 1); return exact_log2 ((UINTVAL (mask) >> shval) + 1); } + +/* If X is a bitmask (consists of one consecutive block of ones) + return the position of the bottom bit and set *LEN to the length of + the bitmask. Otherwise return -1. MODE is the operation this + constant is used with. If VOIDmode LEN is not used an can be + passed as NULL. */ + +int +mips_bitmask (unsigned HOST_WIDE_INT x, int *len, enum machine_mode mode) +{ + int top, bottom; + + top = floor_log2 (x); + if (top == HOST_BITS_PER_WIDE_INT - 1) + x = -x; + else + x = ((unsigned HOST_WIDE_INT) 1 << (top + 1)) - x; + + bottom = exact_log2 (x); + if (mode == VOIDmode || bottom == -1) + return bottom; + + if (mode == SImode && top > 31) + { + if (top == 63) + top = 31; + else + gcc_unreachable (); + } + + *len = top - bottom + 1; + return bottom; +} + +/* True if X is a bitmask (consists of one consecutive block of + ones). */ + +bool +mips_bitmask_p (unsigned HOST_WIDE_INT x) +{ + return ISA_HAS_EXT_INS && mips_bitmask (x, NULL, VOIDmode) != -1; +} + +/* True if X is a bitmask in MODE and the starting of the bitmask is at + 0 and the length is the same as POS. */ +bool +mips_bitmask_ins_p (unsigned HOST_WIDE_INT x, int pos, enum machine_mode mode) +{ + int len, position; + if (!ISA_HAS_EXT_INS) + return 0; + position = mips_bitmask (x, &len, mode); + return position == 0 && len == pos; +} + +/* True if X is a bitmask (consists of one consecutive block of + ones) and only the bottom bits are set. */ + +bool +mips_bottom_bitmask_p (unsigned HOST_WIDE_INT x) +{ + return ISA_HAS_EXT_INS && mips_bitmask (x, NULL, VOIDmode) == 0; +} /* Return true if -msplit-addresses is selected and should be honored.
Hi, Right now we only produce ins when a zero_extract is used on the right hand side. We can do better by adding some patterns which combine for the ins instruction. This patch adds those patterns and a testcase which shows a simple example where the code is improved. OK? Bootstrapped and tested on mips64-linux-gnu with no regressions. Thanks, Andrew Pinski ChangeLog: Andrew Pinski <apinski@cavium.com> Adam Nemet <anemet@caviumnetworks.com> * config/mips/mips-protos.h (mips_bitmask, mips_bitmask_p, mips_bottom_bitmask_p): Declare them. * config/mips/mips.c (mips_bitmask, mips_bitmask_p, mips_bottom_bitmask_p): New functions. * config/mips/mips.md (*insv<mode>_internal1): New pattern to match a bottom insert. (*insv<mode>_internal2): Likewise. (*insv<mode>_internal3): New pattern to match an insert. (*insv<mode>_internal4): Likewise. * config/mips/predicates.md (bitmask_operand bottom_bitmask_operand, inverse_bitmask_operand): New predicates. * testsuite/gcc.target/mips/ins-4.c: New testcase.