Re: [PATCH] Fix type field walking in gimplifier unsharing

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Thu, 28 Apr 2016, Richard Biener wrote:

> On Thu, 28 Apr 2016, Eric Botcazou wrote:
> 
> > > Aww, I was hoping for sth that would not require me to fix all
> > > frontends ...
> > 
> > I don't really see how this can work without DECL_EXPR though.  You need to 
> > define when the variable-sized expressions are evaluated to lay out the type, 
> > otherwise it will be laid out on the first use, which may see a different 
> > value of the expressions than the definition point.  The only way to do that 
> > for a locally-defined type is to add a DECL_EXPR in GENERIC, so that the 
> > gimplifier evaluates the expressions at the right spot.
> 
> Ah, so the C++ FE does this correctly but in addition to that it has
> 
>           /* When the pointed-to type involves components of variable 
> size,
>              care must be taken to ensure that the size evaluation code is
>              emitted early enough to dominate all the possible later uses
>              and late enough for the variables on which it depends to have
>              been assigned.
> 
>              This is expected to happen automatically when the pointed-to
>              type has a name/declaration of it's own, but special 
> attention
>              is required if the type is anonymous.
> ...
>           if (!TYPE_NAME (type)
>               && (decl_context == NORMAL || decl_context == FIELD)
>               && at_function_scope_p ()
>               && variably_modified_type_p (type, NULL_TREE))
>             /* Force evaluation of the SAVE_EXPR.  */
>             finish_expr_stmt (TYPE_SIZE (type));
> 
> so in this case the type doesn't have an associated TYPE_DECL and thus
> we can't build a DECL_EXPR.  To me the correct fix is then to
> always force a TYPE_DECL for variable-modified types.
> 
> Jason?

The following works (for the testcase):

Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c       (revision 235547)
+++ gcc/cp/decl.c       (working copy)
@@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
              && (decl_context == NORMAL || decl_context == FIELD)
              && at_function_scope_p ()
              && variably_modified_type_p (type, NULL_TREE))
-           /* Force evaluation of the SAVE_EXPR.  */
-           finish_expr_stmt (TYPE_SIZE (type));
+           {
+             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
+                                            NULL_TREE, type);
+             add_decl_expr (TYPE_NAME (type));
+           }
 
          if (declarator->kind == cdk_reference)
            {

and I have a similar fix for the Fortran FE for one testcase I
reduced to

  character(10), dimension (2) :: implicit_result
  character(10), dimension (2) :: source
  implicit_result = reallocate_hnv (LEN (source))
contains
  FUNCTION reallocate_hnv(LEN)
    CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
  END FUNCTION reallocate_hnv
end

Index: fortran/trans-array.c
===================================================================
--- fortran/trans-array.c       (revision 235547)
+++ fortran/trans-array.c       (working copy)
@@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
   info->descriptor = desc;
   size = gfc_index_one_node;
 
+  /* Emit a DECL_EXPR for the variable sized array type in
+     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
+     sizes works correctly.  */
+  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
+  if (! TYPE_NAME (arraytype))
+    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
+                                       NULL_TREE, arraytype);
+  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
+                                     arraytype, TYPE_NAME (arraytype)));
+
   /* Fill in the array dtype.  */
   tmp = gfc_conv_descriptor_dtype (desc);
   gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));


I wonder if we can avoid allocating the TYPE_DECL by simply also
allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
a 'TYPE_EXPR').

Richard.

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Eric Botcazou]

> The following works (for the testcase):
> 
> Index: gcc/cp/decl.c
> ===================================================================
> --- gcc/cp/decl.c       (revision 235547)
> +++ gcc/cp/decl.c       (working copy)
> @@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
>               && (decl_context == NORMAL || decl_context == FIELD)
>               && at_function_scope_p ()
>               && variably_modified_type_p (type, NULL_TREE))
> -           /* Force evaluation of the SAVE_EXPR.  */
> -           finish_expr_stmt (TYPE_SIZE (type));
> +           {
> +             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> +                                            NULL_TREE, type);
> +             add_decl_expr (TYPE_NAME (type));
> +           }
> 
>           if (declarator->kind == cdk_reference)
>             {
> 
> and I have a similar fix for the Fortran FE for one testcase I
> reduced to
> 
>   character(10), dimension (2) :: implicit_result
>   character(10), dimension (2) :: source
>   implicit_result = reallocate_hnv (LEN (source))
> contains
>   FUNCTION reallocate_hnv(LEN)
>     CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
>   END FUNCTION reallocate_hnv
> end
> 
> Index: fortran/trans-array.c
> ===================================================================
> --- fortran/trans-array.c       (revision 235547)
> +++ fortran/trans-array.c       (working copy)
> @@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
>    info->descriptor = desc;
>    size = gfc_index_one_node;
> 
> +  /* Emit a DECL_EXPR for the variable sized array type in
> +     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
> +     sizes works correctly.  */
> +  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
> +  if (! TYPE_NAME (arraytype))
> +    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> +                                       NULL_TREE, arraytype);
> +  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
> +                                     arraytype, TYPE_NAME (arraytype)));
> +
>    /* Fill in the array dtype.  */
>    tmp = gfc_conv_descriptor_dtype (desc);
>    gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));

Great.  We do exactly that in the Ada compiler (but of course the number of 
places where we need to do it is an order of magnitude larger).

> I wonder if we can avoid allocating the TYPE_DECL by simply also
> allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
> a 'TYPE_EXPR').

I agree that DECL_EXPR + TYPE_DECL is a bit heavy, but I'm not sure that the 
benefit would be worth introducing the irregularity in the IL.

-- 
Eric Botcazou

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Fri, 29 Apr 2016, Eric Botcazou wrote:

> > The following works (for the testcase):
> > 
> > Index: gcc/cp/decl.c
> > ===================================================================
> > --- gcc/cp/decl.c       (revision 235547)
> > +++ gcc/cp/decl.c       (working copy)
> > @@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
> >               && (decl_context == NORMAL || decl_context == FIELD)
> >               && at_function_scope_p ()
> >               && variably_modified_type_p (type, NULL_TREE))
> > -           /* Force evaluation of the SAVE_EXPR.  */
> > -           finish_expr_stmt (TYPE_SIZE (type));
> > +           {
> > +             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> > +                                            NULL_TREE, type);
> > +             add_decl_expr (TYPE_NAME (type));
> > +           }
> > 
> >           if (declarator->kind == cdk_reference)
> >             {
> > 
> > and I have a similar fix for the Fortran FE for one testcase I
> > reduced to
> > 
> >   character(10), dimension (2) :: implicit_result
> >   character(10), dimension (2) :: source
> >   implicit_result = reallocate_hnv (LEN (source))
> > contains
> >   FUNCTION reallocate_hnv(LEN)
> >     CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
> >   END FUNCTION reallocate_hnv
> > end
> > 
> > Index: fortran/trans-array.c
> > ===================================================================
> > --- fortran/trans-array.c       (revision 235547)
> > +++ fortran/trans-array.c       (working copy)
> > @@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
> >    info->descriptor = desc;
> >    size = gfc_index_one_node;
> > 
> > +  /* Emit a DECL_EXPR for the variable sized array type in
> > +     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
> > +     sizes works correctly.  */
> > +  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
> > +  if (! TYPE_NAME (arraytype))
> > +    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> > +                                       NULL_TREE, arraytype);
> > +  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
> > +                                     arraytype, TYPE_NAME (arraytype)));
> > +
> >    /* Fill in the array dtype.  */
> >    tmp = gfc_conv_descriptor_dtype (desc);
> >    gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));
> 
> Great.  We do exactly that in the Ada compiler (but of course the number of 
> places where we need to do it is an order of magnitude larger).
> 
> > I wonder if we can avoid allocating the TYPE_DECL by simply also
> > allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
> > a 'TYPE_EXPR').
> 
> I agree that DECL_EXPR + TYPE_DECL is a bit heavy, but I'm not sure that the 
> benefit would be worth introducing the irregularity in the IL.

Not sure either.  I'll add a helper like build_decl_expr_for_type
that does the magic (if TYPE_NAME is NULL).  That at least reduces
the amount of code duplication.

Richard.