Bug 80108 - ICE in aggregate_value_p at function.c:2028
Summary: ICE in aggregate_value_p at function.c:2028
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: kelvin
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2017-03-20 08:50 UTC by Martin Liška
Modified: 2017-04-10 19:04 UTC (History)
4 users (show)

See Also:
Host: x86_64-linux-gnu
Target: ppc64-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-03-21 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2017-03-20 08:50:35 UTC
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
Comment 1 Segher Boessenkool 2017-03-21 03:30:25 UTC
-mpower9-minmax enables VSX support, and we end up with carnage (during
expand already).  No big surprise when targeting 405.
Comment 2 kelvin 2017-03-29 17:14:38 UTC
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?
Comment 3 kelvin 2017-03-29 17:24:30 UTC
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/
Comment 4 Bill Schmidt 2017-03-29 18:10:04 UTC
(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.
Comment 5 Michael Meissner 2017-03-29 18:11:37 UTC
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.
Comment 6 Bill Schmidt 2017-03-29 18:25:23 UTC
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...
Comment 7 kelvin 2017-03-29 20:06:03 UTC
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...   :(
Comment 8 Segher Boessenkool 2017-03-29 22:16:51 UTC
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.
Comment 9 kelvin 2017-03-31 18:35:20 UTC
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.
Comment 10 kelvin 2017-03-31 21:10:20 UTC
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".
Comment 11 Michael Meissner 2017-04-01 20:53:57 UTC
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")])
Comment 12 kelvin 2017-04-10 19:02:09 UTC
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
Comment 13 kelvin 2017-04-10 19:04:10 UTC
committed patch has resolved this issue.