| Message ID | CAHz1=dWh6nP9kDj8SnSRqz970z+ikV=5f3Yb5syL6h00im_FrA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Revital Eres <revital.eres@linaro.org> writes: > +/* Return true if one of the definitions in INSN has MODE_CC. Otherwise > + return false. */ > +static bool > +def_has_ccmode_p (rtx insn) > +{ > + df_ref *def; > + > + for (def = DF_INSN_DEFS (insn); *def; def++) > + { > + enum machine_mode mode = GET_MODE (DF_REF_REG (*def)); > + > + if (GET_MODE_CLASS (mode) == MODE_CC) > + return true; > + } > + > + return false; > +} FWIW, an alternative might be to test have_regs_of_mode[(int) mode]. That says whether there are any allocatable (non-fixed) registers of the given mode. Richard
Hello, > FWIW, an alternative might be to test have_regs_of_mode[(int) mode]. > That says whether there are any allocatable (non-fixed) registers > of the given mode. Thanks, I'll prepare a new version of the patch using have_regs_of_mode. Revital > > Richard
On 12/20/2011 09:47 AM, Richard Sandiford wrote: > Revital Eres <revital.eres@linaro.org> writes: >> +/* Return true if one of the definitions in INSN has MODE_CC. Otherwise >> + return false. */ >> +static bool >> +def_has_ccmode_p (rtx insn) >> +{ >> + df_ref *def; >> + >> + for (def = DF_INSN_DEFS (insn); *def; def++) >> + { >> + enum machine_mode mode = GET_MODE (DF_REF_REG (*def)); >> + >> + if (GET_MODE_CLASS (mode) == MODE_CC) >> + return true; >> + } >> + >> + return false; >> +} > > FWIW, an alternative might be to test have_regs_of_mode[(int) mode]. > That says whether there are any allocatable (non-fixed) registers > of the given mode. While true, I doubt either PPC or MIPS really benefit from moving around registers of CCmode. Certainly MIPS has no way of easily moving CCmode registers around. It's a rather complicated reload, that. I'd be very tempted to simply go with the original patch. r~
Richard Henderson <rth@redhat.com> writes: > On 12/20/2011 09:47 AM, Richard Sandiford wrote: >> Revital Eres <revital.eres@linaro.org> writes: >>> +/* Return true if one of the definitions in INSN has MODE_CC. Otherwise >>> + return false. */ >>> +static bool >>> +def_has_ccmode_p (rtx insn) >>> +{ >>> + df_ref *def; >>> + >>> + for (def = DF_INSN_DEFS (insn); *def; def++) >>> + { >>> + enum machine_mode mode = GET_MODE (DF_REF_REG (*def)); >>> + >>> + if (GET_MODE_CLASS (mode) == MODE_CC) >>> + return true; >>> + } >>> + >>> + return false; >>> +} >> >> FWIW, an alternative might be to test have_regs_of_mode[(int) mode]. >> That says whether there are any allocatable (non-fixed) registers >> of the given mode. > > While true, I doubt either PPC or MIPS really benefit from moving > around registers of CCmode. Certainly MIPS has no way of easily > moving CCmode registers around. It's a rather complicated reload, > that. > > I'd be very tempted to simply go with the original patch. OK, sorry for the noise. Richard
>The attached patch prevents the creation of reg-moves for definitions >with MODE_CC and thus solves this ICE. > >Currently testing and bootstrap on ppc64-redhat-linux, enabling SMS on >loops with SC 1. > >OK for 4.7 once testing completes? Yes, thanks for catching this. Shouldn't we prevent creating such regmoves for (the other case of) intra-loop anti-deps as well? >> While true, I doubt either PPC or MIPS really benefit from moving >> around registers of CCmode. IIRC, PPC has eight 4-bit CR's and supports efficient copying of *one bit* from one CR to another (via cror), which would require special regmove handling. Hence I share your doubt. Ayal. On Thu, Dec 22, 2011 at 10:53 AM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > Richard Henderson <rth@redhat.com> writes: >> On 12/20/2011 09:47 AM, Richard Sandiford wrote: >>> Revital Eres <revital.eres@linaro.org> writes: >>>> +/* Return true if one of the definitions in INSN has MODE_CC. Otherwise >>>> + return false. */ >>>> +static bool >>>> +def_has_ccmode_p (rtx insn) >>>> +{ >>>> + df_ref *def; >>>> + >>>> + for (def = DF_INSN_DEFS (insn); *def; def++) >>>> + { >>>> + enum machine_mode mode = GET_MODE (DF_REF_REG (*def)); >>>> + >>>> + if (GET_MODE_CLASS (mode) == MODE_CC) >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>> >>> FWIW, an alternative might be to test have_regs_of_mode[(int) mode]. >>> That says whether there are any allocatable (non-fixed) registers >>> of the given mode. >> >> While true, I doubt either PPC or MIPS really benefit from moving >> around registers of CCmode. Certainly MIPS has no way of easily >> moving CCmode registers around. It's a rather complicated reload, >> that. >> >> I'd be very tempted to simply go with the original patch. > > OK, sorry for the noise. > > Richard
Index: ddg.c =================================================================== --- ddg.c (revision 182482) +++ ddg.c (working copy) @@ -263,6 +263,23 @@ create_ddg_dep_no_link (ddg_ptr g, ddg_n add_edge_to_ddg (g, e); } +/* Return true if one of the definitions in INSN has MODE_CC. Otherwise + return false. */ +static bool +def_has_ccmode_p (rtx insn) +{ + df_ref *def; + + for (def = DF_INSN_DEFS (insn); *def; def++) + { + enum machine_mode mode = GET_MODE (DF_REF_REG (*def)); + + if (GET_MODE_CLASS (mode) == MODE_CC) + return true; + } + + return false; +} /* Given a downwards exposed register def LAST_DEF (which is the last definition of that register in the bb), add inter-loop true dependences @@ -335,7 +352,8 @@ add_cross_iteration_register_deps (ddg_p if (DF_REF_ID (last_def) != DF_REF_ID (first_def) || !flag_modulo_sched_allow_regmoves || JUMP_P (use_node->insn) - || autoinc_var_is_used_p (DF_REF_INSN (last_def), use_insn)) + || autoinc_var_is_used_p (DF_REF_INSN (last_def), use_insn) + || def_has_ccmode_p (DF_REF_INSN (last_def))) create_ddg_dep_no_link (g, use_node, first_def_node, ANTI_DEP, REG_DEP, 1); Index: testsuite/gcc.dg/sms-11.c =================================================================== --- testsuite/gcc.dg/sms-11.c (revision 0) +++ testsuite/gcc.dg/sms-11.c (revision 0) @@ -0,0 +1,37 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms" } */ + +extern void abort (void); + +float out[4][4] = { 6, 6, 7, 5, 6, 7, 5, 5, 6, 4, 4, 4, 6, 2, 3, 4 }; + +void +invert (void) +{ + int i, j, k = 0, swap; + float tmp[4][4] = { 5, 6, 7, 5, 6, 7, 5, 5, 4, 4, 4, 4, 3, 2, 3, 4 }; + + for (i = 0; i < 4; i++) + { + for (j = i + 1; j < 4; j++) + if (tmp[j][i] > tmp[i][i]) + swap = j; + + if (swap != i) + tmp[i][k] = tmp[swap][k]; + } + + for (i = 0; i < 4; i++) + for (j = 0; j < 4; j++) + if (tmp[i][j] != out[i][j]) + abort (); +} + +int +main () +{ + invert (); + return 0; +} + +/* { dg-final { cleanup-rtl-dump "sms" } } */
Hello, The testcase attached causes ICE when compiling with -fmodulo-sched-allow-regmoves on ARM due to reg-moves created for the definition of mode MODE_CC. The following is a snippet from the ddg of the definition and use of vfpcc which triggers the creation of the reg-move: Node num: 1 (insn 151 77 152 6 (set (reg:CCFP 127 vfpcc) (compare:CCFP (reg:SF 202 [ MEM[base: D.5306_32, offset: 0B] ]) (reg:SF 183 [ D.5284 ]))) test_new.c:8 694 {*cmpsf_vfp} (expr_list:REG_DEAD (reg:SF 202 [ MEM[base: D.5306_32, offset: 0B] ]) (nil))) OUT ARCS: [151 -(T,4,0)-> 152] IN ARCS: [77 -(T,3,0)-> 151] Node num: 2 (insn 152 151 120 6 (set (reg:CCFP 24 cc) (reg:CCFP 127 vfpcc)) test_new.c:8 689 {*movcc_vfp} (expr_list:REG_DEAD (reg:CCFP 127 vfpcc) (nil))) OUT ARCS: [152 -(O,0,0)-> 144] [152 -(T,0,0)-> 120] IN ARCS: [145 -(A,0,1)-> 152] [151 -(T,4,0)-> 152] The attached patch prevents the creation of reg-moves for definitions with MODE_CC and thus solves this ICE. Currently testing and bootstrap on ppc64-redhat-linux, enabling SMS on loops with SC 1. OK for 4.7 once testing completes? Thanks, Revital Changelog: gcc/ * ddg.c (def_has_ccmode_p): New function. (add_cross_iteration_register_deps): Call it. testsuite/ * gcc.dg/sms-11.c: New file.