| Message ID | CAKwh3qh=FF-4zt7Gy26u3EJir0y2CHrtzX_MsuF3X5Oca2WhzA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Janus,
As part of Tobias's fix for PR50640, he introduced:
+ if ((sym->attr.flavor == FL_PARAMETER
+ && (sym->attr.dimension || sym->ts.type == BT_DERIVED))
+ || sym->attr.vtab)
TREE_READONLY (decl) = 1;
Is this not sufficient to fix this PR too?
Otherwise, your patch is, of course, OK.
Paul
On Tue, Nov 8, 2011 at 11:51 AM, Janus Weil <janus@gcc.gnu.org> wrote:
> Hi all,
>
> the attached patch marks the 'vtab' symbols as constant
> (FL_PARAMETER). They are fixed objects which are initialized once and
> never change.
>
> Regtested on x86_64-unknown-linux-gnu. Ok for trunk?
>
> Is it ok to commit without a test case? If not, any suggestions how a
> good test case could look like?
>
> Cheers,
> Janus
>
>
> 2011-11-08 Janus Weil <janus@gcc.gnu.org>
>
> PR fortran/50960
> * class.c (gfc_find_derived_vtab): Make the vtab symbols FL_PARAMETER.
> * expr.c (gfc_simplify_expr): Prevent vtabs from being replaced with
> their value.
> * resolve.c (resolve_values): Use-associated symbols do not need to
> be resolved again.
> (resolve_fl_parameter): Make sure the symbol has a value.
>
Hi Paul, > As part of Tobias's fix for PR50640, he introduced: > > + if ((sym->attr.flavor == FL_PARAMETER > + && (sym->attr.dimension || sym->ts.type == BT_DERIVED)) > + || sym->attr.vtab) > TREE_READONLY (decl) = 1; > > Is this not sufficient to fix this PR too? well, I think what you quote here is a proposed patch which was not committed. What Tobias actually committed for PR50960 is this: http://gcc.gnu.org/viewcvs/trunk/gcc/fortran/trans-decl.c?r1=180878&r2=180877&pathrev=180878 It does not contain any special case for vtabs (which is not needed if the vtabs become parameters). To answer your question: With respect to TREE_READONLY Tobias' patch is surely equivalent. I was hoping that FL_PARAMETER might have other (positive) side effects, but I'm not sure about that. If you think it's preferable, I can commit Tobias' version instead ... Cheers, Janus > On Tue, Nov 8, 2011 at 11:51 AM, Janus Weil <janus@gcc.gnu.org> wrote: >> Hi all, >> >> the attached patch marks the 'vtab' symbols as constant >> (FL_PARAMETER). They are fixed objects which are initialized once and >> never change. >> >> Regtested on x86_64-unknown-linux-gnu. Ok for trunk? >> >> Is it ok to commit without a test case? If not, any suggestions how a >> good test case could look like? >> >> Cheers, >> Janus >> >> >> 2011-11-08 Janus Weil <janus@gcc.gnu.org> >> >> PR fortran/50960 >> * class.c (gfc_find_derived_vtab): Make the vtab symbols FL_PARAMETER. >> * expr.c (gfc_simplify_expr): Prevent vtabs from being replaced with >> their value. >> * resolve.c (resolve_values): Use-associated symbols do not need to >> be resolved again. >> (resolve_fl_parameter): Make sure the symbol has a value. >> > > > > -- > The knack of flying is learning how to throw yourself at the ground and miss. > --Hitchhikers Guide to the Galaxy >
Dear Janus, I am asking the question :-) Are the two equivalent? To my mind, it is a matter of taste, if they are. Cheers Paul On Tue, Nov 8, 2011 at 9:02 PM, Janus Weil <janus@gcc.gnu.org> wrote: > Hi Paul, > >> As part of Tobias's fix for PR50640, he introduced: >> >> + if ((sym->attr.flavor == FL_PARAMETER >> + && (sym->attr.dimension || sym->ts.type == BT_DERIVED)) >> + || sym->attr.vtab) >> TREE_READONLY (decl) = 1; >> >> Is this not sufficient to fix this PR too? > > well, I think what you quote here is a proposed patch which was not > committed. What Tobias actually committed for PR50960 is this: > > http://gcc.gnu.org/viewcvs/trunk/gcc/fortran/trans-decl.c?r1=180878&r2=180877&pathrev=180878 > > It does not contain any special case for vtabs (which is not needed if > the vtabs become parameters). > > To answer your question: With respect to TREE_READONLY Tobias' patch > is surely equivalent. I was hoping that FL_PARAMETER might have other > (positive) side effects, but I'm not sure about that. > > If you think it's preferable, I can commit Tobias' version instead ... > > Cheers, > Janus > > > >> On Tue, Nov 8, 2011 at 11:51 AM, Janus Weil <janus@gcc.gnu.org> wrote: >>> Hi all, >>> >>> the attached patch marks the 'vtab' symbols as constant >>> (FL_PARAMETER). They are fixed objects which are initialized once and >>> never change. >>> >>> Regtested on x86_64-unknown-linux-gnu. Ok for trunk? >>> >>> Is it ok to commit without a test case? If not, any suggestions how a >>> good test case could look like? >>> >>> Cheers, >>> Janus >>> >>> >>> 2011-11-08 Janus Weil <janus@gcc.gnu.org> >>> >>> PR fortran/50960 >>> * class.c (gfc_find_derived_vtab): Make the vtab symbols FL_PARAMETER. >>> * expr.c (gfc_simplify_expr): Prevent vtabs from being replaced with >>> their value. >>> * resolve.c (resolve_values): Use-associated symbols do not need to >>> be resolved again. >>> (resolve_fl_parameter): Make sure the symbol has a value. >>> >> >> >> >> -- >> The knack of flying is learning how to throw yourself at the ground and miss. >> --Hitchhikers Guide to the Galaxy >> >
Paul, > I am asking the question :-) Are the two equivalent? To my mind, it > is a matter of taste, if they are. in principle I prefer the approach of giving the vtabs the flavor 'FL_PARAMETER' from the start. However, that induces a couple of regressions which are fixed by the (more ugly) additions in expr.c and resolve.c. So, I'm not sure what's better in the end. I'll wait until tomorrow, to give Tobias (or others) a chance to comment. Then I'll commit my version of the patch. Cheers, Janus > On Tue, Nov 8, 2011 at 9:02 PM, Janus Weil <janus@gcc.gnu.org> wrote: >> Hi Paul, >> >>> As part of Tobias's fix for PR50640, he introduced: >>> >>> + if ((sym->attr.flavor == FL_PARAMETER >>> + && (sym->attr.dimension || sym->ts.type == BT_DERIVED)) >>> + || sym->attr.vtab) >>> TREE_READONLY (decl) = 1; >>> >>> Is this not sufficient to fix this PR too? >> >> well, I think what you quote here is a proposed patch which was not >> committed. What Tobias actually committed for PR50960 is this: >> >> http://gcc.gnu.org/viewcvs/trunk/gcc/fortran/trans-decl.c?r1=180878&r2=180877&pathrev=180878 >> >> It does not contain any special case for vtabs (which is not needed if >> the vtabs become parameters). >> >> To answer your question: With respect to TREE_READONLY Tobias' patch >> is surely equivalent. I was hoping that FL_PARAMETER might have other >> (positive) side effects, but I'm not sure about that. >> >> If you think it's preferable, I can commit Tobias' version instead ... >> >> Cheers, >> Janus >> >> >> >>> On Tue, Nov 8, 2011 at 11:51 AM, Janus Weil <janus@gcc.gnu.org> wrote: >>>> Hi all, >>>> >>>> the attached patch marks the 'vtab' symbols as constant >>>> (FL_PARAMETER). They are fixed objects which are initialized once and >>>> never change. >>>> >>>> Regtested on x86_64-unknown-linux-gnu. Ok for trunk? >>>> >>>> Is it ok to commit without a test case? If not, any suggestions how a >>>> good test case could look like? >>>> >>>> Cheers, >>>> Janus >>>> >>>> >>>> 2011-11-08 Janus Weil <janus@gcc.gnu.org> >>>> >>>> PR fortran/50960 >>>> * class.c (gfc_find_derived_vtab): Make the vtab symbols FL_PARAMETER. >>>> * expr.c (gfc_simplify_expr): Prevent vtabs from being replaced with >>>> their value. >>>> * resolve.c (resolve_values): Use-associated symbols do not need to >>>> be resolved again. >>>> (resolve_fl_parameter): Make sure the symbol has a value. >>>> >>> >>> >>> >>> -- >>> The knack of flying is learning how to throw yourself at the ground and miss. >>> --Hitchhikers Guide to the Galaxy >>> >> > > > > -- > The knack of flying is learning how to throw yourself at the ground and miss. > --Hitchhikers Guide to the Galaxy >
Dear Paul, Paul Richard Thomas wrote: > I am asking the question :-) Are the two equivalent? To my mind, it > is a matter of taste, if they are. I think in practice they are the same. A derived-type entity with parameter attribute ends up as static variable with TREE_READONLY. Thus, within the same file (or with LTO) the middle end can do all kind of fancy optimizations and in particular it can "devirtualize" function calls. In principle, the FE could do replacements at compile time if one loads a module with a derived-type parameter. However, I think the value is not stored in the .mod file and hence, a module user in a different translation unit would not be able to replace, e.g., the _vtable.size information at compile time. And for methods (type-bound procedures), I think it is basically impossible to do the compile-time replacement in the FE. Thus, for the size or hash, I could imagine that PARAMETER has a slight advantage - but only if we modify the information stored in the .mod file. Hence, with the current .mod information and (even with modified .mod) with typical usage, the result should be the same. (The only thing I am not quite sure is Janus' change to resolve.c, i.e. whether it avoids re-running diagnostic checks [which is good] for use-associated variables (known to be valid from the initial resolution) or whether it can lead to a missing diagnostic [which would be bad].) * * * Regarding my patch: It is at least shorter, a single "|| attr.vtab". But I don't have an opinion which patch is better. (All the work with the VTAB flag and push_to_toplevel is unrelated to that issue, but should also be solved at some point.) Tobias
2011/11/8 Tobias Burnus <burnus@net-b.de>: >> I am asking the question :-) Are the two equivalent? To my mind, it >> is a matter of taste, if they are. > > I think in practice they are the same. Alright, since no one seems to have any strong preference, I'll just go ahead and commit my version of the patch (as approved by Paul). Cheers, Janus
> Alright, since no one seems to have any strong preference, I'll just > go ahead and commit my version of the patch (as approved by Paul). Committed as r181199. Thanks for the comments, everyone! Cheers, Janus
Index: gcc/fortran/class.c =================================================================== --- gcc/fortran/class.c (revision 181107) +++ gcc/fortran/class.c (working copy) @@ -428,7 +428,7 @@ gfc_find_derived_vtab (gfc_symbol *derived) { gfc_get_symbol (name, ns, &vtab); vtab->ts.type = BT_DERIVED; - if (gfc_add_flavor (&vtab->attr, FL_VARIABLE, NULL, + if (gfc_add_flavor (&vtab->attr, FL_PARAMETER, NULL, &gfc_current_locus) == FAILURE) goto cleanup; vtab->attr.target = 1; Index: gcc/fortran/expr.c =================================================================== --- gcc/fortran/expr.c (revision 181106) +++ gcc/fortran/expr.c (working copy) @@ -1883,7 +1883,8 @@ gfc_simplify_expr (gfc_expr *p, int type) initialization expression, or we want a subsection. */ if (p->symtree->n.sym->attr.flavor == FL_PARAMETER && (gfc_init_expr_flag || p->ref - || p->symtree->n.sym->value->expr_type != EXPR_ARRAY)) + || p->symtree->n.sym->value->expr_type != EXPR_ARRAY) + && !p->symtree->n.sym->attr.vtab) { if (simplify_parameter_variable (p, type) == FAILURE) return FAILURE; Index: gcc/fortran/resolve.c =================================================================== --- gcc/fortran/resolve.c (revision 181107) +++ gcc/fortran/resolve.c (working copy) @@ -9514,7 +9514,7 @@ resolve_values (gfc_symbol *sym) { gfc_try t; - if (sym->value == NULL) + if (sym->value == NULL || sym->attr.use_assoc) return; if (sym->value->expr_type == EXPR_STRUCTURE) @@ -11982,7 +11982,7 @@ resolve_fl_parameter (gfc_symbol *sym) /* Make sure the types of derived parameters are consistent. This type checking is deferred until resolution because the type may refer to a derived type from the host. */ - if (sym->ts.type == BT_DERIVED + if (sym->ts.type == BT_DERIVED && sym->value && !gfc_compare_types (&sym->ts, &sym->value->ts)) { gfc_error ("Incompatible derived type in PARAMETER at %L",