[PATCH,fortran]: partial fix for PR 32600

[PATCH,fortran]: partial fix for PR 32600

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1317 bytes --]

hi all,

the attached patch is a partial fix for PR 32600; it generates code to 
inline c_associated rather than call a library routine.  PR 32600 is an 
enhancement pr, so if it is too late into 4.3 for this patch to be 
applied, i can hold off on it until later.

there are two main parts to the patch; one is mostly in trans-expr.c and 
it generates the inlined code and the other is simply removing 
c_associated from libgfortran.  the part for the frontend does not depend 
on the changes to libgfortran, so it is compatible with existing versions 
of the library for gcc 4.3.0 if it is too late to change the library.

bootstrapped and regtested on x86 linux with no new failures.

thanks.
Chris

:ADDPATCH fortran:

ChangeLog entry:

2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>

 	PR fortran/32600
 	* trans-expr.c (gfc_conv_function_call): Generate code to inline
 	c_associated.
 	* symbol.c (get_iso_c_sym): Preserve from_intmod and intmod_sym_id
 	attributes in the resolved symbol.
 	* resolve.c (gfc_iso_c_sub_interface): Remove dead code.

2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>

 	PR fortran/32600
 	* libgfortran/intrinsics/iso_c_binding.c: Remove c_associated_1
 	and c_associated_2.
 	* libgfortran/intrinsics/iso_c_binding.h: Ditto.
 	* libgfortran/gfortran.map: Ditto.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 6948 bytes --]

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 129272)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -2110,6 +2110,52 @@ gfc_conv_function_call (gfc_se * se, gfc
       
 	  return 0;
 	}
+      else if (sym->intmod_sym_id == ISOCBINDING_ASSOCIATED)
+        {
+	  gfc_se arg1se;
+	  gfc_se arg2se;
+
+	  /* Build the addr_expr for the first argument.  The argument is
+	     already an *address* so we don't need to set want_pointer in
+	     the gfc_se.  */
+	  gfc_init_se (&arg1se, NULL);
+	  gfc_conv_expr (&arg1se, arg->expr);
+	  gfc_add_block_to_block (&se->pre, &arg1se.pre);
+	  gfc_add_block_to_block (&se->post, &arg1se.post);
+
+	  /* See if we were given two arguments.  */
+	  if (arg->next == NULL)
+	    /* Only given one arg so generate a null and do a
+	       not-equal comparison against the first arg.  */
+	    se->expr = build2 (NE_EXPR, boolean_type_node, arg1se.expr,
+			       fold_convert (TREE_TYPE (arg1se.expr),
+					     null_pointer_node));
+	  else
+	    {
+	      tree eq_expr;
+	      tree not_null_expr;
+	      
+	      /* Given two arguments so build the arg2se from second arg.  */
+	      gfc_init_se (&arg2se, NULL);
+	      gfc_conv_expr (&arg2se, arg->next->expr);
+	      gfc_add_block_to_block (&se->pre, &arg2se.pre);
+	      gfc_add_block_to_block (&se->post, &arg2se.post);
+
+	      /* Generate test to compare that the two args are equal.  */
+	      eq_expr = build2 (EQ_EXPR, boolean_type_node, arg1se.expr,
+				arg2se.expr);
+	      /* Generate test to ensure that the first arg is not null.  */
+	      not_null_expr = build2 (NE_EXPR, boolean_type_node, arg1se.expr,
+				      null_pointer_node);
+
+	      /* Finally, the generated test must check that both arg1 is not
+		 NULL and that it is equal to the second arg.  */
+	      se->expr = build2 (TRUTH_AND_EXPR, boolean_type_node,
+				 not_null_expr, eq_expr);
+	    }
+
+	  return 0;
+	}
     }
   
   if (se->ss != NULL)
Index: gcc/fortran/symbol.c
===================================================================
--- gcc/fortran/symbol.c	(revision 129272)
+++ gcc/fortran/symbol.c	(working copy)
@@ -4029,6 +4029,8 @@ get_iso_c_sym (gfc_symbol *old_sym, char
   new_symtree->n.sym->attr = old_sym->attr;
   new_symtree->n.sym->ts = old_sym->ts;
   new_symtree->n.sym->module = gfc_get_string (old_sym->module);
+  new_symtree->n.sym->from_intmod = old_sym->from_intmod;
+  new_symtree->n.sym->intmod_sym_id = old_sym->intmod_sym_id;
   /* Build the formal arg list.  */
   build_formal_args (new_symtree->n.sym, old_sym, add_optional_arg);
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 129272)
+++ gcc/fortran/resolve.c	(working copy)
@@ -2479,31 +2479,6 @@ gfc_iso_c_sub_interface (gfc_code *c, gf
 	  new_sym->declared_at = sym->declared_at;
 	}
     }
-  else if (sym->intmod_sym_id == ISOCBINDING_ASSOCIATED)
-    {
-      /* TODO: Figure out if this is even reachable; this part of the
-         conditional may not be necessary.  */
-      int num_args = 0;
-      if (c->ext.actual->next == NULL)
-	{
-	  /* The user did not give two args, so resolve to the version
-	     of c_associated expecting one arg.	 */
-	  num_args = 1;
-	  /* get rid of the second arg */
-	  /* TODO!! Should free up the memory here!  */
-	  sym->formal->next = NULL;
-	}
-      else
-	{
-	  num_args = 2;
-	}
-
-      new_sym = sym;
-      sprintf (name, "%s_%d", sym->name, num_args);
-      sprintf (binding_label, "%s_%d", sym->binding_label, num_args);
-      sym->name = gfc_get_string (name);
-      strcpy (sym->binding_label, binding_label);
-    }
   else
     {
       /* no differences for c_loc or c_funloc */
Index: libgfortran/intrinsics/iso_c_binding.c
===================================================================
--- libgfortran/intrinsics/iso_c_binding.c	(revision 129272)
+++ libgfortran/intrinsics/iso_c_binding.c	(working copy)
@@ -193,42 +193,3 @@ ISO_C_BINDING_PREFIX (c_f_procpointer) (
 }
 
 
-/* Test if the given c_ptr is associated or not.  This function is
-   called if the user only supplied one c_ptr parameter to the
-   c_associated function.  The second argument is optional, and the
-   Fortran compiler will resolve the function to this version if only
-   one arg was given.  Associated here simply means whether or not the
-   c_ptr is NULL or not.  */
-
-GFC_LOGICAL_4
-ISO_C_BINDING_PREFIX (c_associated_1) (void *c_ptr_in_1)
-{
-  if (c_ptr_in_1 != NULL)
-    return 1;
-  else
-    return 0;
-}
-
-
-/* Test if the two c_ptr arguments are associated with one another.
-   This version of the c_associated function is called if the user
-   supplied two c_ptr args in the Fortran source.  According to the
-   draft standard (J3/04-007), if c_ptr_in_1 is NULL, the two pointers
-   are NOT associated.  If c_ptr_in_1 is non-NULL and it is not equal
-   to c_ptr_in_2, then either c_ptr_in_2 is NULL or is associated with
-   another address; either way, the two pointers are not associated
-   with each other then.  */
-
-GFC_LOGICAL_4
-ISO_C_BINDING_PREFIX (c_associated_2) (void *c_ptr_in_1, void *c_ptr_in_2)
-{
-  /* Since we have the second arg, if it doesn't equal the first,
-     return false; true otherwise.  However, if the first one is null,
-     then return false; otherwise compare the two ptrs for equality.  */
-  if (c_ptr_in_1 == NULL)
-    return 0;
-  else if (c_ptr_in_1 != c_ptr_in_2)
-    return 0;
-  else
-    return 1;
-}
Index: libgfortran/intrinsics/iso_c_binding.h
===================================================================
--- libgfortran/intrinsics/iso_c_binding.h	(revision 129272)
+++ libgfortran/intrinsics/iso_c_binding.h	(working copy)
@@ -56,9 +56,6 @@ void ISO_C_BINDING_PREFIX(c_f_pointer)(v
    implemented.  */
 void ISO_C_BINDING_PREFIX(c_f_procpointer) (void *, gfc_array_void *);
 
-GFC_LOGICAL_4 ISO_C_BINDING_PREFIX(c_associated_1) (void *);
-GFC_LOGICAL_4 ISO_C_BINDING_PREFIX(c_associated_2) (void *, void *);
-
 void ISO_C_BINDING_PREFIX(c_f_pointer_u0) (void *, gfc_array_void *,
 					   const array_t *);
 void ISO_C_BINDING_PREFIX(c_f_pointer_d0) (void *, gfc_array_void *,
Index: libgfortran/gfortran.map
===================================================================
--- libgfortran/gfortran.map	(revision 129272)
+++ libgfortran/gfortran.map	(working copy)
@@ -1003,8 +1003,6 @@ GFORTRAN_1.0 {
     _gfortran_unpack0_char;
     _gfortran_unpack1;
     _gfortran_unpack1_char;
-    __iso_c_binding_c_associated_1;
-    __iso_c_binding_c_associated_2;
     __iso_c_binding_c_f_pointer;
     __iso_c_binding_c_f_pointer_d0;
     __iso_c_binding_c_f_pointer_i1;

Re: [PATCH,fortran]: partial fix for PR 32600

[Tobias Schlüter]

Hi Christopher,

Christopher D. Rickett wrote:
> 2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>
> 
>     PR fortran/32600
>     * trans-expr.c (gfc_conv_function_call): Generate code to inline
>     c_associated.
>     * symbol.c (get_iso_c_sym): Preserve from_intmod and intmod_sym_id
>     attributes in the resolved symbol.
>     * resolve.c (gfc_iso_c_sub_interface): Remove dead code.
> 
> 2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>
> 
>     PR fortran/32600
>     * libgfortran/intrinsics/iso_c_binding.c: Remove c_associated_1
>     and c_associated_2.
>     * libgfortran/intrinsics/iso_c_binding.h: Ditto.
>     * libgfortran/gfortran.map: Ditto.

I think the patch is ok, but I would appreciate if one of our 
ISO-C-bindings experts gave it a second look.

I agree that this is too late for 4.3, as the enhancement is barely 
user-visible.

Cheers,
- Tobi

Re: [PATCH,fortran]: partial fix for PR 32600

[Tobias Burnus]

Tobias Schlüter wrote:
> I think the patch is ok, but I would appreciate if one of our
> ISO-C-bindings experts gave it a second look.
I also think it is ok.


Christopher D. Rickett wrote:
> there are two main parts to the patch; one is mostly in trans-expr.c
> and it generates the inlined code and the other is simply removing
> c_associated from libgfortran.  the part for the frontend does not
> depend on the changes to libgfortran, so it is compatible with
> existing versions of the library for gcc 4.3.0 if it is too late to
> change the library.

Tobias Schlüter wrote:
> I agree that this is too late for 4.3, as the enhancement is barely
> user-visible.
I actually would like if we could still get it into 4.3.0 as this will
remove dead library code and the change is very localized and not a new
feature; however, it is strictly speaking also not a bug fix (but a
clean up/dead code removal/optimization improvement).

How is your OK meant: OK for 4.4.x (implies w/o library change for
compatible reasons), OK for 4.3.x (w/o library change) or OK for 4.3.0
w/ the library change?

Tobias

Re: [PATCH,fortran]: partial fix for PR 32600

[Steve Kargl]

On Mon, Oct 15, 2007 at 06:31:45PM +0200, Tobias Burnus wrote:
> 
> Tobias Schl?ter wrote:
> > I agree that this is too late for 4.3, as the enhancement is barely
> > user-visible.
> I actually would like if we could still get it into 4.3.0 as this will
> remove dead library code and the change is very localized and not a new
> feature; however, it is strictly speaking also not a bug fix (but a
> clean up/dead code removal/optimization improvement).

With 4.3.0 being the first release of gfortran with symbol versioning,
I think your observation of clean up and dead code removal are 
sufficient to ask markm for his opinion/approval.

-- 
Steve

Re: [PATCH,fortran]: partial fix for PR 32600 - OK for GCC 4.3.0/stage 3

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

Mark,

Stage 3: "the only changes that may be made are changes that fix bugs
[...] New functionality may not be introduced during this period."

The following patch does not introduce new functionality, but is not a
real bugfix either. It moves functionality from the library to the front
end, removes dead code, does some cleanup, improves the performance and
code size a tiny bit and the changes are very localized.

One reason we would like to have it in GCC 4.3 and not in GCC 4.4 is
that starting with GCC 4.3.0 we will have symbol versioning and can
therefore not remove the library function in 4.4.0. As the function
belongs to Fortran 2003 (ISO C Binding), it is still not widely used and
thus most GCC 4.3.0-pre-patch-compiled programs will be unaffected by
this change.

Do you think that the patch can go in GCC 4.3.0 or has it to wait for
GCC 4.4.0?

The patch itself was oked by Tobias Schlüter and me; it also survived
several bootstraps, regtests and real-world program builds.


Steve Kargl wrote:
> With 4.3.0 being the first release of gfortran with symbol versioning,
> I think your observation of clean up and dead code removal are 
> sufficient to ask markm for his opinion/approval.
>   

Christopher D. Rickett wrote:
> the attached patch is a partial fix for PR 32600; it generates code to
> inline c_associated rather than call a library routine.  PR 32600 is
> an enhancement pr, so if it is too late into 4.3 for this patch to be
> applied, i can hold off on it until later.
>
> there are two main parts to the patch; one is mostly in trans-expr.c
> and it generates the inlined code and the other is simply removing
> c_associated from libgfortran.  the part for the frontend does not
> depend on the changes to libgfortran, so it is compatible with
> existing versions of the library for gcc 4.3.0 if it is too late to
> change the library.
>
> bootstrapped and regtested on x86 linux with no new failures.
>
> thanks.
> Chris
>
> :ADDPATCH fortran:
>
> ChangeLog entry:
>
> 2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>
>
>     PR fortran/32600
>     * trans-expr.c (gfc_conv_function_call): Generate code to inline
>     c_associated.
>     * symbol.c (get_iso_c_sym): Preserve from_intmod and intmod_sym_id
>     attributes in the resolved symbol.
>     * resolve.c (gfc_iso_c_sub_interface): Remove dead code.
>
> 2007-10-12  Christopher D. Rickett  <crickett@lanl.gov>
>
>     PR fortran/32600
>     * libgfortran/intrinsics/iso_c_binding.c: Remove c_associated_1
>     and c_associated_2.
>     * libgfortran/intrinsics/iso_c_binding.h: Ditto.
>     * libgfortran/gfortran.map: Ditto.

Tobias

[-- Attachment #2: icb.diff --]
[-- Type: text/plain, Size: 6768 bytes --]

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 129272)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -2110,6 +2110,52 @@ gfc_conv_function_call (gfc_se * se, gfc
       
 	  return 0;
 	}
+      else if (sym->intmod_sym_id == ISOCBINDING_ASSOCIATED)
+        {
+	  gfc_se arg1se;
+	  gfc_se arg2se;
+
+	  /* Build the addr_expr for the first argument.  The argument is
+	     already an *address* so we don't need to set want_pointer in
+	     the gfc_se.  */
+	  gfc_init_se (&arg1se, NULL);
+	  gfc_conv_expr (&arg1se, arg->expr);
+	  gfc_add_block_to_block (&se->pre, &arg1se.pre);
+	  gfc_add_block_to_block (&se->post, &arg1se.post);
+
+	  /* See if we were given two arguments.  */
+	  if (arg->next == NULL)
+	    /* Only given one arg so generate a null and do a
+	       not-equal comparison against the first arg.  */
+	    se->expr = build2 (NE_EXPR, boolean_type_node, arg1se.expr,
+			       fold_convert (TREE_TYPE (arg1se.expr),
+					     null_pointer_node));
+	  else
+	    {
+	      tree eq_expr;
+	      tree not_null_expr;
+	      
+	      /* Given two arguments so build the arg2se from second arg.  */
+	      gfc_init_se (&arg2se, NULL);
+	      gfc_conv_expr (&arg2se, arg->next->expr);
+	      gfc_add_block_to_block (&se->pre, &arg2se.pre);
+	      gfc_add_block_to_block (&se->post, &arg2se.post);
+
+	      /* Generate test to compare that the two args are equal.  */
+	      eq_expr = build2 (EQ_EXPR, boolean_type_node, arg1se.expr,
+				arg2se.expr);
+	      /* Generate test to ensure that the first arg is not null.  */
+	      not_null_expr = build2 (NE_EXPR, boolean_type_node, arg1se.expr,
+				      null_pointer_node);
+
+	      /* Finally, the generated test must check that both arg1 is not
+		 NULL and that it is equal to the second arg.  */
+	      se->expr = build2 (TRUTH_AND_EXPR, boolean_type_node,
+				 not_null_expr, eq_expr);
+	    }
+
+	  return 0;
+	}
     }
   
   if (se->ss != NULL)
Index: gcc/fortran/symbol.c
===================================================================
--- gcc/fortran/symbol.c	(revision 129272)
+++ gcc/fortran/symbol.c	(working copy)
@@ -4029,6 +4029,8 @@ get_iso_c_sym (gfc_symbol *old_sym, char
   new_symtree->n.sym->attr = old_sym->attr;
   new_symtree->n.sym->ts = old_sym->ts;
   new_symtree->n.sym->module = gfc_get_string (old_sym->module);
+  new_symtree->n.sym->from_intmod = old_sym->from_intmod;
+  new_symtree->n.sym->intmod_sym_id = old_sym->intmod_sym_id;
   /* Build the formal arg list.  */
   build_formal_args (new_symtree->n.sym, old_sym, add_optional_arg);
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 129272)
+++ gcc/fortran/resolve.c	(working copy)
@@ -2479,31 +2479,6 @@ gfc_iso_c_sub_interface (gfc_code *c, gf
 	  new_sym->declared_at = sym->declared_at;
 	}
     }
-  else if (sym->intmod_sym_id == ISOCBINDING_ASSOCIATED)
-    {
-      /* TODO: Figure out if this is even reachable; this part of the
-         conditional may not be necessary.  */
-      int num_args = 0;
-      if (c->ext.actual->next == NULL)
-	{
-	  /* The user did not give two args, so resolve to the version
-	     of c_associated expecting one arg.	 */
-	  num_args = 1;
-	  /* get rid of the second arg */
-	  /* TODO!! Should free up the memory here!  */
-	  sym->formal->next = NULL;
-	}
-      else
-	{
-	  num_args = 2;
-	}
-
-      new_sym = sym;
-      sprintf (name, "%s_%d", sym->name, num_args);
-      sprintf (binding_label, "%s_%d", sym->binding_label, num_args);
-      sym->name = gfc_get_string (name);
-      strcpy (sym->binding_label, binding_label);
-    }
   else
     {
       /* no differences for c_loc or c_funloc */
Index: libgfortran/intrinsics/iso_c_binding.c
===================================================================
--- libgfortran/intrinsics/iso_c_binding.c	(revision 129272)
+++ libgfortran/intrinsics/iso_c_binding.c	(working copy)
@@ -193,42 +193,3 @@ ISO_C_BINDING_PREFIX (c_f_procpointer) (
 }
 
 
-/* Test if the given c_ptr is associated or not.  This function is
-   called if the user only supplied one c_ptr parameter to the
-   c_associated function.  The second argument is optional, and the
-   Fortran compiler will resolve the function to this version if only
-   one arg was given.  Associated here simply means whether or not the
-   c_ptr is NULL or not.  */
-
-GFC_LOGICAL_4
-ISO_C_BINDING_PREFIX (c_associated_1) (void *c_ptr_in_1)
-{
-  if (c_ptr_in_1 != NULL)
-    return 1;
-  else
-    return 0;
-}
-
-
-/* Test if the two c_ptr arguments are associated with one another.
-   This version of the c_associated function is called if the user
-   supplied two c_ptr args in the Fortran source.  According to the
-   draft standard (J3/04-007), if c_ptr_in_1 is NULL, the two pointers
-   are NOT associated.  If c_ptr_in_1 is non-NULL and it is not equal
-   to c_ptr_in_2, then either c_ptr_in_2 is NULL or is associated with
-   another address; either way, the two pointers are not associated
-   with each other then.  */
-
-GFC_LOGICAL_4
-ISO_C_BINDING_PREFIX (c_associated_2) (void *c_ptr_in_1, void *c_ptr_in_2)
-{
-  /* Since we have the second arg, if it doesn't equal the first,
-     return false; true otherwise.  However, if the first one is null,
-     then return false; otherwise compare the two ptrs for equality.  */
-  if (c_ptr_in_1 == NULL)
-    return 0;
-  else if (c_ptr_in_1 != c_ptr_in_2)
-    return 0;
-  else
-    return 1;
-}
Index: libgfortran/intrinsics/iso_c_binding.h
===================================================================
--- libgfortran/intrinsics/iso_c_binding.h	(revision 129272)
+++ libgfortran/intrinsics/iso_c_binding.h	(working copy)
@@ -56,9 +56,6 @@ void ISO_C_BINDING_PREFIX(c_f_pointer)(v
    implemented.  */
 void ISO_C_BINDING_PREFIX(c_f_procpointer) (void *, gfc_array_void *);
 
-GFC_LOGICAL_4 ISO_C_BINDING_PREFIX(c_associated_1) (void *);
-GFC_LOGICAL_4 ISO_C_BINDING_PREFIX(c_associated_2) (void *, void *);
-
 void ISO_C_BINDING_PREFIX(c_f_pointer_u0) (void *, gfc_array_void *,
 					   const array_t *);
 void ISO_C_BINDING_PREFIX(c_f_pointer_d0) (void *, gfc_array_void *,
Index: libgfortran/gfortran.map
===================================================================
--- libgfortran/gfortran.map	(revision 129272)
+++ libgfortran/gfortran.map	(working copy)
@@ -1003,8 +1003,6 @@ GFORTRAN_1.0 {
     _gfortran_unpack0_char;
     _gfortran_unpack1;
     _gfortran_unpack1_char;
-    __iso_c_binding_c_associated_1;
-    __iso_c_binding_c_associated_2;
     __iso_c_binding_c_f_pointer;
     __iso_c_binding_c_f_pointer_d0;
     __iso_c_binding_c_f_pointer_i1;

Re: [PATCH,fortran]: partial fix for PR 32600 - OK for GCC 4.3.0/stage 3

[Mark Mitchell]

Tobias Burnus wrote:

> One reason we would like to have it in GCC 4.3 and not in GCC 4.4 is
> that starting with GCC 4.3.0 we will have symbol versioning and can
> therefore not remove the library function in 4.4.0. As the function
> belongs to Fortran 2003 (ISO C Binding), it is still not widely used and
> thus most GCC 4.3.0-pre-patch-compiled programs will be unaffected by
> this change.
> 
> Do you think that the patch can go in GCC 4.3.0 or has it to wait for
> GCC 4.4.0?

Thank you for asking.  As these changes are completely within the
Fortran compiler and its run-time library, and as there's a good
justification, yes, this is OK.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH,fortran]: partial fix for PR 32600

[Tobias Schlüter]

Tobias Burnus wrote:
> Tobias Schlüter wrote:
>> I agree that this is too late for 4.3, as the enhancement is barely
>> user-visible.
> I actually would like if we could still get it into 4.3.0 as this will
> remove dead library code and the change is very localized and not a new
> feature; however, it is strictly speaking also not a bug fix (but a
> clean up/dead code removal/optimization improvement).
> 
> How is your OK meant: OK for 4.4.x (implies w/o library change for
> compatible reasons), OK for 4.3.x (w/o library change) or OK for 4.3.0
> w/ the library change?

That's a very good reason you're giving.  The less cruft to carry 
around, the better our lives.

- Tobi