Gross hack to fix bug

Gross hack to fix bug

[Steve Kargl]

See 

https://groups.google.com/forum/#!topic/comp.lang.fortran/8q2nYfHnZfU

for original code.  Boiling down the issue, we have defined 
assignment where a temporary for the RHS is not created.  A
reduced testcase is 

module m
   implicit none
   type :: t_a
      real, allocatable :: x
   end type t_a
   interface assignment(=)
      module procedure copy_t_a
   end interface
   contains
      subroutine copy_t_a(y,x)
         type(t_a), intent(in)  :: x
         type(t_a), intent(out) :: y
         allocate(y%x , source=x%x)
      end subroutine copy_t_a
end module m

program p
   use m
   implicit none
   type(t_a) :: v1
   allocate(v1%x)
   v1%x = 1.5
   v1 = v1
   write(*,*) "v1%x = ",v1%x
end program p
 

gfortran appears to convert 'v1 = v1' into 'call copy_t_a(v1,v1)',
which is incorrect.  The F03 standard (p. 263) mandates that 

  A defined assignment is treated as a reference to the subroutine,
  with the left-hand side as the first argument and the right-hand side
  enclosed in parentheses as the second argument.

Thus, it should translate to 'call copy_t_a(v1,(v1))' and the (v1)
is now an expression that needs to be evaluated.  So, the gross
hack I've found is (watch for cut-n-paste tab corruption)

troutmask:sgk[245] svn diff resolve.c
Index: resolve.c
===================================================================
--- resolve.c   (revision 227458)
+++ resolve.c   (working copy)
@@ -9439,10 +9439,12 @@ resolve_ordinary_assign (gfc_code *code,
 
       /* Make a temporary rhs when there is a default initializer
         and rhs is the same symbol as the lhs.  */
+#if 0
       if ((*rhsptr)->expr_type == EXPR_VARIABLE
            && (*rhsptr)->symtree->n.sym->ts.type == BT_DERIVED
            && gfc_has_default_initializer ((*rhsptr)->symtree->n.sym->ts.u.derived)
            && (lhs->symtree->n.sym == (*rhsptr)->symtree->n.sym))
+#endif
        *rhsptr = gfc_get_parentheses (*rhsptr);
 
       return true;

Anyone have a better idea?

-- 
Steve

Re: Gross hack to fix bug

[Steve Kargl]

On Wed, Sep 09, 2015 at 10:38:29AM -0700, Steve Kargl wrote:
> troutmask:sgk[245] svn diff resolve.c
> Index: resolve.c
> ===================================================================
> --- resolve.c   (revision 227458)
> +++ resolve.c   (working copy)
> @@ -9439,10 +9439,12 @@ resolve_ordinary_assign (gfc_code *code,
>  
>        /* Make a temporary rhs when there is a default initializer
>          and rhs is the same symbol as the lhs.  */
> +#if 0
>        if ((*rhsptr)->expr_type == EXPR_VARIABLE
>             && (*rhsptr)->symtree->n.sym->ts.type == BT_DERIVED
>             && gfc_has_default_initializer ((*rhsptr)->symtree->n.sym->ts.u.derived)
>             && (lhs->symtree->n.sym == (*rhsptr)->symtree->n.sym))
> +#endif
>         *rhsptr = gfc_get_parentheses (*rhsptr);
>  
>        return true;
> 
> Anyone have a better idea?
> 

Yep, it was a gross hack, which led to testsuite regressions.
This one is better.

troutmask:sgk[246] svn diff resolve.c 
Index: resolve.c
===================================================================
--- resolve.c   (revision 227600)
+++ resolve.c   (working copy)
@@ -9421,6 +9421,7 @@ resolve_ordinary_assign (gfc_code *code,
        {
          lhs = code->ext.actual->expr;
          rhsptr = &code->ext.actual->next->expr;
+         *rhsptr = gfc_get_parentheses (*rhsptr);
        }
       else
        {

-- 
Steve

Re: Gross hack to fix bug

[FX]

> Yep, it was a gross hack, which led to testsuite regressions.
> This one is better.

Two questions: 1. could you maybe add a comment saying why we add parentheses, 2. doesn’t that lead to performance regressions (temporary creations) in some cases?

Thanks,
FX

Re: Gross hack to fix bug

[Steve Kargl]

On Wed, Sep 09, 2015 at 10:44:16PM +0200, FX wrote:
> > Yep, it was a gross hack, which led to testsuite regressions.
> > This one is better.
> 
> Two questions: 1. could you maybe add a comment saying why we
> add parentheses, 2. doesn???t that lead to performance regressions
> (temporary creations) in some cases?
> 

For 1, yes I'll add a comment.  The F03 standard is quite
explicit:

  A defined assignment is treated as a reference to the subroutine,
  with the left-hand side as the first argument and the right-hand side
  enclosed in parentheses as the second argument.

For 2, I'm still trying to determine if the EXEC_ASSIGN_CALL
branch in resolve_ordinary_assign() is only called for defined
assignment.  If the answer is "yes" only defined assignments
go through this code path, then there is no performance 
regression in that the patch does what the standard requires.
If the answer is "no", then whatever other thing goes through
this code path may suffer a performance hit.

There is the caveat that transforming 'v1 = v1' into
'call foo(v1, (v1))' means that the 2nd argument is an
expression and it is evaluated at entry into the subroutine.
I suspect that the Standard does not require the creation of
a temporary as a result of that evaluation.  That is, a
compiler can evaluate (v1) and come to the conclusion that
(v1) is v1.

-- 
Steve