| Message ID | AANLkTinA9B9PuMdEvoNG4h7wnRd9egzqBNuUChFMJ+S1@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
ping On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote: > In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same > operation. This patch simply detect the situation in pattern "andsi3" and call > gen_thumb2_zero_extendqisi2_v6. > > Tested on arm qemu. > > ChangeLog: > 2010-07-22 Wei Guozhi <carrot@google.com> > > PR target/44999 > * config/arm/arm.md (andsi3): Change to zero extension if possible. > * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. > > ChangeLog: > 2010-07-22 Wei Guozhi <carrot@google.com> > > PR target/44999 > * gcc.target/arm/pr44999.c: New testcase. > > > Index: thumb2.md > =================================================================== > --- thumb2.md (revision 162396) > +++ thumb2.md (working copy) > @@ -970,7 +970,7 @@ > (set_attr "neg_pool_range" "*,250")] > ) > > -(define_insn "*thumb2_zero_extendqisi2_v6" > +(define_insn "thumb2_zero_extendqisi2_v6" > [(set (match_operand:SI 0 "s_register_operand" "=r,r") > (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))] > "TARGET_THUMB2 && arm_arch6" > > > Index: arm.md > =================================================================== > --- arm.md (revision 162396) > +++ arm.md (working copy) > @@ -1933,9 +1933,16 @@ > { > if (GET_CODE (operands[2]) == CONST_INT) > { > - arm_split_constant (AND, SImode, NULL_RTX, > - INTVAL (operands[2]), operands[0], > - operands[1], optimize && can_create_pseudo_p ()); > + if (INTVAL (operands[2]) == 255 && arm_arch6) > + { > + operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0); > + gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]); > + } > + else > + arm_split_constant (AND, SImode, NULL_RTX, > + INTVAL (operands[2]), operands[0], > + operands[1], > + optimize && can_create_pseudo_p ()); > > DONE; > } > > > Index: pr44999.c > =================================================================== > --- pr44999.c (revision 0) > +++ pr44999.c (revision 0) > @@ -0,0 +1,7 @@ > +/* { dg-options "-march=armv7-a -mthumb -Os" } */ > +/* { dg-final { scan-assembler "uxtb" } } */ > + > +int tp(int x, int y) > +{ > + return (x & 0xff) - (y & 0xffff); > +} >
On 07/26/2010 08:39 AM, Carrot Wei wrote:
> ping
After four days? People are often on holiday in July.
The patch may be a reasonable approach, however I'd also check whether
combine attempts this change, and if so why does it fail.
Paolo
I'm not an expert of combine, and I find the following comments in combine.c: We try to combine each pair of insns joined by a logical link. We also try to combine triples of insns A, B and C when C has a link back to B and B has a link back to A. Only one insn is involved in the change, so combine should not check it. thanks Guozhi On Mon, Jul 26, 2010 at 3:23 PM, Paolo Bonzini <bonzini@gnu.org> wrote: > On 07/26/2010 08:39 AM, Carrot Wei wrote: >> >> ping > > After four days? People are often on holiday in July. > > The patch may be a reasonable approach, however I'd also check whether > combine attempts this change, and if so why does it fail. > > Paolo >
On 07/26/2010 11:08 AM, Carrot Wei wrote: > I'm not an expert of combine, and I find the following comments in combine.c: > > We try to combine each pair of insns joined by a logical link. > We also try to combine triples of insns A, B and C when > C has a link back to B and B has a link back to A. > > Only one insn is involved in the change, so combine should not check it. Right, of course. I check the i386 backend and it is also doing the same optimization as your patch does, i.e. without the help of the middle-end. (It actually does it using a constraint instead of RTL, but that doesn't really matter and would be a bit harder to do for ARM). Paolo
ARM maintainers, is this OK? thanks Guozhi On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote: > In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same > operation. This patch simply detect the situation in pattern "andsi3" and call > gen_thumb2_zero_extendqisi2_v6. > > Tested on arm qemu. > > ChangeLog: > 2010-07-22 Wei Guozhi <carrot@google.com> > > PR target/44999 > * config/arm/arm.md (andsi3): Change to zero extension if possible. > * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. > > ChangeLog: > 2010-07-22 Wei Guozhi <carrot@google.com> > > PR target/44999 > * gcc.target/arm/pr44999.c: New testcase. > > > Index: thumb2.md > =================================================================== > --- thumb2.md (revision 162396) > +++ thumb2.md (working copy) > @@ -970,7 +970,7 @@ > (set_attr "neg_pool_range" "*,250")] > ) > > -(define_insn "*thumb2_zero_extendqisi2_v6" > +(define_insn "thumb2_zero_extendqisi2_v6" > [(set (match_operand:SI 0 "s_register_operand" "=r,r") > (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))] > "TARGET_THUMB2 && arm_arch6" > > > Index: arm.md > =================================================================== > --- arm.md (revision 162396) > +++ arm.md (working copy) > @@ -1933,9 +1933,16 @@ > { > if (GET_CODE (operands[2]) == CONST_INT) > { > - arm_split_constant (AND, SImode, NULL_RTX, > - INTVAL (operands[2]), operands[0], > - operands[1], optimize && can_create_pseudo_p ()); > + if (INTVAL (operands[2]) == 255 && arm_arch6) > + { > + operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0); > + gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]); > + } > + else > + arm_split_constant (AND, SImode, NULL_RTX, > + INTVAL (operands[2]), operands[0], > + operands[1], > + optimize && can_create_pseudo_p ()); > > DONE; > } > > > Index: pr44999.c > =================================================================== > --- pr44999.c (revision 0) > +++ pr44999.c (revision 0) > @@ -0,0 +1,7 @@ > +/* { dg-options "-march=armv7-a -mthumb -Os" } */ > +/* { dg-final { scan-assembler "uxtb" } } */ > + > +int tp(int x, int y) > +{ > + return (x & 0xff) - (y & 0xffff); > +} >
ping On Fri, Jul 30, 2010 at 2:31 PM, Carrot Wei <carrot@google.com> wrote: > ARM maintainers, is this OK? > > thanks > Guozhi > > On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote: >> In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same >> operation. This patch simply detect the situation in pattern "andsi3" and call >> gen_thumb2_zero_extendqisi2_v6. >> >> Tested on arm qemu. >> >> ChangeLog: >> 2010-07-22 Wei Guozhi <carrot@google.com> >> >> PR target/44999 >> * config/arm/arm.md (andsi3): Change to zero extension if possible. >> * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. >> >> ChangeLog: >> 2010-07-22 Wei Guozhi <carrot@google.com> >> >> PR target/44999 >> * gcc.target/arm/pr44999.c: New testcase. >> >> >> Index: thumb2.md >> =================================================================== >> --- thumb2.md (revision 162396) >> +++ thumb2.md (working copy) >> @@ -970,7 +970,7 @@ >> (set_attr "neg_pool_range" "*,250")] >> ) >> >> -(define_insn "*thumb2_zero_extendqisi2_v6" >> +(define_insn "thumb2_zero_extendqisi2_v6" >> [(set (match_operand:SI 0 "s_register_operand" "=r,r") >> (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))] >> "TARGET_THUMB2 && arm_arch6" >> >> >> Index: arm.md >> =================================================================== >> --- arm.md (revision 162396) >> +++ arm.md (working copy) >> @@ -1933,9 +1933,16 @@ >> { >> if (GET_CODE (operands[2]) == CONST_INT) >> { >> - arm_split_constant (AND, SImode, NULL_RTX, >> - INTVAL (operands[2]), operands[0], >> - operands[1], optimize && can_create_pseudo_p ()); >> + if (INTVAL (operands[2]) == 255 && arm_arch6) >> + { >> + operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0); >> + gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]); >> + } >> + else >> + arm_split_constant (AND, SImode, NULL_RTX, >> + INTVAL (operands[2]), operands[0], >> + operands[1], >> + optimize && can_create_pseudo_p ()); >> >> DONE; >> } >> >> >> Index: pr44999.c >> =================================================================== >> --- pr44999.c (revision 0) >> +++ pr44999.c (revision 0) >> @@ -0,0 +1,7 @@ >> +/* { dg-options "-march=armv7-a -mthumb -Os" } */ >> +/* { dg-final { scan-assembler "uxtb" } } */ >> + >> +int tp(int x, int y) >> +{ >> + return (x & 0xff) - (y & 0xffff); >> +} >> >
On Thu, 2010-07-22 at 15:11 +0800, Carrot Wei wrote: > In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same > operation. This patch simply detect the situation in pattern "andsi3" and call > gen_thumb2_zero_extendqisi2_v6. > Index: arm.md > =================================================================== > --- arm.md (revision 162396) > +++ arm.md (working copy) > @@ -1933,9 +1933,16 @@ > { > if (GET_CODE (operands[2]) == CONST_INT) > { > - arm_split_constant (AND, SImode, NULL_RTX, > - INTVAL (operands[2]), operands[0], > - operands[1], optimize && can_create_pseudo_p ()); > + if (INTVAL (operands[2]) == 255 && arm_arch6) > + { > + operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0); No, never generate SUBREGs directly like this (you get the wrong byte for a big-endian core). Instead, you should use convert_to_mode(). >+/* { dg-options "-march=armv7-a -mthumb -Os" } */ >+/* { dg-final { scan-assembler "uxtb" } } */ This is also wrong and will break if people are testing other CPU permutations. See thumb2-cmpneg2add-1.c for an example of how to do this correctly. R.
Hi Carrot, >> ARM maintainers, is this OK? >>> ChangeLog: >>> 2010-07-22 Wei Guozhi<carrot@google.com> >>> >>> PR target/44999 >>> * config/arm/arm.md (andsi3): Change to zero extension if possible. >>> * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. >>> >>> ChangeLog: >>> 2010-07-22 Wei Guozhi<carrot@google.com> >>> >>> PR target/44999 >>> * gcc.target/arm/pr44999.c: New testcase. Approved - please apply. Cheers Nick
On Tue, 2010-08-10 at 10:08 +0100, Nick Clifton wrote: > Hi Carrot, > > >> ARM maintainers, is this OK? > > >>> ChangeLog: > >>> 2010-07-22 Wei Guozhi<carrot@google.com> > >>> > >>> PR target/44999 > >>> * config/arm/arm.md (andsi3): Change to zero extension if possible. > >>> * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. > >>> > >>> ChangeLog: > >>> 2010-07-22 Wei Guozhi<carrot@google.com> > >>> > >>> PR target/44999 > >>> * gcc.target/arm/pr44999.c: New testcase. > > Approved - please apply. Sorry, no it's not OK. See my earlier review. R.
Index: thumb2.md =================================================================== --- thumb2.md (revision 162396) +++ thumb2.md (working copy) @@ -970,7 +970,7 @@ (set_attr "neg_pool_range" "*,250")] ) -(define_insn "*thumb2_zero_extendqisi2_v6" +(define_insn "thumb2_zero_extendqisi2_v6" [(set (match_operand:SI 0 "s_register_operand" "=r,r") (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))] "TARGET_THUMB2 && arm_arch6" Index: arm.md =================================================================== --- arm.md (revision 162396) +++ arm.md (working copy) @@ -1933,9 +1933,16 @@ { if (GET_CODE (operands[2]) == CONST_INT) { - arm_split_constant (AND, SImode, NULL_RTX, - INTVAL (operands[2]), operands[0], - operands[1], optimize && can_create_pseudo_p ()); + if (INTVAL (operands[2]) == 255 && arm_arch6) + { + operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0); + gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]); + } + else + arm_split_constant (AND, SImode, NULL_RTX, + INTVAL (operands[2]), operands[0], + operands[1], + optimize && can_create_pseudo_p ()); DONE; } Index: pr44999.c =================================================================== --- pr44999.c (revision 0) +++ pr44999.c (revision 0) @@ -0,0 +1,7 @@ +/* { dg-options "-march=armv7-a -mthumb -Os" } */ +/* { dg-final { scan-assembler "uxtb" } } */ + +int tp(int x, int y) +{ + return (x & 0xff) - (y & 0xffff); +}
In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same operation. This patch simply detect the situation in pattern "andsi3" and call gen_thumb2_zero_extendqisi2_v6. Tested on arm qemu. ChangeLog: 2010-07-22 Wei Guozhi <carrot@google.com> PR target/44999 * config/arm/arm.md (andsi3): Change to zero extension if possible. * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name. ChangeLog: 2010-07-22 Wei Guozhi <carrot@google.com> PR target/44999 * gcc.target/arm/pr44999.c: New testcase.