Patchwork [Fortran,OOP] PR 50960: vtables not marked as constant

login
register
mail settings
Submitter Janus Weil
Date Nov. 8, 2011, 10:51 a.m.
Message ID <CAKwh3qh=FF-4zt7Gy26u3EJir0y2CHrtzX_MsuF3X5Oca2WhzA@mail.gmail.com>
Download mbox | patch
Permalink /patch/124320/
State New
Headers show

Comments

Janus Weil - Nov. 8, 2011, 10:51 a.m.
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.
Paul Richard Thomas - Nov. 8, 2011, 6:34 p.m.
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.
>
Janus Weil - Nov. 8, 2011, 8:02 p.m.
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 Richard Thomas - Nov. 8, 2011, 8:37 p.m.
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
>>
>
Janus Weil - Nov. 8, 2011, 8:59 p.m.
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
>
Tobias Burnus - Nov. 8, 2011, 9:56 p.m.
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
Janus Weil - Nov. 9, 2011, 8:37 a.m.
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
Janus Weil - Nov. 9, 2011, 9:48 a.m.
> 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

Patch

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",