| Message ID | 005001cd793a$4e8956e0$eb9c04a0$@cheng@arm.com |
|---|---|
| State | New |
| Headers | show |
> Hi, > For thumb1, arm-gcc rewrites move insn into subtract of ZERO in peephole2 pass > intentionally, then executes > pass_if_after_reload/pass_regrename/pass_cprop_hardreg sequentially. > > In this scenario, copy propagation opportunities are missed because: > 1. the move insns are re-written. > 2. pass_cprop_hardreg currently don't notice the subtract of ZERO. > > This patch fixes the problem and the logic is: > 1. notice the plus/subtract of ZERO in pass_cprop_hardreg. > 2. if the last insn providing information about conditional codes is in the > form of "dest_reg = src_reg - 0", record the src_reg in newly added field > thumb1_cc_op0_src of structure machine_function. > 3. in pattern "cbranchsi4_insn", check thumb1_cc_op0_src along with > thumb1_cc_op0 to save one comparison insn. > > I measured the patch on CSiBE, about 600 bytes are saved for both O2 and Os on > cortex-m0 without any regression. > > I also tested the patch on > arm-none-eabi+cortex-m0/arm-none-eabi+cortex-m3/i686-pc-linux and no > regressions introduced. > > So is it OK? > > Thanks > > 2012-08-13 Bin Cheng <bin.cheng@arm.com> > > * regcprop.c (copyprop_hardreg_forward_1) Notice copies in the form of > subtract of ZERO. > * config/arm/arm.h (thumb1_cc_op0_src) New field. > * config/arm/arm.c (thumb1_final_prescan_insn) Record thumb1_cc_op0_src. > * config/arm/arm.md (cbranchsi4_insn) Check thumb1_cc_op0_src along > with > thumb1_cc_op0. Ping? Hi Ramana, could you help me review this patch? Hi Eric, Richard, could you help me review the change in regcprop.c? Thanks very much
"Bin Cheng" <bin.cheng@arm.com> writes: >> Hi, >> For thumb1, arm-gcc rewrites move insn into subtract of ZERO in peephole2 > pass >> intentionally, then executes >> pass_if_after_reload/pass_regrename/pass_cprop_hardreg sequentially. >> >> In this scenario, copy propagation opportunities are missed because: >> 1. the move insns are re-written. >> 2. pass_cprop_hardreg currently don't notice the subtract of ZERO. >> >> This patch fixes the problem and the logic is: >> 1. notice the plus/subtract of ZERO in pass_cprop_hardreg. >> 2. if the last insn providing information about conditional codes is in > the >> form of "dest_reg = src_reg - 0", record the src_reg in newly added field >> thumb1_cc_op0_src of structure machine_function. >> 3. in pattern "cbranchsi4_insn", check thumb1_cc_op0_src along with >> thumb1_cc_op0 to save one comparison insn. >> >> I measured the patch on CSiBE, about 600 bytes are saved for both O2 and > Os on >> cortex-m0 without any regression. >> >> I also tested the patch on >> arm-none-eabi+cortex-m0/arm-none-eabi+cortex-m3/i686-pc-linux and no >> regressions introduced. >> >> So is it OK? >> >> Thanks >> >> 2012-08-13 Bin Cheng <bin.cheng@arm.com> >> >> * regcprop.c (copyprop_hardreg_forward_1) Notice copies in the form > of >> subtract of ZERO. >> * config/arm/arm.h (thumb1_cc_op0_src) New field. >> * config/arm/arm.c (thumb1_final_prescan_insn) Record > thumb1_cc_op0_src. >> * config/arm/arm.md (cbranchsi4_insn) Check thumb1_cc_op0_src along >> with >> thumb1_cc_op0. > > Ping? > > Hi Ramana, could you help me review this patch? > Hi Eric, Richard, could you help me review the change in regcprop.c? Subtraction of zero isn't canonical rtl though. Passes after peephole2 would be well within their rights to simplify the expression back to a move. From that point of view, making the passes recognise (plus X 0) and (minus X 0) as special cases would be inconsistent. Rather than make the Thumb 1 CC usage implicit in the rtl stream, and carry the current state around in cfun->machine, it seems like it would be better to get md_reorg to rewrite the instructions into a form that makes the use of condition codes explicit. md_reorg also sounds like a better place in the pipeline than peephole2 to be doing this kind of transformation, although I admit I have zero evidence to back that up... Richard
Hi Richard, Thanks very much for comments. > > Ping? > > > > Hi Ramana, could you help me review this patch? > > Hi Eric, Richard, could you help me review the change in regcprop.c? > > Subtraction of zero isn't canonical rtl though. Passes after peephole2 would > be well within their rights to simplify the expression back to a move. > From that point of view, making the passes recognise (plus X 0) and (minus X 0) > as special cases would be inconsistent. > > Rather than make the Thumb 1 CC usage implicit in the rtl stream, and carry > the current state around in cfun->machine, it seems like it would be better to > get md_reorg to rewrite the instructions into a form that makes the use of > condition codes explicit. Here is a problem that two versions MOV instruction are supported in Thumb1 instruction set. The flag-setting version MOV can only take low register operands, while non-flag-setting version MOV can take high register operands. So we cannot rewrite non-flag-setting version MOV(with high register operands) into explicitly flag-setting one, and that's why it is rewritten into subtract of zero instruction now. > > md_reorg also sounds like a better place in the pipeline than peephole2 to be > doing this kind of transformation, although I admit I have zero evidence to > back that up... Yes, it may be feasible to rewrite the instruction in machine reorg pass, rather than peephole2. But that need bigger change in ARM back end. Hi Ramana, Richard, what's your opinion on this? Thanks very much.
On 06/09/12 06:41, Bin Cheng wrote: > Hi Richard, > Thanks very much for comments. > >>> Ping? >>> >>> Hi Ramana, could you help me review this patch? >>> Hi Eric, Richard, could you help me review the change in regcprop.c? >> >> Subtraction of zero isn't canonical rtl though. Passes after peephole2 > would >> be well within their rights to simplify the expression back to a move. >> From that point of view, making the passes recognise (plus X 0) and (minus > X 0) >> as special cases would be inconsistent. >> >> Rather than make the Thumb 1 CC usage implicit in the rtl stream, and > carry >> the current state around in cfun->machine, it seems like it would be > better to >> get md_reorg to rewrite the instructions into a form that makes the use of >> condition codes explicit. > > Here is a problem that two versions MOV instruction are supported in Thumb1 > instruction set. The flag-setting version MOV can only take low register > operands, while non-flag-setting version MOV can take high register > operands. So we cannot rewrite non-flag-setting version MOV(with high > register operands) into explicitly flag-setting one, and that's why it is > rewritten into subtract of zero instruction now. > >> >> md_reorg also sounds like a better place in the pipeline than peephole2 to > be >> doing this kind of transformation, although I admit I have zero evidence > to >> back that up... > > Yes, it may be feasible to rewrite the instruction in machine reorg pass, > rather than peephole2. But that need bigger change in ARM back end. > Hi Ramana, Richard, what's your opinion on this? > > Thanks very much. > > I side with Richard on this one. The mid-end should only have to deal with RTL that's in canonical form. R.
> > > > Yes, it may be feasible to rewrite the instruction in machine reorg > > pass, rather than peephole2. But that need bigger change in ARM back end. > > Hi Ramana, Richard, what's your opinion on this? > > > > Thanks very much. > > > > > > I side with Richard on this one. The mid-end should only have to deal with > RTL that's in canonical form. > So how about rewrite mov insn into sub in machine reorg pass and remove the current peeophole2 codes? Thanks
On 06/09/12 09:33, Bin Cheng wrote: >>> >>> Yes, it may be feasible to rewrite the instruction in machine reorg >>> pass, rather than peephole2. But that need bigger change in ARM back > end. >>> Hi Ramana, Richard, what's your opinion on this? >>> >>> Thanks very much. >>> >>> >> >> I side with Richard on this one. The mid-end should only have to deal > with >> RTL that's in canonical form. >> > So how about rewrite mov insn into sub in machine reorg pass and remove the > current peeophole2 codes? > > Thanks > > I think you should keep a peephole that transforms X := Y cbranch (cmp (Y, 0) tgt) into X := Y cbranch (cmp (X, 0) tgt) since that gives you a dataflow dependency between the two patterns that is easier to track. Once you've got that the mdreorg can track the dataflow and transform the mov into a sub. R.
Ok if no regressions. Ramana On Wed, Oct 10, 2012 at 9:57 AM, Bin Cheng <bin.cheng@arm.com> wrote: > Ping^2 > >> -----Original Message----- >> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] > On >> Behalf Of Bin Cheng >> Sent: Monday, October 08, 2012 2:36 PM >> To: gcc-patches@gcc.gnu.org >> Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford' >> Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that > hardreg_cprop >> opportunities are missed on thumb1 >> >> Ping. >> >> > -----Original Message----- >> > From: gcc-patches-owner@gcc.gnu.org >> > [mailto:gcc-patches-owner@gcc.gnu.org] >> On >> > Behalf Of Bin Cheng >> > Sent: Tuesday, September 25, 2012 4:00 PM >> > To: 'Richard Sandiford' >> > Cc: Ramana Radhakrishnan; Richard Earnshaw; gcc-patches@gcc.gnu.org >> > Subject: RE: [Updated]: [PATCH GCC/ARM] Fix problem that hardreg_cprop >> > opportunities are missed on thumb1 >> > >> > >> > > -----Original Message----- >> > > From: Richard Sandiford [mailto:rdsandiford@googlemail.com] >> > > Sent: Wednesday, September 05, 2012 6:09 AM >> > > To: Bin Cheng >> > > Cc: Ramana Radhakrishnan; 'Eric Botcazou'; gcc-patches@gcc.gnu.org >> > > Subject: Re: Ping: [PATCH GCC/ARM] Fix problem that hardreg_cprop >> > > opportunities are missed on thumb1 >> > >> > > Subtraction of zero isn't canonical rtl though. Passes after >> > > peephole2 >> > would >> > > be well within their rights to simplify the expression back to a move. >> > > From that point of view, making the passes recognise (plus X 0) and >> > > (minus >> > X 0) >> > > as special cases would be inconsistent. >> > > >> > > Rather than make the Thumb 1 CC usage implicit in the rtl stream, >> > > and >> > carry >> > > the current state around in cfun->machine, it seems like it would be >> > better to >> > > get md_reorg to rewrite the instructions into a form that makes the >> > > use of condition codes explicit. >> > > >> > > md_reorg also sounds like a better place in the pipeline than >> > > peephole2 to >> > be >> > > doing this kind of transformation, although I admit I have zero >> > > evidence >> > to >> > > back that up... >> > > >> > >> > Hi Richard, >> > >> > This is the updated patch according to your suggestions. I removed the >> > peephole2 patterns and introduced function thumb1_reorg to rewrite >> > instructions in md_reorg pass. >> > >> > In addition to missed propagation, this patch also detects following > case: >> > mov r5, r0 >> > str r0, [r4] <-------miscellaneous irrelevant instructions >> > [cmp r0, 0] <-------saved >> > bne .Lxxx >> > >> > Patch tested on arm-none-eabi/cortex-m0, no regressions introduced. >> > >> > Is it OK? >> > >> > Thanks. >> > >> > 2012-09-25 Bin Cheng <bin.cheng@arm.com> >> > >> > * config/arm/arm.c (thumb1_reorg): New function. >> > (arm_reorg): Call thumb1_reorg. >> > (thumb1_final_prescan_insn): Record src operand in thumb1_cc_op0. >> > * config/arm/arm.md : Remove peephole2 patterns which rewrites move >> > into subtract of ZERO. >> >> >>
> -----Original Message----- > From: Ramana Radhakrishnan [mailto:ramana.gcc@googlemail.com] > Sent: Saturday, November 24, 2012 7:20 AM > To: Bin Cheng > Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan; Richard Earnshaw; Richard > Sandiford > Subject: Re: [PING Updated]: [PATCH GCC/ARM] Fix problem that hardreg_cprop > opportunities are missed on thumb1 > > Ok if no regressions. > I re-tested it against trunk and committed it as r193841 Thanks. > Ramana > > On Wed, Oct 10, 2012 at 9:57 AM, Bin Cheng <bin.cheng@arm.com> wrote: > > Ping^2 > > > >> -----Original Message----- > >> From: gcc-patches-owner@gcc.gnu.org > >> [mailto:gcc-patches-owner@gcc.gnu.org] > > On > >> Behalf Of Bin Cheng > >> Sent: Monday, October 08, 2012 2:36 PM > >> To: gcc-patches@gcc.gnu.org > >> Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford' > >> Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that > > hardreg_cprop > >> opportunities are missed on thumb1 > >> > >> Ping. > >> > >> > -----Original Message----- > >> > From: gcc-patches-owner@gcc.gnu.org > >> > [mailto:gcc-patches-owner@gcc.gnu.org] > >> On > >> > Behalf Of Bin Cheng > >> > Sent: Tuesday, September 25, 2012 4:00 PM > >> > To: 'Richard Sandiford' > >> > Cc: Ramana Radhakrishnan; Richard Earnshaw; gcc-patches@gcc.gnu.org > >> > Subject: RE: [Updated]: [PATCH GCC/ARM] Fix problem that > >> > hardreg_cprop opportunities are missed on thumb1 > >> > > >> > > >> > > -----Original Message----- > >> > > From: Richard Sandiford [mailto:rdsandiford@googlemail.com] > >> > > Sent: Wednesday, September 05, 2012 6:09 AM > >> > > To: Bin Cheng > >> > > Cc: Ramana Radhakrishnan; 'Eric Botcazou'; > >> > > gcc-patches@gcc.gnu.org > >> > > Subject: Re: Ping: [PATCH GCC/ARM] Fix problem that hardreg_cprop > >> > > opportunities are missed on thumb1 > >> > > >> > > Subtraction of zero isn't canonical rtl though. Passes after > >> > > peephole2 > >> > would > >> > > be well within their rights to simplify the expression back to a move. > >> > > From that point of view, making the passes recognise (plus X 0) > >> > > and (minus > >> > X 0) > >> > > as special cases would be inconsistent. > >> > > > >> > > Rather than make the Thumb 1 CC usage implicit in the rtl stream, > >> > > and > >> > carry > >> > > the current state around in cfun->machine, it seems like it would > >> > > be > >> > better to > >> > > get md_reorg to rewrite the instructions into a form that makes > >> > > the use of condition codes explicit. > >> > > > >> > > md_reorg also sounds like a better place in the pipeline than > >> > > peephole2 to > >> > be > >> > > doing this kind of transformation, although I admit I have zero > >> > > evidence > >> > to > >> > > back that up... > >> > > > >> > > >> > Hi Richard, > >> > > >> > This is the updated patch according to your suggestions. I removed > >> > the > >> > peephole2 patterns and introduced function thumb1_reorg to rewrite > >> > instructions in md_reorg pass. > >> > > >> > In addition to missed propagation, this patch also detects > >> > following > > case: > >> > mov r5, r0 > >> > str r0, [r4] <-------miscellaneous irrelevant instructions > >> > [cmp r0, 0] <-------saved > >> > bne .Lxxx > >> > > >> > Patch tested on arm-none-eabi/cortex-m0, no regressions introduced. > >> > > >> > Is it OK? > >> > > >> > Thanks. > >> > > >> > 2012-09-25 Bin Cheng <bin.cheng@arm.com> > >> > > >> > * config/arm/arm.c (thumb1_reorg): New function. > >> > (arm_reorg): Call thumb1_reorg. > >> > (thumb1_final_prescan_insn): Record src operand in thumb1_cc_op0. > >> > * config/arm/arm.md : Remove peephole2 patterns which rewrites move > >> > into subtract of ZERO. > >> > >> > >>
Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 189835) +++ gcc/config/arm/arm.c (working copy) @@ -21663,6 +21663,8 @@ if (cfun->machine->thumb1_cc_insn) { if (modified_in_p (cfun->machine->thumb1_cc_op0, insn) + || (cfun->machine->thumb1_cc_op0_src != NULL_RTX + && modified_in_p (cfun->machine->thumb1_cc_op0_src, insn)) || modified_in_p (cfun->machine->thumb1_cc_op1, insn)) CC_STATUS_INIT; } @@ -21672,13 +21674,18 @@ rtx set = single_set (insn); cfun->machine->thumb1_cc_insn = insn; cfun->machine->thumb1_cc_op0 = SET_DEST (set); + cfun->machine->thumb1_cc_op0_src = NULL_RTX; cfun->machine->thumb1_cc_op1 = const0_rtx; cfun->machine->thumb1_cc_mode = CC_NOOVmode; if (INSN_CODE (insn) == CODE_FOR_thumb1_subsi3_insn) { rtx src1 = XEXP (SET_SRC (set), 1); if (src1 == const0_rtx) - cfun->machine->thumb1_cc_mode = CCmode; + { + cfun->machine->thumb1_cc_mode = CCmode; + /* Record the minuend in thumb1_subsi3_insn pattern. */ + cfun->machine->thumb1_cc_op0_src = XEXP (SET_SRC (set), 0); + } } } else if (conds != CONDS_NOCOND) Index: gcc/config/arm/arm.h =================================================================== --- gcc/config/arm/arm.h (revision 189835) +++ gcc/config/arm/arm.h (working copy) @@ -1459,9 +1459,17 @@ is not needed. */ int return_used_this_function; /* When outputting Thumb-1 code, record the last insn that provides - information about condition codes, and the comparison operands. */ + information about condition codes, and the comparison operands. + + If the last insn that provides information about condition codes + is in the form of "dest_reg = src_reg - 0", record the src_reg in + thumb1_cc_op0_src, and for following insn sequence: + dest_reg = src_reg - 0; + if (src_reg ?= 0) goto label; + the comparison insn can also be saved. */ rtx thumb1_cc_insn; rtx thumb1_cc_op0; + rtx thumb1_cc_op0_src; rtx thumb1_cc_op1; /* Also record the CC mode that is supported. */ enum machine_mode thumb1_cc_mode; Index: gcc/config/arm/arm.md =================================================================== --- gcc/config/arm/arm.md (revision 189835) +++ gcc/config/arm/arm.md (working copy) @@ -7018,7 +7018,8 @@ rtx t = cfun->machine->thumb1_cc_insn; if (t != NULL_RTX) { - if (!rtx_equal_p (cfun->machine->thumb1_cc_op0, operands[1]) + if ((!rtx_equal_p (cfun->machine->thumb1_cc_op0, operands[1]) + && !rtx_equal_p (cfun->machine->thumb1_cc_op0_src, operands[1])) || !rtx_equal_p (cfun->machine->thumb1_cc_op1, operands[2])) t = NULL_RTX; if (cfun->machine->thumb1_cc_mode == CC_NOOVmode) @@ -7034,6 +7035,7 @@ output_asm_insn ("cmp\t%1, %2", operands); cfun->machine->thumb1_cc_insn = insn; cfun->machine->thumb1_cc_op0 = operands[1]; + cfun->machine->thumb1_cc_op0_src = NULL_RTX; cfun->machine->thumb1_cc_op1 = operands[2]; cfun->machine->thumb1_cc_mode = CCmode; } Index: gcc/regcprop.c =================================================================== --- gcc/regcprop.c (revision 189835) +++ gcc/regcprop.c (working copy) @@ -1019,8 +1019,18 @@ note_stores (PATTERN (insn), kill_set_value, &ksvd); /* Notice copies. */ - if (set && REG_P (SET_DEST (set)) && REG_P (SET_SRC (set))) - copy_value (SET_DEST (set), SET_SRC (set), vd); + if (set && REG_P (SET_DEST (set))) + { + if (REG_P (SET_SRC (set))) + copy_value (SET_DEST (set), SET_SRC (set), vd); + /* ARM backend intentionally rewrite move insn into subtract of 0 if + the condition codes will be useful, so also notice this case. */ + else if ((GET_CODE (SET_SRC (set)) == PLUS + || GET_CODE (SET_SRC (set)) == MINUS) + && REG_P (XEXP (SET_SRC (set), 0)) + && XEXP (SET_SRC (set), 1) == const0_rtx) + copy_value (SET_DEST (set), XEXP (SET_SRC (set), 0), vd); + } if (insn == BB_END (bb)) break;