Message ID | CAKwh3qjDOO1OfPk7CPdOR2n84A=gR53Yqb_FTO3Vs1DuBiASQQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [Fortran] PR 88009: [9 Regression] ICE in find_intrinsic_vtab, at fortran/class.c:2761 | expand |
Hi Janus, > the attached patch fixes PR 88009, an ICE-on-invalid regression caused > by one of my earlier commits. Apart from adding some extra checks to > avoid ICEs, it also uses the 'artificial' attribute to suppress bogus > errors (see comment #3) and does some minor cleanup in > resolve_fl_variable. > > Regtests cleanly on x86_64-linux-gnu. Ok for trunk? the patch looks good, OK for trunk. Regards Thomas
Am Sa., 5. Jan. 2019 um 14:37 Uhr schrieb Thomas Koenig <tkoenig@netcologne.de>: > > Hi Janus, > > > the attached patch fixes PR 88009, an ICE-on-invalid regression caused > > by one of my earlier commits. Apart from adding some extra checks to > > avoid ICEs, it also uses the 'artificial' attribute to suppress bogus > > errors (see comment #3) and does some minor cleanup in > > resolve_fl_variable. > > > > Regtests cleanly on x86_64-linux-gnu. Ok for trunk? > > > the patch looks good, OK for trunk. thanks for the quick review, Thomas! Committed as r267598. Cheers, Janus
On Sat, Jan 05, 2019 at 03:35:21PM +0100, Janus Weil wrote: > Am Sa., 5. Jan. 2019 um 14:37 Uhr schrieb Thomas Koenig <tkoenig@netcologne.de>: > > > > Hi Janus, > > > > > the attached patch fixes PR 88009, an ICE-on-invalid regression caused > > > by one of my earlier commits. Apart from adding some extra checks to > > > avoid ICEs, it also uses the 'artificial' attribute to suppress bogus > > > errors (see comment #3) and does some minor cleanup in > > > resolve_fl_variable. > > > > > > Regtests cleanly on x86_64-linux-gnu. Ok for trunk? > > > > > > the patch looks good, OK for trunk. > > thanks for the quick review, Thomas! Committed as r267598. > I still need to verify, but it appears that this commit is causing a regression on x86_64-*-freebsd. FAIL: gfortran.dg/class_alias.f90 -Os execution test Reverting parts of commits and rebuilding and testing takes about an hour or more, verification won't be available for bit.
On Sat, Jan 05, 2019 at 11:39:08AM -0800, Steve Kargl wrote: > On Sat, Jan 05, 2019 at 03:35:21PM +0100, Janus Weil wrote: > > Am Sa., 5. Jan. 2019 um 14:37 Uhr schrieb Thomas Koenig <tkoenig@netcologne.de>: > > > > > > Hi Janus, > > > > > > > the attached patch fixes PR 88009, an ICE-on-invalid regression caused > > > > by one of my earlier commits. Apart from adding some extra checks to > > > > avoid ICEs, it also uses the 'artificial' attribute to suppress bogus > > > > errors (see comment #3) and does some minor cleanup in > > > > resolve_fl_variable. > > > > > > > > Regtests cleanly on x86_64-linux-gnu. Ok for trunk? > > > > > > > > > the patch looks good, OK for trunk. > > > > thanks for the quick review, Thomas! Committed as r267598. > > > > I still need to verify, but it appears that this commit > is causing a regression on x86_64-*-freebsd. > > FAIL: gfortran.dg/class_alias.f90 -Os execution test > > Reverting parts of commits and rebuilding and testing takes > about an hour or more, verification won't be available for > bit. > More info from build log. Executing on host: /safe/sgk/gcc/objx/gcc/testsuite/gfortran/../../gfortran -B/safe/sgk/gcc/objx/gcc/testsuite/gfortran/../../ -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/ /safe/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/class_alias.f90 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -Os -fdump-tree-original -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libatomic/.libs -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -lm -o ./class_alias.exe (timeout = 300) spawn -ignore SIGHUP /safe/sgk/gcc/objx/gcc/testsuite/gfortran/../../gfortran -B/safe/sgk/gcc/objx/gcc/testsuite/gfortran/../../ -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/ /safe/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/class_alias.f90 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -Os -fdump-tree-original -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libatomic/.libs -B/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -L/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs -lm -o ./class_alias.exe PASS: gfortran.dg/class_alias.f90 -Os (test for excess errors) Setting LD_LIBRARY_PATH to .:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libatomic/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs:/safe/sgk/gcc/objx/gcc:.:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libgfortran/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libatomic/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs:/safe/sgk/gcc/objx/x86_64-unknown-freebsd13.0/./libquadmath/.libs:/safe/sgk/gcc/objx/gcc Execution timeout is: 300 spawn [open ...] STOP 2 FAIL: gfortran.dg/class_alias.f90 -Os execution test STOP 2 occurs at the end of testcase, so I suspect finalization is messed up.
On Sat, Jan 05, 2019 at 11:45:42AM -0800, Steve Kargl wrote: > Execution timeout is: 300 > spawn [open ...] > STOP 2 > FAIL: gfortran.dg/class_alias.f90 -Os execution test > > STOP 2 occurs at the end of testcase, so I suspect finalization is messed up. This is bizarre. Changing the code where STOP 2 occurs to if (var_p%x .ne. 2) then print *, var_p%x STOP 2 end if Compiling with gfortran gives % gfcx -o z -Os class_alias.f90 && ./z 2 STOP 2 Something is broken with -Os and your patch. The original dump contains if (var_p._data->x != 2) { { struct __st_parameter_dt dt_parm.5; dt_parm.5.common.filename = &"class_alias.f90"[1]{lb: 1 sz: 1}; dt_parm.5.common.line = 92; dt_parm.5.common.flags = 128; dt_parm.5.common.unit = 6; _gfortran_st_write (&dt_parm.5); _gfortran_transfer_integer_write (&dt_parm.5, &var_p._data->x, 4); _gfortran_st_write_done (&dt_parm.5); } _gfortran_stop_numeric (2, 0); } which looks correct. The -fdump-tree-optimized looks like var_p has been sort of skipped. _11 = var_a._data; _12 = _11->x; if (_12 != 2) goto <bb 6>; [0.04%] else goto <bb 7>; [99.96%] <bb 6> [local count: 429153]: _gfortran_stop_numeric (1, 0); This is the 'if (var_a%x /= 2) STOP 1' chunk. We jump to <bb 7>, which is the body of the if-endif block that contains STOP 2. <bb 7> [local count: 428982]: dt_parm.5.common.filename = &"class_alias.f90"[1]{lb: 1 sz: 1}; dt_parm.5.common.line = 92; MEM[(integer(kind=4) *)&dt_parm.5] = 25769803904; _gfortran_st_write (&dt_parm.5); _14 = &MEM[(struct test *)_6].x; _gfortran_transfer_integer_write (&dt_parm.5, _14, 4); _gfortran_st_write_done (&dt_parm.5); dt_parm.5 ={v} {CLOBBER}; _gfortran_stop_numeric (2, 0); I would expect the basic block <bb 7> to look like <bb 7> [local count: xxxxx] _13 = var_p._data; _14 = _13->x; if (_14 != 2) goto <bb 8> else goto <bb 9> Now, what is <bb 7> above would be the contents of <bb 8> and <bb 9> would then jump to the deallocation statements.
> > FAIL: gfortran.dg/class_alias.f90 -Os execution test > > I see this failure on trunk as well, but I don't think it was > introduced by my commit. Instead, I see I it starting with r267601: > > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=267601 > > Jan, could you please have a look? Actually I have noticed about half an hour ago that i commited WIP variant of the patch. It is fixed by this patch I apologize for the breakage. Index: ChangeLog =================================================================== --- ChangeLog (revision 267602) +++ ChangeLog (working copy) @@ -1,5 +1,15 @@ 2019-01-05 Jan Hubicka <hubicka@ucw.cz> + * doc/invoke.texi (max-inline-insns-small): New parameters. + * ipa-inline.c (want_early_inline_function_p): simplify. + (want_inline_small_function_p): Fix pasto from previous patch; + use max-inline-insns-small bound. + * params.def (max-inline-insns-small): New param. + * ipa-fnsummary.c (analyze_function_body): Initialize time/size + variables correctly. + +2019-01-05 Jan Hubicka <hubicka@ucw.cz> + * doc/invoke.texi: Document max-inline-insns-size, uninlined-function-insns, uninlined-function-time, uninlined-thunk-insns and uninlined-thunk-time. Index: testsuite/ChangeLog =================================================================== --- testsuite/ChangeLog (revision 267601) +++ testsuite/ChangeLog (working copy) @@ -1,3 +1,7 @@ +2019-01-05 Jan Hubicka <hubicka@ucw.cz> + + * gcc.dg/ipa/ipcp-2.c: Update bounds. + 2019-01-05 Dominique d'Humieres <dominiq@gcc.gnu.org> * gcc.dg/plugin/plugindir1.c: Adjust dg-prune-output for Darwin. Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 267601) +++ doc/invoke.texi (working copy) @@ -11007,6 +11007,10 @@ by the compiler are investigated. To th (more restrictive) limit compared to functions declared inline can be applied. +@item max-inline-insns-small +This is bound applied to calls which are considered relevant with +@option{-finline-small-functions}. + @item max-inline-insns-size This is bound applied to calls which are optimized for size. Small growth may be desirable to anticipate optimization oppurtunities exposed by inlining. Index: testsuite/gcc.dg/ipa/ipcp-2.c =================================================================== --- testsuite/gcc.dg/ipa/ipcp-2.c (revision 267601) +++ testsuite/gcc.dg/ipa/ipcp-2.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp -fno-early-inlining --param ipa-cp-eval-threshold=80" } */ +/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp -fno-early-inlining --param ipa-cp-eval-threshold=200" } */ /* { dg-add-options bind_pic_locally } */ extern int get_stuff (int); Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 267601) +++ ipa-inline.c (working copy) @@ -637,8 +637,7 @@ want_early_inline_function_p (struct cgr if (growth <= PARAM_VALUE (PARAM_MAX_INLINE_INSNS_SIZE)) ; - else if (!e->maybe_hot_p () - && growth > 0) + else if (!e->maybe_hot_p ()) { if (dump_enabled_p ()) dump_printf_loc (MSG_MISSED_OPTIMIZATION, e->call_stmt, @@ -791,7 +790,7 @@ want_inline_small_function_p (struct cgr ipa_hints hints = estimate_edge_hints (e); int big_speedup = -1; /* compute this lazily */ - if (growth <= PARAM_VALUE (PARAM_VALUE (PARAM_MAX_INLINE_INSNS_SIZE))) + if (growth <= PARAM_VALUE (PARAM_MAX_INLINE_INSNS_SIZE)) ; /* Apply MAX_INLINE_INSNS_SINGLE limit. Do not do so when hints suggests that inlining given function is very profitable. */ @@ -809,9 +808,8 @@ want_inline_small_function_p (struct cgr want_inline = false; } else if (!DECL_DECLARED_INLINE_P (callee->decl) - && (in_lto_p - && growth >= PARAM_VALUE (PARAM_EARLY_INLINING_INSNS)) - && !opt_for_fn (e->caller->decl, flag_inline_functions)) + && !opt_for_fn (e->caller->decl, flag_inline_functions) + && growth >= PARAM_VALUE (PARAM_MAX_INLINE_INSNS_SMALL)) { /* growth_likely_positive is expensive, always test it last. */ if (growth >= MAX_INLINE_INSNS_SINGLE Index: params.def =================================================================== --- params.def (revision 267601) +++ params.def (working copy) @@ -83,6 +83,11 @@ DEFPARAM (PARAM_MAX_INLINE_INSNS_AUTO, "The maximum number of instructions when automatically inlining.", 30, 0, 0) +DEFPARAM (PARAM_MAX_INLINE_INSNS_SMALL, + "max-inline-insns-small", + "The maximum number of instructions when automatically inlining small functions.", + 0, 0, 0) + DEFPARAM (PARAM_MAX_INLINE_INSNS_SIZE, "max-inline-insns-size", "The maximum number of instructions when inlining for size.", Index: ipa-fnsummary.c =================================================================== --- ipa-fnsummary.c (revision 267601) +++ ipa-fnsummary.c (working copy) @@ -1969,9 +1969,9 @@ fp_expression_p (gimple *stmt) static void analyze_function_body (struct cgraph_node *node, bool early) { - sreal time = 0; + sreal time = PARAM_VALUE (PARAM_UNINLINED_FUNCTION_TIME); /* Estimate static overhead for function prologue/epilogue and alignment. */ - int size = 2; + int size = PARAM_VALUE (PARAM_UNINLINED_FUNCTION_INSNS); /* Benefits are scaled by probability of elimination that is in range <0,2>. */ basic_block bb;
On Sat, Jan 05, 2019 at 11:48:15PM +0100, Jan Hubicka wrote: > > > FAIL: gfortran.dg/class_alias.f90 -Os execution test > > > > I see this failure on trunk as well, but I don't think it was > > introduced by my commit. Instead, I see I it starting with r267601: > > > > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=267601 > > > > Jan, could you please have a look? > > Actually I have noticed about half an hour ago that i commited WIP > variant of the patch. It is fixed by this patch > > I apologize for the breakage. > > Index: ChangeLog > =================================================================== > --- ChangeLog (revision 267602) > +++ ChangeLog (working copy) > @@ -1,5 +1,15 @@ > 2019-01-05 Jan Hubicka <hubicka@ucw.cz> > > + * doc/invoke.texi (max-inline-insns-small): New parameters. > + * ipa-inline.c (want_early_inline_function_p): simplify. > + (want_inline_small_function_p): Fix pasto from previous patch; > + use max-inline-insns-small bound. > + * params.def (max-inline-insns-small): New param. > + * ipa-fnsummary.c (analyze_function_body): Initialize time/size > + variables correctly. > + /gfortran.dg/pr79966.f90 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O -O2 -fpeel-loops -finline-functions -fipa-cp-clone -fdump-ipa-inline-details -S -o pr79966.s PASS: gfortran.dg/pr79966.f90 -O (test for excess errors) FAIL: gfortran.dg/pr79966.f90 -O scan-ipa-dump inline "Inlined tp_sum/[0-9]+ into runtptests/[0-9]+" testcase /safe/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/dg.exp completed in 0 seconds on x86_64-*-freebsd.
On Sat, Jan 05, 2019 at 03:50:06PM -0800, Steve Kargl wrote: > > /gfortran.dg/pr79966.f90 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O -O2 -fpeel-loops -finline-functions -fipa-cp-clone -fdump-ipa-inline-details -S -o pr79966.s > PASS: gfortran.dg/pr79966.f90 -O (test for excess errors) > FAIL: gfortran.dg/pr79966.f90 -O scan-ipa-dump inline "Inlined tp_sum/[0-9]+ into runtptests/[0-9]+" > testcase /safe/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/dg.exp completed in 0 seconds > > on x86_64-*-freebsd. > This Bug ipa/88711
> On Sat, Jan 05, 2019 at 03:50:06PM -0800, Steve Kargl wrote: > > > > /gfortran.dg/pr79966.f90 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O -O2 -fpeel-loops -finline-functions -fipa-cp-clone -fdump-ipa-inline-details -S -o pr79966.s > > PASS: gfortran.dg/pr79966.f90 -O (test for excess errors) > > FAIL: gfortran.dg/pr79966.f90 -O scan-ipa-dump inline "Inlined tp_sum/[0-9]+ into runtptests/[0-9]+" > > testcase /safe/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/dg.exp completed in 0 seconds > > > > on x86_64-*-freebsd. > > > > This Bug ipa/88711 Thanks, I will take a look tomorrow. https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00218.html fixes rather important and old bug in accounting runtime estimate for functions. This affects decisions of inliner and ipa-cp, so the testcase may need retuning. I am working on retuning of the inlining parameters and discovered the bug while looking into few anomalies. Funilly it means that I can start my tests over. Honza > > -- > Steve
diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c index e55ab25882f..77f0fca9385 100644 --- a/gcc/fortran/class.c +++ b/gcc/fortran/class.c @@ -2466,6 +2466,7 @@ gfc_find_derived_vtab (gfc_symbol *derived) goto cleanup; c->attr.proc_pointer = 1; c->attr.access = ACCESS_PRIVATE; + c->attr.artificial = 1; c->tb = XCNEW (gfc_typebound_proc); c->tb->ppc = 1; generate_finalization_wrapper (derived, ns, tname, c); @@ -2762,9 +2763,9 @@ find_intrinsic_vtab (gfc_typespec *ts) /* This is elemental so that arrays are automatically treated correctly by the scalarizer. */ copy->attr.elemental = 1; - if (ns->proc_name->attr.flavor == FL_MODULE) + if (ns->proc_name && ns->proc_name->attr.flavor == FL_MODULE) copy->module = ns->proc_name->name; - gfc_set_sym_referenced (copy); + gfc_set_sym_referenced (copy); /* Set up formal arguments. */ gfc_get_symbol ("src", sub_ns, &src); src->ts.type = ts->type; @@ -2798,6 +2799,7 @@ find_intrinsic_vtab (gfc_typespec *ts) goto cleanup; c->attr.proc_pointer = 1; c->attr.access = ACCESS_PRIVATE; + c->attr.artificial = 1; c->tb = XCNEW (gfc_typebound_proc); c->tb->ppc = 1; c->initializer = gfc_get_null_expr (NULL); diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index a498d19e411..beafe8da8bc 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -12274,13 +12274,8 @@ deferred_requirements (gfc_symbol *sym) static bool resolve_fl_variable (gfc_symbol *sym, int mp_flag) { - int no_init_flag, automatic_flag; - gfc_expr *e; - const char *auto_save_msg; - bool saved_specification_expr; - - auto_save_msg = "Automatic object %qs at %L cannot have the " - "SAVE attribute"; + const char *auto_save_msg = "Automatic object %qs at %L cannot have the " + "SAVE attribute"; if (!resolve_fl_var_and_proc (sym, mp_flag)) return false; @@ -12288,7 +12283,7 @@ resolve_fl_variable (gfc_symbol *sym, int mp_flag) /* Set this flag to check that variables are parameters of all entries. This check is effected by the call to gfc_resolve_expr through is_non_constant_shape_array. */ - saved_specification_expr = specification_expr; + bool saved_specification_expr = specification_expr; specification_expr = true; if (sym->ns->proc_name @@ -12315,6 +12310,8 @@ resolve_fl_variable (gfc_symbol *sym, int mp_flag) { /* Make sure that character string variables with assumed length are dummy arguments. */ + gfc_expr *e = NULL; + if (sym->ts.u.cl) e = sym->ts.u.cl->length; else @@ -12364,7 +12361,7 @@ resolve_fl_variable (gfc_symbol *sym, int mp_flag) apply_default_init_local (sym); /* Try to apply a default initialization. */ /* Determine if the symbol may not have an initializer. */ - no_init_flag = automatic_flag = 0; + int no_init_flag = 0, automatic_flag = 0; if (sym->attr.allocatable || sym->attr.external || sym->attr.dummy || sym->attr.intrinsic || sym->attr.result) no_init_flag = 1; @@ -12494,7 +12491,7 @@ resolve_fl_procedure (gfc_symbol *sym, int mp_flag) module procedures are excluded by 2.2.3.3 - i.e., they are not externally accessible and can access all the objects accessible in the host. */ - if (!(sym->ns->parent + if (!(sym->ns->parent && sym->ns->parent->proc_name && sym->ns->parent->proc_name->attr.flavor == FL_MODULE) && gfc_check_symbol_access (sym)) { diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index a88e7c01df7..cd52c73031b 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -440,6 +440,9 @@ check_conflict (symbol_attribute *attr, const char *name, locus *where) const char *a1, *a2; int standard; + if (attr->artificial) + return true; + if (where == NULL) where = &gfc_current_locus; diff --git a/gcc/testsuite/gfortran.dg/blockdata_10.f90 b/gcc/testsuite/gfortran.dg/blockdata_10.f90 new file mode 100644 index 00000000000..ce7ba25c269 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/blockdata_10.f90 @@ -0,0 +1,13 @@ +! { dg-do compile } +! +! PR 88009: [9 Regression] ICE in find_intrinsic_vtab, at fortran/class.c:2761 +! +! Contributed by G. Steinmetz <gscfq@t-online.de> + +module m + class(*), allocatable :: z +end +block data + use m + z = 'z' ! { dg-error "assignment statement is not allowed|Unexpected assignment statement" } +end