Following fortran test-case fails with cross compiler: $ ppc64-linux-gnu-gfortran /home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/streamio_11.f90 -m32 -mcpu=405 -mpower9-minmax -mfloat128-type internal compiler error: Segmentation fault 0xbdc9ef crash_signal .././../gcc/toplev.c:337 0x90c99a aggregate_value_p(tree_node const*, tree_node const*) .././../gcc/function.c:2028 0x78aedf emit_library_call_value_1 .././../gcc/calls.c:4455 0x79156f emit_library_call_value(rtx_def*, rtx_def*, libcall_type, machine_mode, int, ...) .././../gcc/calls.c:5159 0x8b1a90 convert_move(rtx_def*, rtx_def*, int) .././../gcc/expr.c:314 0x8b208d convert_modes(machine_mode, machine_mode, rtx_def*, int) .././../gcc/expr.c:685 0xae4dc2 prepare_operand(insn_code, rtx_def*, int, machine_mode, machine_mode, int) .././../gcc/optabs.c:3963 0xae4fda prepare_cmp_insn .././../gcc/optabs.c:3873 0xae5a35 emit_cmp_and_jump_insns(rtx_def*, rtx_def*, rtx_code, rtx_def*, machine_mode, int, rtx_def*, int) .././../gcc/optabs.c:4051 0x81aa5c do_compare_rtx_and_jump(rtx_def*, rtx_def*, rtx_code, int, machine_mode, rtx_def*, rtx_code_label*, rtx_code_label*, int) .././../gcc/dojump.c:1145 0x81b9d9 do_compare_and_jump .././../gcc/dojump.c:1224 0x81d69b do_jump_1(tree_code, tree_node*, tree_node*, rtx_code_label*, rtx_code_label*, int) .././../gcc/dojump.c:253 0x7a3554 expand_gimple_cond .././../gcc/cfgexpand.c:2479 0x7a3554 expand_gimple_basic_block .././../gcc/cfgexpand.c:5612 0x7a7d46 execute .././../gcc/cfgexpand.c:6357
-mpower9-minmax enables VSX support, and we end up with carnage (during expand already). No big surprise when targeting 405.
I'm seeking consensus on the "right thing to do". Should I make sure that -mpower9-minmax turns on whatever additional target options are necessary in order to make this work?
Author: kelvin Date: Wed Mar 29 17:23:58 2017 New Revision: 246572 URL: https://gcc.gnu.org/viewcvs?rev=246572&root=gcc&view=rev Log: Use this branch for work on PR 80108. Added: branches/ibm/bz80108/ - copied from r246571, trunk/
(In reply to kelvin from comment #2) > I'm seeking consensus on the "right thing to do". Should I make sure that > -mpower9-minmax turns on whatever additional target options are necessary in > order to make this work? Use of -mcpu that is not power9 or higher together with a power9-specific option is just wrong. We should reject this combination of options with an error message, in my view.
On Wed, Mar 29, 2017 at 05:14:38PM +0000, kelvin at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80108 > > kelvin at gcc dot gnu.org changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > CC| |kelvin at gcc dot gnu.org > > --- Comment #2 from kelvin at gcc dot gnu.org --- > I'm seeking consensus on the "right thing to do". Should I make sure that > -mpower9-minmax turns on whatever additional target options are necessary in > order to make this work? My opinion is if -mcpu=<xxx> is not set, then -mpower9-minmax should set -mcpu=power9. However, -mno-power9-minmax should not set anything. If -mcpu=<xxx> is not -mcpu=power9, then this is an eror.
For stage 4, fixing this particular error combination (along with what Mike suggests) should be enough. There is a vast array of ridiculous option combinations that should no doubt be rejected, but let's not go there for now...
I'll pursue the recommendations offered by Michael and Bill. Aside: as I read the existing implementation, I believe the more "consistent" behavior would be to behave as suggested in my original "proposal". I don't feel strongly about this, and I may have misunderstood some of the existing code. Here is my current understanding of how this behaves in general: 1. If someone specifies a particular CPU option (e.g. -mcpu=405), this has the effect of setting a bunch of target options "implicitly". 2. If someone specifies a particular target option explicitly (-mpower9-minmax), this has the effect of setting a bunch of additional target options on which this option depends, and marking all of these dependent target options as explicit, UNLESS any of the dependent target options have been "explicitly" disabled, in which case we issue an error message and quit. Feel free to correct my understanding if I'm still confused about this. If we feel that -mcpu=405 -mpower9-minmax should be an error, there are probably a bunch of other option combinations that we should probably treat as errors as well, whereas today we may be "fixing up the inconsistencies" silently. In terms of "fixing this mess", I've been thinking the "right way" to handle all the target options would be to "auto-generate" rs6000_option_override_internal from tables that describe in a more manageable way the lattice of dependencies between all of the different target options. That would be the best way to enforce consistent handling of everything. But it would require some "effort" and would probably break existing behavior... :(
Hi Kelvin, 405 does not have VSX (or even VMX). The instructions enabled by -mpower9-minmax require VSX. The following behaviours all make sense, for -mcpu=405 -mpower9-minmax: 1) Ignore the latter option; 2) Give a warning; 3) Give an error. The following do not make sense: 1) Enable VSX; 2) Select -mcpu=power9 instead; 3) Omelette du fromage.
I've got a patch now that resolves this problem without creating regressions. I'm attaching it for "discussion" here before I post for "official review". Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 246573) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -4273,8 +4273,30 @@ rs6000_option_override_internal (bool global_init_ /* For the newer switches (vsx, dfp, etc.) set some of the older options, unless the user explicitly used the -mno-<option> to disable the code. */ if (TARGET_P9_VECTOR || TARGET_MODULO || TARGET_P9_DFORM_SCALAR - || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0 || TARGET_P9_MINMAX) + || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0) rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); + else if (TARGET_P9_MINMAX) + { + if (have_cpu) + { + if (cpu_index == PROCESSOR_POWER9) + /* legacy behavior: allow -mcpu-power9 with certain capabilities + (eg -mno-vsx) explicitly disabled. */ + rs6000_isa_flags |= + (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); + else + error ("Power9 target option is incompatible with -mcpu=<xxx> for " + "<xxx> less than power9"); + } + else if ((ISA_3_0_MASKS_SERVER & rs6000_isa_flags_explicit) + != (ISA_3_0_MASKS_SERVER & rs6000_isa_flags + & rs6000_isa_flags_explicit)) + /* Enforce that none of the ISA_3_0_MASKS_SERVER flags + were explicitly cleared. */ + error ("-mpower9-minmax incompatible with explicitly disabled options"); + else + rs6000_isa_flags |= ISA_3_0_MASKS_SERVER; + } else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO) rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~rs6000_isa_flags_explicit); else if (TARGET_VSX) I'm seeking feedback on whether this is the treatment you are all comfortable with. Note that the way I handle TARGET_P9_MINMAX is very different than the way we handle other p9-specific target options (see the first line replaced by this patch). An earlier draft of my patch handled all six of these options the same way that my patch handles TARGET_P9_MINMAX, but that resulted in 942 fewer "expected passes", 4 more "unexpected failures", and 309 more "unsupported tests" in the gcc summary. It is these inconsistencies that make me a little uncomfortable with how this solution is structured. I welcome feedback and suggestions for further refinement.
FWIW, I tried another variant on the patch, which is shown below. This variant handles all of the p9-specific target options the same, as seen below: Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 246573) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -4273,8 +4273,32 @@ rs6000_option_override_internal (bool global_init_ /* For the newer switches (vsx, dfp, etc.) set some of the older options, unless the user explicitly used the -mno-<option> to disable the code. */ if (TARGET_P9_VECTOR || TARGET_MODULO || TARGET_P9_DFORM_SCALAR - || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0 || TARGET_P9_MINMAX) - rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); + || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0 + || TARGET_P9_MINMAX) + { + if (have_cpu) + { + if (cpu_index == PROCESSOR_POWER9) + /* legacy behavior: allow -mcpu-power9 with certain capabilities + (eg -mno-vsx) explicitly disabled. */ + rs6000_isa_flags |= + (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); + else + error ("Power9 target option is incompatible with -mcpu=<xxx> for " + "<xxx> less than power9"); + } +#ifdef KELVIN_REMOVE + else if ((ISA_3_0_MASKS_SERVER & rs6000_isa_flags_explicit) + != (ISA_3_0_MASKS_SERVER & rs6000_isa_flags + & rs6000_isa_flags_explicit)) + /* Enforce that none of the ISA_3_0_MASKS_SERVER flags + were explicitly cleared. */ + error ("-mpower9-minmax incompatible with explicitly disabled options"); +#endif + else + rs6000_isa_flags |= + (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); + } else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO) rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~rs6000_isa_flags_explicit); else if (TARGET_VSX) This variant also results in numerous regressions: 942 new fewer "expected passes", 4 more "unexpected failures", 339 more "unsupported tests".
On Fri, Mar 31, 2017 at 06:35:20PM +0000, kelvin at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80108 > > --- Comment #9 from kelvin at gcc dot gnu.org --- > I've got a patch now that resolves this problem without creating regressions. > I'm attaching it for "discussion" here before I post for "official review". > > Index: gcc/config/rs6000/rs6000.c > =================================================================== > --- gcc/config/rs6000/rs6000.c (revision 246573) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -4273,8 +4273,30 @@ rs6000_option_override_internal (bool global_init_ > /* For the newer switches (vsx, dfp, etc.) set some of the older options, > unless the user explicitly used the -mno-<option> to disable the code. > */ > if (TARGET_P9_VECTOR || TARGET_MODULO || TARGET_P9_DFORM_SCALAR > - || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0 || > TARGET_P9_MINMAX) > + || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0) > rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit); > + else if (TARGET_P9_MINMAX) > + { > + if (have_cpu) > + { > + if (cpu_index == PROCESSOR_POWER9) > + /* legacy behavior: allow -mcpu-power9 with certain capabilities > + (eg -mno-vsx) explicitly disabled. */ Note, -mpower9-minmax makes no sense without -mvsx, since it is a VSX instruction. It also requires upper reg support for the appropriate mode (i.e. double requires TARGET_UPPER_REGS_DF and float requires TARGET_UPPER_REGS_SF, it is probably simpler to require both). This comes from a LRA behavior of crashing if the constraint allows more registers than the register class allows. We could add additional constraints in the min/max insns if we wanted to support no upper regs and min/max, but I don't think it is worth it. (define_insn "*s<minmax><mode>3_vsx" [(set (match_operand:SFDF 0 "vsx_register_operand" "=<Ff>,<Fv>") (fp_minmax:SFDF (match_operand:SFDF 1 "vsx_register_operand" "<Ff>,<Fv>") (match_operand:SFDF 2 "vsx_register_operand" "<Ff>,<Fv>")))] "TARGET_VSX && TARGET_<MODE>_FPR" { return (TARGET_P9_MINMAX ? "xs<minmax>cdp %x0,%x1,%x2" : "xs<minmax>dp %x0,%x1,%x2"); } [(set_attr "type" "fp")])
Author: kelvin Date: Mon Apr 10 19:01:37 2017 New Revision: 246818 URL: https://gcc.gnu.org/viewcvs?rev=246818&root=gcc&view=rev Log: gcc/ChangeLog: 2017-04-10 Kelvin Nilsen <kelvin@gcc.gnu.org> PR target/80108 * config/rs6000/rs6000.c (rs6000_option_override_internal): Enhance special handling given to the TARGET_P9_MINMAX option in relation to certain other options. gcc/testsuite/ChangeLog: 2017-04-10 Kelvin Nilsen <kelvin@gcc.gnu.org> PR target/80108 * gcc.target/powerpc/ppc-fortran/ppc-fortran.exp: New file. * gcc.target/powerpc/ppc-fortran/pr80108-1.f90: New test. Added: trunk/gcc/testsuite/gcc.target/powerpc/ppc-fortran/ trunk/gcc/testsuite/gcc.target/powerpc/ppc-fortran/ppc-fortran.exp trunk/gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr80108-1.f90 Modified: trunk/gcc/ChangeLog trunk/gcc/config/rs6000/rs6000.c trunk/gcc/testsuite/ChangeLog
committed patch has resolved this issue.