| Message ID | 87r452ygnm.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Richard Sandiford [mailto:rdsandiford@googlemail.com] > Sent: Saturday, April 12, 2014 6:41 AM > To: Matthew Fortune > Cc: Moore, Catherine; gcc-patches@gcc.gnu.org; rguenther@suse.de; > jakub@redhat.com > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > SB16 > > Richard Sandiford <rdsandiford@googlemail.com> writes: > > Matthew Fortune <Matthew.Fortune@imgtec.com> writes: > >> Hi Catherine/Richard, > >> > >> I think there may be some impact on register move costs by > >> introducing this class. > > > > Yeah, I was worried about that too. I'm going to do some code > > comparison tests for SE and MIPS16 to see what happens. > > OK, I compared the .s testsuite output for -O, -O2, -O3, -Os, "-O -mips16 - > mabi=32" and "-Os -mips16 -mabi=32" on mips64-linux-gnu. > I also tried CSiBE with -O2 and "-Os -mips16 -mabi=32". The code was > identical in all cases so I think we should be OK. > > I went ahead and applied the adjusted version of the patch to trunk as below > (because I wanted to add a testcase too). Thanks for doing this. > > Adding a new register class is definitely a bit invasive for this stage of 4.9. > OTOH microMIPS is a new feature and it would be good to have it working in > 4.9.0. Since the testing suggests that the patch really doesn't affect non- > microMIPS code, I'd like it to go in 4.9 now. > Richard, Jakub, would that be OK? > Okay, understood. Will wait to hear. > > > gcc/ > 2014-04-12 Catherine Moore <clm@codesourcery.com> > > * config/mips/constraints.md: Add new register constraint "kb". > * config/mips/mips.md (*mov<mode>_internal): Use constraint > "kb". > (*movhi_internal): Likewise. > (*movqi_internal): Likewise. > * config/mips/mips.h (M16_STORE_REGS): New register class. > (REG_CLASS_NAMES): Add M16_STORE_REGS. > (REG_CLASS_CONTENTS): Likewise. > * config/mips/mips.c (mips_regno_to_class): Add > M16_STORE_REGS. > > gcc/testsuite/ > * gcc.target/mips/umips-store16-1.c: New test. > > Index: gcc/config/mips/constraints.md > ========================================================== > ========= > --- gcc/config/mips/constraints.md 2014-04-12 10:36:09.105788710 +0100 > +++ gcc/config/mips/constraints.md 2014-04-12 10:38:48.895224932 +0100 > @@ -92,6 +92,9 @@ (define_register_constraint "D" "COP3_RE ;; but the > DSP version allows any accumulator target. > (define_register_constraint "ka" "ISA_HAS_DSP_MULT ? ACC_REGS : > MD_REGS") > > +(define_register_constraint "kb" "M16_STORE_REGS" > + "@internal") > + > (define_constraint "kf" > "@internal" > (match_operand 0 "force_to_mem_operand")) > Index: gcc/config/mips/mips.md > ========================================================== > ========= > --- gcc/config/mips/mips.md 2014-04-12 10:36:09.105788710 +0100 > +++ gcc/config/mips/mips.md 2014-04-12 10:38:48.925225200 +0100 > @@ -4437,7 +4437,7 @@ (define_expand "mov<mode>" > > (define_insn "*mov<mode>_internal" > [(set (match_operand:IMOVE32 0 "nonimmediate_operand" > "=d,!u,!u,d,e,!u,!ks,d,ZS,ZT,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d, > *m") > - (match_operand:IMOVE32 1 "move_operand" > "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C > *D,*B*C*D"))] > + (match_operand:IMOVE32 1 "move_operand" > +"d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!kb,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B > +*C*D,*B*C*D"))] > "!TARGET_MIPS16 > && (register_operand (operands[0], <MODE>mode) > || reg_or_0_operand (operands[1], <MODE>mode))" > @@ -4578,7 +4578,7 @@ (define_expand "movhi" > > (define_insn "*movhi_internal" > [(set (match_operand:HI 0 "nonimmediate_operand" > "=d,!u,d,!u,d,ZU,m,*a,*d") > - (match_operand:HI 1 "move_operand" > "d,J,I,ZU,m,!u,dJ,*d*J,*a"))] > + (match_operand:HI 1 "move_operand" > "d,J,I,ZU,m,!kb,dJ,*d*J,*a"))] > "!TARGET_MIPS16 > && (register_operand (operands[0], HImode) > || reg_or_0_operand (operands[1], HImode))" > @@ -4654,7 +4654,7 @@ (define_expand "movqi" > > (define_insn "*movqi_internal" > [(set (match_operand:QI 0 "nonimmediate_operand" > "=d,!u,d,!u,d,ZV,m,*a,*d") > - (match_operand:QI 1 "move_operand" > "d,J,I,ZW,m,!u,dJ,*d*J,*a"))] > + (match_operand:QI 1 "move_operand" > "d,J,I,ZW,m,!kb,dJ,*d*J,*a"))] > "!TARGET_MIPS16 > && (register_operand (operands[0], QImode) > || reg_or_0_operand (operands[1], QImode))" > Index: gcc/config/mips/mips.h > ========================================================== > ========= > --- gcc/config/mips/mips.h 2014-04-12 10:36:09.105788710 +0100 > +++ gcc/config/mips/mips.h 2014-04-12 10:38:48.924225191 +0100 > @@ -1870,6 +1870,7 @@ #define PIC_OFFSET_TABLE_REGNUM \ enum > reg_class { > NO_REGS, /* no registers in set */ > + M16_STORE_REGS, /* microMIPS store registers */ > M16_REGS, /* mips16 directly accessible registers */ > T_REG, /* mips16 T register ($24) */ > M16_T_REGS, /* mips16 registers plus T register */ > @@ -1907,6 +1908,7 @@ #define GENERAL_REGS GR_REGS > #define REG_CLASS_NAMES > \ > { \ > "NO_REGS", \ > + "M16_STORE_REGS", > \ > "M16_REGS", \ > "T_REG", \ > "M16_T_REGS", > \ > @@ -1947,6 +1949,7 @@ #define REG_CLASS_NAMES > \ > #define REG_CLASS_CONTENTS > \ > { > \ > { 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, > 0x00000000 }, /* NO_REGS */ \ > + { 0x000200fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, > 0x00000000 }, /* M16_STORE_REGS */ \ > { 0x000300fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 > }, /* M16_REGS */ \ > { 0x01000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, > 0x00000000 }, /* T_REG */ \ > { 0x010300fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 > }, /* M16_T_REGS */ \ > Index: gcc/config/mips/mips.c > ========================================================== > ========= > --- gcc/config/mips/mips.c 2014-04-12 10:36:09.105788710 +0100 > +++ gcc/config/mips/mips.c 2014-04-12 10:38:48.923225182 +0100 > @@ -648,14 +648,15 @@ struct target_globals *mips16_globals; > > /* Index R is the smallest register class that contains register R. */ const > enum reg_class mips_regno_to_class[FIRST_PSEUDO_REGISTER] = { > - LEA_REGS, LEA_REGS, M16_REGS, V1_REG, > - M16_REGS, M16_REGS, M16_REGS, M16_REGS, > - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > - M16_REGS, M16_REGS, LEA_REGS, LEA_REGS, > - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > - T_REG, PIC_FN_ADDR_REG, LEA_REGS, LEA_REGS, > - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > + LEA_REGS, LEA_REGS, M16_STORE_REGS, V1_REG, > + M16_STORE_REGS, M16_STORE_REGS, M16_STORE_REGS, > M16_STORE_REGS, > + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > + M16_REGS, M16_STORE_REGS, LEA_REGS, LEA_REGS, > + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > + T_REG, PIC_FN_ADDR_REG, LEA_REGS, LEA_REGS, > + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, > + > FP_REGS, FP_REGS, FP_REGS, FP_REGS, > FP_REGS, FP_REGS, FP_REGS, FP_REGS, > FP_REGS, FP_REGS, FP_REGS, FP_REGS, > Index: gcc/testsuite/gcc.target/mips/umips-store16-1.c > ========================================================== > ========= > --- /dev/null 2014-04-11 11:50:02.034263237 +0100 > +++ gcc/testsuite/gcc.target/mips/umips-store16-1.c 2014-04-12 > 10:38:48.926225209 +0100 > @@ -0,0 +1,30 @@ > +/* { dg-options "(-mmicromips)" } */ > +/* { dg-do assemble } */ > + > +register unsigned int global asm ("$16"); > + > +extern void exit (int) __attribute__((noreturn)); > + > +MICROMIPS void > +test_sb (unsigned char *ptr, void (*f) (void)) { > + ptr[0] = global; > + f (); > + exit (0); > +} > + > +MICROMIPS void > +test_sh (unsigned short *ptr, void (*f) (void)) { > + ptr[0] = global; > + f (); > + exit (0); > +} > + > +MICROMIPS void > +test_sw (unsigned int *ptr, void (*f) (void)) { > + ptr[0] = global; > + f (); > + exit (0); > +}
"Moore, Catherine" <Catherine_Moore@mentor.com> writes: >> Adding a new register class is definitely a bit invasive for this >> stage of 4.9. >> OTOH microMIPS is a new feature and it would be good to have it working in >> 4.9.0. Since the testing suggests that the patch really doesn't affect non- >> microMIPS code, I'd like it to go in 4.9 now. >> Richard, Jakub, would that be OK? >> > Okay, understood. Will wait to hear. The decision on IRC was that this is too invasive for 4.9.0 at this stage. It would probably be going in without another release candidate before the release. Thanks, Richard
On Sat, 12 Apr 2014, Richard Sandiford wrote: > I went ahead and applied the adjusted version of the patch to trunk > as below (because I wanted to add a testcase too). I believe you need to adjust constraints to ensure constant 0 is known to produce a 16-bit instruction encoding where possible. Otherwise you'll end up with suboptimal code when the instruction is in a branch delay slot. E.g. here: > --- gcc/config/mips/mips.md 2014-04-12 10:36:09.105788710 +0100 > +++ gcc/config/mips/mips.md 2014-04-12 10:38:48.925225200 +0100 > @@ -4437,7 +4437,7 @@ (define_expand "mov<mode>" > > (define_insn "*mov<mode>_internal" > [(set (match_operand:IMOVE32 0 "nonimmediate_operand" "=d,!u,!u,d,e,!u,!ks,d,ZS,ZT,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d,*m") > - (match_operand:IMOVE32 1 "move_operand" "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))] > + (match_operand:IMOVE32 1 "move_operand" "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!kb,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))] > "!TARGET_MIPS16 > && (register_operand (operands[0], <MODE>mode) > || reg_or_0_operand (operands[1], <MODE>mode))" using: (match_operand:IMOVE32 1 "move_operand" "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!kbJ,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))] will make: jals foo sw $0, 0($2) be produced (6 bytes) rather than: jal foo sw $0, 0($2) (8 bytes), because the "!kbJ"/"ZT" constraint pair will match rather than more general "dJ"/"m". Likewise with the other two RTL patterns. I'm fairly sure you'll be able to come up with an appropriate test case to cover it (or to prove me wrong if I missed something here). Maciej
"Maciej W. Rozycki" <macro@codesourcery.com> writes: > On Sat, 12 Apr 2014, Richard Sandiford wrote: > >> I went ahead and applied the adjusted version of the patch to trunk >> as below (because I wanted to add a testcase too). > > I believe you need to adjust constraints to ensure constant 0 is known to > produce a 16-bit instruction encoding where possible. Otherwise you'll > end up with suboptimal code when the instruction is in a branch delay > slot. Yeah, it'd be good to do that too (although this is a preexisting problem). I'm relying on you guys to do the microMIPS stuff though -- I don't have a way of testing it. Thanks, Richard
Richard Sandiford <rdsandiford@googlemail.com> writes: > "Maciej W. Rozycki" <macro@codesourcery.com> writes: > > On Sat, 12 Apr 2014, Richard Sandiford wrote: > > > >> I went ahead and applied the adjusted version of the patch to trunk > >> as below (because I wanted to add a testcase too). > > > > I believe you need to adjust constraints to ensure constant 0 is > > known to produce a 16-bit instruction encoding where possible. > > Otherwise you'll end up with suboptimal code when the instruction is > > in a branch delay slot. > > Yeah, it'd be good to do that too (although this is a preexisting > problem). > I'm relying on you guys to do the microMIPS stuff though -- I don't have > a way of testing it. FYI, we have GNUSIM patches awaiting submission to add micromips support. We are waiting on copyright assignment for GDB which is why they are not available yet. We were planning on getting them submitting as 'on behalf of' but it seems this may not be permitted any more by FSF. Matthew
On Tue, 15 Apr 2014, Richard Sandiford wrote: > > I believe you need to adjust constraints to ensure constant 0 is known to > > produce a 16-bit instruction encoding where possible. Otherwise you'll > > end up with suboptimal code when the instruction is in a branch delay > > slot. > > Yeah, it'd be good to do that too (although this is a preexisting problem). Well, it depends on how you look at the problem being solved here -- if it is "for SW16, SH16 and SB16 GCC produces broken code for the `s0' source register", then indeed it is, whereas if it is "GCC does not handle the source register set for SW16, SH16 and SB16 correctly", then it is a part of the same problem, not completely corrected. I can live with that until 4.10/4.9.1 though if you prefer. > I'm relying on you guys to do the microMIPS stuff though -- I don't have > a way of testing it. An assembly/objdump test is enough to cover this, so you've got all tools at hand, although I understand you may not be inclined to rush working on it. ;) Maciej
> -----Original Message----- > From: Maciej W. Rozycki [mailto:macro@codesourcery.com] > Sent: Tuesday, April 15, 2014 7:28 AM > To: Richard Sandiford > Cc: Matthew Fortune; Moore, Catherine; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > SB16 > > On Tue, 15 Apr 2014, Richard Sandiford wrote: > > > > I believe you need to adjust constraints to ensure constant 0 is > > > known to produce a 16-bit instruction encoding where possible. > > > Otherwise you'll end up with suboptimal code when the instruction is > > > in a branch delay slot. > > > > Yeah, it'd be good to do that too (although this is a preexisting problem). > > Well, it depends on how you look at the problem being solved here -- if it is > "for SW16, SH16 and SB16 GCC produces broken code for the `s0' > source register", then indeed it is, whereas if it is "GCC does not handle the > source register set for SW16, SH16 and SB16 correctly", then it is a part of the > same problem, not completely corrected. I can live with that until 4.10/4.9.1 > though if you prefer. > > > I'm relying on you guys to do the microMIPS stuff though -- I don't > > have a way of testing it. > > An assembly/objdump test is enough to cover this, so you've got all tools at > hand, although I understand you may not be inclined to rush working on it. ;) > I'll take care of this bit.
> -----Original Message----- > From: Moore, Catherine > Sent: Tuesday, April 15, 2014 8:49 AM > To: Rozycki, Maciej; Richard Sandiford > Cc: Matthew Fortune; gcc-patches@gcc.gnu.org; Moore, Catherine > Subject: RE: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > SB16 > > > > > -----Original Message----- > > From: Maciej W. Rozycki [mailto:macro@codesourcery.com] > > Sent: Tuesday, April 15, 2014 7:28 AM > > To: Richard Sandiford > > Cc: Matthew Fortune; Moore, Catherine; gcc-patches@gcc.gnu.org > > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > > SB16 > > > > On Tue, 15 Apr 2014, Richard Sandiford wrote: > > > > > > I believe you need to adjust constraints to ensure constant 0 is > > > > known to produce a 16-bit instruction encoding where possible. > > > > Otherwise you'll end up with suboptimal code when the instruction > > > > is in a branch delay slot. > > > > > > Yeah, it'd be good to do that too (although this is a preexisting problem). > > > > Well, it depends on how you look at the problem being solved here -- > > if it is "for SW16, SH16 and SB16 GCC produces broken code for the `s0' > > source register", then indeed it is, whereas if it is "GCC does not > > handle the source register set for SW16, SH16 and SB16 correctly", > > then it is a part of the same problem, not completely corrected. I > > can live with that until 4.10/4.9.1 though if you prefer. > > > > > I'm relying on you guys to do the microMIPS stuff though -- I don't > > > have a way of testing it. > > > > An assembly/objdump test is enough to cover this, so you've got all > > tools at hand, although I understand you may not be inclined to rush > > working on it. ;) > > > I'll take care of this bit. I've attached an updated patch to address Maciej's concern with $0 and the microMIPS store instructions. Does this look okay to install? Thanks, Catherine
"Moore, Catherine" <Catherine_Moore@mentor.com> writes: >> -----Original Message----- >> From: Moore, Catherine >> Sent: Tuesday, April 15, 2014 8:49 AM >> To: Rozycki, Maciej; Richard Sandiford >> Cc: Matthew Fortune; gcc-patches@gcc.gnu.org; Moore, Catherine >> Subject: RE: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and >> SB16 >> >> >> >> > -----Original Message----- >> > From: Maciej W. Rozycki [mailto:macro@codesourcery.com] >> > Sent: Tuesday, April 15, 2014 7:28 AM >> > To: Richard Sandiford >> > Cc: Matthew Fortune; Moore, Catherine; gcc-patches@gcc.gnu.org >> > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and >> > SB16 >> > >> > On Tue, 15 Apr 2014, Richard Sandiford wrote: >> > >> > > > I believe you need to adjust constraints to ensure constant 0 is >> > > > known to produce a 16-bit instruction encoding where possible. >> > > > Otherwise you'll end up with suboptimal code when the instruction >> > > > is in a branch delay slot. >> > > >> > > Yeah, it'd be good to do that too (although this is a preexisting >> > > problem). >> > >> > Well, it depends on how you look at the problem being solved here -- >> > if it is "for SW16, SH16 and SB16 GCC produces broken code for the `s0' >> > source register", then indeed it is, whereas if it is "GCC does not >> > handle the source register set for SW16, SH16 and SB16 correctly", >> > then it is a part of the same problem, not completely corrected. I >> > can live with that until 4.10/4.9.1 though if you prefer. >> > >> > > I'm relying on you guys to do the microMIPS stuff though -- I don't >> > > have a way of testing it. >> > >> > An assembly/objdump test is enough to cover this, so you've got all >> > tools at hand, although I understand you may not be inclined to rush >> > working on it. ;) >> > >> I'll take care of this bit. > > I've attached an updated patch to address Maciej's concern with $0 and > the microMIPS store instructions. > Does this look okay to install? No, the point was that zero is modelled as a constant in RTL, so like Maciej says, the way to handle it is to use the "J" constraint (like some of the existing contraints use "dJ" for "any GPR or zero"). What we want to test is that: *ptr = 0; is a 16-bit instruction. You could do that by adding "-dp" to the options and matching something like: MICROMIPS void f1 (unsigned char *ptr) { *ptr = 0; } ...[similarly for short and int]... /* { dg-final { scan-assembler "\tsb\t\\\$0, 0\\(\\\$4\\)\[^\n\]length = 2" } } */ ...[similarly for sh and sw]... Completely untested. I bet the regexp needs different backslashes. :-) Thanks, Richard
> -----Original Message----- > From: Richard Sandiford [mailto:rdsandiford@googlemail.com] > Sent: Tuesday, April 15, 2014 4:32 PM > To: Moore, Catherine > Cc: Rozycki, Maciej; Matthew Fortune; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > SB16 > > "Moore, Catherine" <Catherine_Moore@mentor.com> writes: > >> -----Original Message----- > >> From: Moore, Catherine > >> Sent: Tuesday, April 15, 2014 8:49 AM > >> To: Rozycki, Maciej; Richard Sandiford > >> Cc: Matthew Fortune; gcc-patches@gcc.gnu.org; Moore, Catherine > >> Subject: RE: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and > >> SB16 > >> > >> > >> > >> > -----Original Message----- > >> > From: Maciej W. Rozycki [mailto:macro@codesourcery.com] > >> > Sent: Tuesday, April 15, 2014 7:28 AM > >> > To: Richard Sandiford > >> > Cc: Matthew Fortune; Moore, Catherine; gcc-patches@gcc.gnu.org > >> > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 > >> > and > >> > SB16 > >> > > >> > On Tue, 15 Apr 2014, Richard Sandiford wrote: > >> > > >> > > > I believe you need to adjust constraints to ensure constant 0 > >> > > > is known to produce a 16-bit instruction encoding where possible. > >> > > > Otherwise you'll end up with suboptimal code when the > >> > > > instruction is in a branch delay slot. > >> > > > >> > > Yeah, it'd be good to do that too (although this is a preexisting > >> > > problem). > >> > > >> > Well, it depends on how you look at the problem being solved here > >> > -- if it is "for SW16, SH16 and SB16 GCC produces broken code for the `s0' > >> > source register", then indeed it is, whereas if it is "GCC does not > >> > handle the source register set for SW16, SH16 and SB16 correctly", > >> > then it is a part of the same problem, not completely corrected. I > >> > can live with that until 4.10/4.9.1 though if you prefer. > >> > > >> > > I'm relying on you guys to do the microMIPS stuff though -- I > >> > > don't have a way of testing it. > >> > > >> > An assembly/objdump test is enough to cover this, so you've got > >> > all tools at hand, although I understand you may not be inclined to > >> > rush working on it. ;) > >> > > >> I'll take care of this bit. > > > > I've attached an updated patch to address Maciej's concern with $0 and > > the microMIPS store instructions. > > Does this look okay to install? > > No, the point was that zero is modelled as a constant in RTL, so like Maciej > says, the way to handle it is to use the "J" constraint (like some of the > existing contraints use "dJ" for "any GPR or zero"). > > What we want to test is that: > > *ptr = 0; > > is a 16-bit instruction. You could do that by adding "-dp" to the options and > matching something like: > > MICROMIPS void > f1 (unsigned char *ptr) > { > *ptr = 0; > } > > ...[similarly for short and int]... > > /* { dg-final { scan-assembler "\tsb\t\\\$0, 0\\(\\\$4\\)\[^\n\]length = 2" } } > */ ...[similarly for sh and sw]... > > Completely untested. I bet the regexp needs different backslashes. :-) > Okay, this patch modifies the constraints instead. Okay?
"Moore, Catherine" <Catherine_Moore@mentor.com> writes: >> -----Original Message----- >> From: Richard Sandiford [mailto:rdsandiford@googlemail.com] >> Sent: Tuesday, April 15, 2014 4:32 PM >> To: Moore, Catherine >> Cc: Rozycki, Maciej; Matthew Fortune; gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and >> SB16 >> >> "Moore, Catherine" <Catherine_Moore@mentor.com> writes: >> >> -----Original Message----- >> >> From: Moore, Catherine >> >> Sent: Tuesday, April 15, 2014 8:49 AM >> >> To: Rozycki, Maciej; Richard Sandiford >> >> Cc: Matthew Fortune; gcc-patches@gcc.gnu.org; Moore, Catherine >> >> Subject: RE: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 and >> >> SB16 >> >> >> >> >> >> >> >> > -----Original Message----- >> >> > From: Maciej W. Rozycki [mailto:macro@codesourcery.com] >> >> > Sent: Tuesday, April 15, 2014 7:28 AM >> >> > To: Richard Sandiford >> >> > Cc: Matthew Fortune; Moore, Catherine; gcc-patches@gcc.gnu.org >> >> > Subject: Re: [PATCH] [MIPS] Fix operands for microMIPS SW16, SH16 >> >> > and >> >> > SB16 >> >> > >> >> > On Tue, 15 Apr 2014, Richard Sandiford wrote: >> >> > >> >> > > > I believe you need to adjust constraints to ensure constant 0 >> >> > > > is known to produce a 16-bit instruction encoding where possible. >> >> > > > Otherwise you'll end up with suboptimal code when the >> >> > > > instruction is in a branch delay slot. >> >> > > >> >> > > Yeah, it'd be good to do that too (although this is a preexisting >> >> > > problem). >> >> > >> >> > Well, it depends on how you look at the problem being solved here >> >> > -- if it is "for SW16, SH16 and SB16 GCC produces broken code for >> >> > the `s0' >> >> > source register", then indeed it is, whereas if it is "GCC does not >> >> > handle the source register set for SW16, SH16 and SB16 correctly", >> >> > then it is a part of the same problem, not completely corrected. I >> >> > can live with that until 4.10/4.9.1 though if you prefer. >> >> > >> >> > > I'm relying on you guys to do the microMIPS stuff though -- I >> >> > > don't have a way of testing it. >> >> > >> >> > An assembly/objdump test is enough to cover this, so you've got >> >> > all tools at hand, although I understand you may not be inclined to >> >> > rush working on it. ;) >> >> > >> >> I'll take care of this bit. >> > >> > I've attached an updated patch to address Maciej's concern with $0 and >> > the microMIPS store instructions. >> > Does this look okay to install? >> >> No, the point was that zero is modelled as a constant in RTL, so like Maciej >> says, the way to handle it is to use the "J" constraint (like some of the >> existing contraints use "dJ" for "any GPR or zero"). >> >> What we want to test is that: >> >> *ptr = 0; >> >> is a 16-bit instruction. You could do that by adding "-dp" to the options and >> matching something like: >> >> MICROMIPS void >> f1 (unsigned char *ptr) >> { >> *ptr = 0; >> } >> >> ...[similarly for short and int]... >> >> /* { dg-final { scan-assembler "\tsb\t\\\$0, 0\\(\\\$4\\)\[^\n\]length >> = 2" } } Oops, I see I forgot the "*", should have: \[^\n\]*length. But: >> */ ...[similarly for sh and sw]... >> >> Completely untested. I bet the regexp needs different backslashes. :-) >> > Okay, this patch modifies the constraints instead. Okay? > > > > Index: testsuite/gcc.target/mips/umips-store16-2.c > =================================================================== > --- testsuite/gcc.target/mips/umips-store16-2.c (revision 0) > +++ testsuite/gcc.target/mips/umips-store16-2.c (revision 0) > @@ -0,0 +1,22 @@ > +/* { dg-options "(-mmicromips) -dp" } */ > + > +MICROMIPS void > +f1 (unsigned char *ptr) > +{ > + *ptr = 0; > +} > + > +MICROMIPS void > +f2 (unsigned short *ptr) > +{ > + *ptr = 0; > +} > + > +MICROMIPS void > +f3 (unsigned int *ptr) > +{ > + *ptr = 0; > +} > +/* { dg-final { scan-assembler "\tsb\t\\\$0,0\\(\\\$\[0-9\]+\\).*length = 2" } } */ ...it does need to be \[^\n\], since "." can match newlines in Tcl. OK with that change if the new tests still pass, and if a full test run passes with -mmicromips. Thanks, Richard
Index: gcc/config/mips/constraints.md =================================================================== --- gcc/config/mips/constraints.md 2014-04-12 10:36:09.105788710 +0100 +++ gcc/config/mips/constraints.md 2014-04-12 10:38:48.895224932 +0100 @@ -92,6 +92,9 @@ (define_register_constraint "D" "COP3_RE ;; but the DSP version allows any accumulator target. (define_register_constraint "ka" "ISA_HAS_DSP_MULT ? ACC_REGS : MD_REGS") +(define_register_constraint "kb" "M16_STORE_REGS" + "@internal") + (define_constraint "kf" "@internal" (match_operand 0 "force_to_mem_operand")) Index: gcc/config/mips/mips.md =================================================================== --- gcc/config/mips/mips.md 2014-04-12 10:36:09.105788710 +0100 +++ gcc/config/mips/mips.md 2014-04-12 10:38:48.925225200 +0100 @@ -4437,7 +4437,7 @@ (define_expand "mov<mode>" (define_insn "*mov<mode>_internal" [(set (match_operand:IMOVE32 0 "nonimmediate_operand" "=d,!u,!u,d,e,!u,!ks,d,ZS,ZT,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d,*m") - (match_operand:IMOVE32 1 "move_operand" "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))] + (match_operand:IMOVE32 1 "move_operand" "d,J,Udb7,Yd,Yf,ZT,ZS,m,!ks,!kb,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))] "!TARGET_MIPS16 && (register_operand (operands[0], <MODE>mode) || reg_or_0_operand (operands[1], <MODE>mode))" @@ -4578,7 +4578,7 @@ (define_expand "movhi" (define_insn "*movhi_internal" [(set (match_operand:HI 0 "nonimmediate_operand" "=d,!u,d,!u,d,ZU,m,*a,*d") - (match_operand:HI 1 "move_operand" "d,J,I,ZU,m,!u,dJ,*d*J,*a"))] + (match_operand:HI 1 "move_operand" "d,J,I,ZU,m,!kb,dJ,*d*J,*a"))] "!TARGET_MIPS16 && (register_operand (operands[0], HImode) || reg_or_0_operand (operands[1], HImode))" @@ -4654,7 +4654,7 @@ (define_expand "movqi" (define_insn "*movqi_internal" [(set (match_operand:QI 0 "nonimmediate_operand" "=d,!u,d,!u,d,ZV,m,*a,*d") - (match_operand:QI 1 "move_operand" "d,J,I,ZW,m,!u,dJ,*d*J,*a"))] + (match_operand:QI 1 "move_operand" "d,J,I,ZW,m,!kb,dJ,*d*J,*a"))] "!TARGET_MIPS16 && (register_operand (operands[0], QImode) || reg_or_0_operand (operands[1], QImode))" Index: gcc/config/mips/mips.h =================================================================== --- gcc/config/mips/mips.h 2014-04-12 10:36:09.105788710 +0100 +++ gcc/config/mips/mips.h 2014-04-12 10:38:48.924225191 +0100 @@ -1870,6 +1870,7 @@ #define PIC_OFFSET_TABLE_REGNUM \ enum reg_class { NO_REGS, /* no registers in set */ + M16_STORE_REGS, /* microMIPS store registers */ M16_REGS, /* mips16 directly accessible registers */ T_REG, /* mips16 T register ($24) */ M16_T_REGS, /* mips16 registers plus T register */ @@ -1907,6 +1908,7 @@ #define GENERAL_REGS GR_REGS #define REG_CLASS_NAMES \ { \ "NO_REGS", \ + "M16_STORE_REGS", \ "M16_REGS", \ "T_REG", \ "M16_T_REGS", \ @@ -1947,6 +1949,7 @@ #define REG_CLASS_NAMES \ #define REG_CLASS_CONTENTS \ { \ { 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 }, /* NO_REGS */ \ + { 0x000200fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 }, /* M16_STORE_REGS */ \ { 0x000300fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 }, /* M16_REGS */ \ { 0x01000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 }, /* T_REG */ \ { 0x010300fc, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 }, /* M16_T_REGS */ \ Index: gcc/config/mips/mips.c =================================================================== --- gcc/config/mips/mips.c 2014-04-12 10:36:09.105788710 +0100 +++ gcc/config/mips/mips.c 2014-04-12 10:38:48.923225182 +0100 @@ -648,14 +648,15 @@ struct target_globals *mips16_globals; /* Index R is the smallest register class that contains register R. */ const enum reg_class mips_regno_to_class[FIRST_PSEUDO_REGISTER] = { - LEA_REGS, LEA_REGS, M16_REGS, V1_REG, - M16_REGS, M16_REGS, M16_REGS, M16_REGS, - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, - M16_REGS, M16_REGS, LEA_REGS, LEA_REGS, - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, - T_REG, PIC_FN_ADDR_REG, LEA_REGS, LEA_REGS, - LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, + LEA_REGS, LEA_REGS, M16_STORE_REGS, V1_REG, + M16_STORE_REGS, M16_STORE_REGS, M16_STORE_REGS, M16_STORE_REGS, + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, + M16_REGS, M16_STORE_REGS, LEA_REGS, LEA_REGS, + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, + T_REG, PIC_FN_ADDR_REG, LEA_REGS, LEA_REGS, + LEA_REGS, LEA_REGS, LEA_REGS, LEA_REGS, + FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, FP_REGS, Index: gcc/testsuite/gcc.target/mips/umips-store16-1.c =================================================================== --- /dev/null 2014-04-11 11:50:02.034263237 +0100 +++ gcc/testsuite/gcc.target/mips/umips-store16-1.c 2014-04-12 10:38:48.926225209 +0100 @@ -0,0 +1,30 @@ +/* { dg-options "(-mmicromips)" } */ +/* { dg-do assemble } */ + +register unsigned int global asm ("$16"); + +extern void exit (int) __attribute__((noreturn)); + +MICROMIPS void +test_sb (unsigned char *ptr, void (*f) (void)) +{ + ptr[0] = global; + f (); + exit (0); +} + +MICROMIPS void +test_sh (unsigned short *ptr, void (*f) (void)) +{ + ptr[0] = global; + f (); + exit (0); +} + +MICROMIPS void +test_sw (unsigned int *ptr, void (*f) (void)) +{ + ptr[0] = global; + f (); + exit (0); +}