Message ID | 20170116112317.GF32333@bubble.grove.modra.org |
---|---|
State | New |
Headers | show |
On Mon, Jan 16, 2017 at 09:53:17PM +1030, Alan Modra wrote: > Commited as obvious. > > PR target/79098 > * config/rs6000/rs6000.c (rs6000_legitimate_combined_insn): Don't > use a switch. Perhaps it would be useful to write why it can't be written as a switch. Or, as a switch it could be of the form: switch (INSN_CODE (insn)) { #ifdef HAVE_ctrsi_internal1 case CODE_FOR_ctrsi_internal1: case CODE_FOR_ctrsi_internal2: case CODE_FOR_ctrsi_internal3: case CODE_FOR_ctrsi_internal4: #endif #ifdef HAVE_ctrdi_internal1 case CODE_FOR_ctrdi_internal1: case CODE_FOR_ctrdi_internal2: case CODE_FOR_ctrdi_internal3: case CODE_FOR_ctrdi_internal4: #endif return false; default: break; } > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 11394b2..f1d5d9d 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -9085,40 +9085,41 @@ rs6000_const_not_ok_for_debug_p (rtx x) > static bool > rs6000_legitimate_combined_insn (rtx_insn *insn) > { > - switch (INSN_CODE (insn)) > - { > - /* Reject creating doloop insns. Combine should not be allowed > - to create these for a number of reasons: > - 1) In a nested loop, if combine creates one of these in an > - outer loop and the register allocator happens to allocate ctr > - to the outer loop insn, then the inner loop can't use ctr. > - Inner loops ought to be more highly optimized. > - 2) Combine often wants to create one of these from what was > - originally a three insn sequence, first combining the three > - insns to two, then to ctrsi/ctrdi. When ctrsi/ctrdi is not > - allocated ctr, the splitter takes use back to the three insn > - sequence. It's better to stop combine at the two insn > - sequence. > - 3) Faced with not being able to allocate ctr for ctrsi/crtdi > - insns, the register allocator sometimes uses floating point > - or vector registers for the pseudo. Since ctrsi/ctrdi is a > - jump insn and output reloads are not implemented for jumps, > - the ctrsi/ctrdi splitters need to handle all possible cases. > - That's a pain, and it gets to be seriously difficult when a > - splitter that runs after reload needs memory to transfer from > - a gpr to fpr. See PR70098 and PR71763 which are not fixed > - for the difficult case. It's better to not create problems > - in the first place. */ > - case CODE_FOR_ctrsi_internal1: > - case CODE_FOR_ctrdi_internal1: > - case CODE_FOR_ctrsi_internal2: > - case CODE_FOR_ctrdi_internal2: > - case CODE_FOR_ctrsi_internal3: > - case CODE_FOR_ctrdi_internal3: > - case CODE_FOR_ctrsi_internal4: > - case CODE_FOR_ctrdi_internal4: > - return false; > - } > + int icode = INSN_CODE (insn); > + > + /* Reject creating doloop insns. Combine should not be allowed > + to create these for a number of reasons: > + 1) In a nested loop, if combine creates one of these in an > + outer loop and the register allocator happens to allocate ctr > + to the outer loop insn, then the inner loop can't use ctr. > + Inner loops ought to be more highly optimized. > + 2) Combine often wants to create one of these from what was > + originally a three insn sequence, first combining the three > + insns to two, then to ctrsi/ctrdi. When ctrsi/ctrdi is not > + allocated ctr, the splitter takes use back to the three insn > + sequence. It's better to stop combine at the two insn > + sequence. > + 3) Faced with not being able to allocate ctr for ctrsi/crtdi > + insns, the register allocator sometimes uses floating point > + or vector registers for the pseudo. Since ctrsi/ctrdi is a > + jump insn and output reloads are not implemented for jumps, > + the ctrsi/ctrdi splitters need to handle all possible cases. > + That's a pain, and it gets to be seriously difficult when a > + splitter that runs after reload needs memory to transfer from > + a gpr to fpr. See PR70098 and PR71763 which are not fixed > + for the difficult case. It's better to not create problems > + in the first place. */ > + if (icode != CODE_FOR_nothing > + && (icode == CODE_FOR_ctrsi_internal1 > + || icode == CODE_FOR_ctrdi_internal1 > + || icode == CODE_FOR_ctrsi_internal2 > + || icode == CODE_FOR_ctrdi_internal2 > + || icode == CODE_FOR_ctrsi_internal3 > + || icode == CODE_FOR_ctrdi_internal3 > + || icode == CODE_FOR_ctrsi_internal4 > + || icode == CODE_FOR_ctrdi_internal4)) > + return false; > + > return true; > } > > > -- > Alan Modra > Australia Development Lab, IBM Jakub
On Mon, Jan 16, 2017 at 12:41:29PM +0100, Jakub Jelinek wrote: > Or, as a switch it could be of the form: > switch (INSN_CODE (insn)) > { > #ifdef HAVE_ctrsi_internal1 > case CODE_FOR_ctrsi_internal1: > case CODE_FOR_ctrsi_internal2: > case CODE_FOR_ctrsi_internal3: > case CODE_FOR_ctrsi_internal4: > #endif > #ifdef HAVE_ctrdi_internal1 > case CODE_FOR_ctrdi_internal1: > case CODE_FOR_ctrdi_internal2: > case CODE_FOR_ctrdi_internal3: > case CODE_FOR_ctrdi_internal4: > #endif > return false; > default: > break; > } I didn't think of that. Segher and I discussed the problem on #gcc and both independently decided an if() was the obvious fix.
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 11394b2..f1d5d9d 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -9085,40 +9085,41 @@ rs6000_const_not_ok_for_debug_p (rtx x) static bool rs6000_legitimate_combined_insn (rtx_insn *insn) { - switch (INSN_CODE (insn)) - { - /* Reject creating doloop insns. Combine should not be allowed - to create these for a number of reasons: - 1) In a nested loop, if combine creates one of these in an - outer loop and the register allocator happens to allocate ctr - to the outer loop insn, then the inner loop can't use ctr. - Inner loops ought to be more highly optimized. - 2) Combine often wants to create one of these from what was - originally a three insn sequence, first combining the three - insns to two, then to ctrsi/ctrdi. When ctrsi/ctrdi is not - allocated ctr, the splitter takes use back to the three insn - sequence. It's better to stop combine at the two insn - sequence. - 3) Faced with not being able to allocate ctr for ctrsi/crtdi - insns, the register allocator sometimes uses floating point - or vector registers for the pseudo. Since ctrsi/ctrdi is a - jump insn and output reloads are not implemented for jumps, - the ctrsi/ctrdi splitters need to handle all possible cases. - That's a pain, and it gets to be seriously difficult when a - splitter that runs after reload needs memory to transfer from - a gpr to fpr. See PR70098 and PR71763 which are not fixed - for the difficult case. It's better to not create problems - in the first place. */ - case CODE_FOR_ctrsi_internal1: - case CODE_FOR_ctrdi_internal1: - case CODE_FOR_ctrsi_internal2: - case CODE_FOR_ctrdi_internal2: - case CODE_FOR_ctrsi_internal3: - case CODE_FOR_ctrdi_internal3: - case CODE_FOR_ctrsi_internal4: - case CODE_FOR_ctrdi_internal4: - return false; - } + int icode = INSN_CODE (insn); + + /* Reject creating doloop insns. Combine should not be allowed + to create these for a number of reasons: + 1) In a nested loop, if combine creates one of these in an + outer loop and the register allocator happens to allocate ctr + to the outer loop insn, then the inner loop can't use ctr. + Inner loops ought to be more highly optimized. + 2) Combine often wants to create one of these from what was + originally a three insn sequence, first combining the three + insns to two, then to ctrsi/ctrdi. When ctrsi/ctrdi is not + allocated ctr, the splitter takes use back to the three insn + sequence. It's better to stop combine at the two insn + sequence. + 3) Faced with not being able to allocate ctr for ctrsi/crtdi + insns, the register allocator sometimes uses floating point + or vector registers for the pseudo. Since ctrsi/ctrdi is a + jump insn and output reloads are not implemented for jumps, + the ctrsi/ctrdi splitters need to handle all possible cases. + That's a pain, and it gets to be seriously difficult when a + splitter that runs after reload needs memory to transfer from + a gpr to fpr. See PR70098 and PR71763 which are not fixed + for the difficult case. It's better to not create problems + in the first place. */ + if (icode != CODE_FOR_nothing + && (icode == CODE_FOR_ctrsi_internal1 + || icode == CODE_FOR_ctrdi_internal1 + || icode == CODE_FOR_ctrsi_internal2 + || icode == CODE_FOR_ctrdi_internal2 + || icode == CODE_FOR_ctrsi_internal3 + || icode == CODE_FOR_ctrdi_internal3 + || icode == CODE_FOR_ctrsi_internal4 + || icode == CODE_FOR_ctrdi_internal4)) + return false; + return true; }