[Bug fortran/41113] New: spurious _gfortran_internal_pack

[Bug fortran/41113] New: spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

gfortran seems to generate a spurious  _gfortran_internal_pack if an array that
is component of a type is passed to a subroutine. More precisely, in the
example
 CALL S1(d%data)
 CALL S1(data)
should basically generate the same pseudocode, but this doesn't happen.


> cat test.f90
MODULE M1
 TYPE T1
   REAL :: data(10)
 END TYPE T1
CONTAINS
 SUBROUTINE S1(data)
   REAL, DIMENSION(*) :: data
 END SUBROUTINE S1
END MODULE

SUBROUTINE S2
 USE M1
 TYPE(T1) :: d
 REAL :: data(10)
 CALL S1(d%data)
 CALL S1(data)
END SUBROUTINE S2

> cat test.f90.003t.original
s1 (real(kind=4)[0:] * data)
{
  (void) 0;
}


s2 ()
{
  struct t1 d;
  real(kind=4) data[10];

  {
    void * D.1554;
    struct array1_real(kind=4) parm.0;

    parm.0.dtype = 281;
    parm.0.dim[0].lbound = 1;
    parm.0.dim[0].ubound = 10;
    parm.0.dim[0].stride = 1;
    parm.0.data = (void *) &d.data[0];
    parm.0.offset = -1;
    D.1554 = _gfortran_internal_pack (&parm.0);
    s1 (D.1554);
    if ((real(kind=4)[0:] *) parm.0.data != (real(kind=4)[0:] *) D.1554)
      {
        _gfortran_internal_unpack (&parm.0, D.1554);
        {
          void * D.1555;

          D.1555 = D.1554;
          if (D.1555 != 0B)
            {
              __builtin_free (D.1555);
            }
        }
      }
  }
  s1 (&data);
}


-- 
           Summary: spurious  _gfortran_internal_pack
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jv244 at cam dot ac dot uk


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #1 from jv244 at cam dot ac dot uk  2009-08-19 08:58 -------
an ugly workaround is to write instead of

CALL S1(d%data)

the following

CALL S1(d%data(1))

which works in simple cases


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[burnus at gcc dot gnu dot org]

------- Comment #2 from burnus at gcc dot gnu dot org  2009-08-19 13:30 -------
Created an attachment (id=18401)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18401&action=view)
Draft patch - first steps but incomplete & will not work

The problem is:

   CALL S1(d%data)

We have the variable "d" which contains first the component reference "data"
and then the array reference whole-array. The current check in trans-array.c's
gfc_conv_array_parameter is, however:

  full_array_var = (expr->expr_type == EXPR_VARIABLE
                    && expr->ref->type == REF_ARRAY
                    && expr->ref->u.ar.type == AR_FULL);

which returns "false". It gets quite complicated if we want to handle:
   foo(1)%bar(1:1)%variable(:)(sub:string)

Attached patch does the first steps, but it needs some improvements to handle 
"sym" and "comp" correctly. "sym->" appears all over the place in the rest of
the function and it needs to be properly handled.  (NOTE: The attached patch
will not work!)


Cross reference:

a) Related PR 41117. Here, one has the same problem as for (1:1) or (j:j) one
does not have an element or a whole array but a known-to-be contiguous section.
 (j:j) is a challenge but one should be able to handle it, (1*i,i) would be
more complicated). In the PR one has (:,1:1), i.e. whole-section access on the
left followed by an array-element or size-one array-section reference on the
right.
Caveat: For (1:1), whole array and (:,1:1) - that does not work for
assumed-shape arrays - unless the dummy is allocatable. Otherwise they are not
contiguous.

b) PR 36933. At least some of the examples are identical to the one in comment
0.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #3 from jv244 at cam dot ac dot uk  2009-08-21 06:09 -------
(In reply to comment #2)
> which returns "false". It gets quite complicated if we want to handle:
>    foo(1)%bar(1:1)%variable(:)(sub:string)

AFAICT this is already handled fine:
write(6,*) foo(1)%bar(1:1)%variable(:)(2:4)
         1
Error: Two or more part references with nonzero rank must not be specified at
(1)

> Attached patch does the first steps, but it needs some improvements to handle 
> "sym" and "comp" correctly. "sym->" appears all over the place in the rest of
> the function and it needs to be properly handled.  (NOTE: The attached patch
> will not work!)

I believe that some function which can find if an expression is really  a
contiguous array in memory is pretty useful. In the end, many optimization
(also vectorization) relies on that.

I have another example, I will file it as a different PR, but make a
'cross-link'


-- 

jv244 at cam dot ac dot uk changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #4 from jv244 at cam dot ac dot uk  2009-08-21 06:15 -------
(In reply to comment #3)
> I have another example, I will file it as a different PR, but make a
> 'cross-link'

PR 41137



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #5 from pault at gcc dot gnu dot org  2009-12-20 18:47 -------
Confirmed

I will post a somewhat simpler patch, once it has regtested.... if it does :-)

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-12-20 18:47:09
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #6 from pault at gcc dot gnu dot org  2009-12-20 21:58 -------
Created an attachment (id=19355)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19355&action=view)
A fix for the PR

The attached bootstraps and regtests.  Will fix PR41117 at the same time.

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |pault at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #7 from jv244 at cam dot ac dot uk  2010-02-06 16:40 -------
(In reply to comment #6)
> Created an attachment (id=19355)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19355&action=view) [edit]
> A fix for the PR
> 
> The attached bootstraps and regtests.  Will fix PR41117 at the same time.
> 
> Paul

Looks like the commit for this one is still pending?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2010-02-06 16:47 -------
Happens a lot in 465.tonto btw.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #9 from jv244 at cam dot ac dot uk  2010-02-08 08:09 -------
(In reply to comment #8)
> Happens a lot in 465.tonto btw.

I had tested Paul's patch on CP2K, where it reduces the calls to
gfortran_internal[un]pack from 4252 to 1276. I think it addresses an issue that
is quite important in codes that are F95-ish in style.

For reference, this was Paul's message to the list:

http://gcc.gnu.org/ml/fortran/2009-12/msg00164.html


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #10 from pault at gcc dot gnu dot org  2010-02-08 14:06 -------
(In reply to comment #9)

> For reference, this was Paul's message to the list:
> 
> http://gcc.gnu.org/ml/fortran/2009-12/msg00164.html
> 

Dear Joost,

This was followed by http://gcc.gnu.org/ml/fortran/2009-12/msg00166.html

I have not returned to this PR to fix the problem that Dominique found because
it is not either a regression nor one of our "serious" bugs.

I would like to commit this patch, so I will try to fix it and then to persuade
the other maintainers to accept it now.

Cheers

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

------- Comment #11 from jv244 at cam dot ac dot uk  2010-02-08 14:38 -------
(In reply to comment #10)
> This was followed by http://gcc.gnu.org/ml/fortran/2009-12/msg00166.html

I have just retested your patch on a clean tree to Dominique's testcase, but I
don't get any segfault, and also valgrind finds nothing to report. Obviously
that doesn't mean that the patch is right, but maybe Dominique can retest on
his machine with a clean tree (or if this is only triggered by specific options
let us know)? 


-- 

jv244 at cam dot ac dot uk changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dominiq at lps dot ens dot
                   |                            |fr


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #12 from pault at gcc dot gnu dot org  2010-02-08 17:25 -------
(In reply to comment #11)
> (In reply to comment #10)
> > This was followed by http://gcc.gnu.org/ml/fortran/2009-12/msg00166.html
> 
> I have just retested your patch on a clean tree to Dominique's testcase, but I
> don't get any segfault, and also valgrind finds nothing to report. Obviously
> that doesn't mean that the patch is right, but maybe Dominique can retest on
> his machine with a clean tree (or if this is only triggered by specific options
> let us know)? 
> 

Joost,

The same here - I just get an undefined reference to 'd_fix_coo_impl_'

What are we doing wrong, Dominique?

Cheers

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #13 from dominiq at lps dot ens dot fr  2010-02-08 20:49 -------
I have applied the patch to a clean trunk at revision 156605 and the test
compiles (further tests pending). I have also applied the patch to fortran-dev
at revision 156573 and the compilation gives a segmentation fault:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000004
0x00000001000b5332 in gfc_conv_array_parameter (se=0x7fff5fbfe240,
expr=0x1418a9890, ss=<value temporarily unavailable, due to optimizations>,
g77=<value temporarily unavailable, due to optimizations>, fsym=0x0,
proc_name=0x141df53c0 "d_csgetrow", size=0x0) at
../../for_work/gcc/fortran/trans-array.c:5520
5520          if (!sym->attr.pointer && sym->as->type != AS_ASSUMED_SHAPE 
(gdb) bt
#0  0x00000001000b5332 in gfc_conv_array_parameter (se=0x7fff5fbfe240,
expr=0x1418a9890, ss=<value temporarily unavailable, due to optimizations>,
g77=<value temporarily unavailable, due to optimizations>, fsym=0x0,
proc_name=0x141df53c0 "d_csgetrow", size=0x0) at
../../for_work/gcc/fortran/trans-array.c:5520
#1  0x00000001000c7cf2 in gfc_conv_procedure_call (se=0x7fff5fbfe910,
sym=0x14187f7d0, arg=0x1418a52d0, expr=0x1418a93f0, append_args=0x0) at
../../for_work/gcc/fortran/trans-expr.c:2919
#2  0x00000001000e9c7c in gfc_trans_call (code=0x1418aafd0, dependency_check=0
'\0', mask=0x0, count1=0x0, invert=0 '\0') at
../../for_work/gcc/fortran/trans-stmt.c:379
#3  0x00000001000a6c57 in gfc_trans_code (code=0x1418aafd0) at
../../for_work/gcc/fortran/trans.c:1143
#4  0x00000001000c3027 in gfc_generate_function_code (ns=<value temporarily
unavailable, due to optimizations>) at
../../for_work/gcc/fortran/trans-decl.c:4382
#5  0x00000001000a6fdb in gfc_generate_module_code (ns=<value temporarily
unavailable, due to optimizations>) at ../../for_work/gcc/fortran/trans.c:1366
#6  0x0000000100068f7f in gfc_parse_file () at
../../for_work/gcc/fortran/parse.c:4228
#7  0x00000001000a1dbc in gfc_be_parse_file (set_yydebug=<value temporarily
unavailable, due to optimizations>) at
../../for_work/gcc/fortran/f95-lang.c:239
#8  0x00000001006d192a in toplev_main (argc=2, argv=0x7fff5fbfed20) at
../../for_work/gcc/toplev.c:1053
#9  0x0000000100000b84 in start ()

So if the patch is applied to trunk, merging trunk to fortran-dev will give a
regression (cc Jerry) on fortran-dev. I'll attach the tests for a convenient
record.

Note that I am only reporting what I see. I don't have any strong opinion on
how to proceed.


-- 

dominiq at lps dot ens dot fr changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jvdelisle at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #15 from dominiq at lps dot ens dot fr  2010-02-08 20:53 -------
Created an attachment (id=19824)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19824&action=view)
Second  test giving a segmentation fault with the patch applied to fortran-dev


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #14 from dominiq at lps dot ens dot fr  2010-02-08 20:52 -------
Created an attachment (id=19823)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19823&action=view)
First test giving a segmentation fault with the patch applied to fortran-dev


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[burnus at gcc dot gnu dot org]

------- Comment #16 from burnus at gcc dot gnu dot org  2010-02-10 09:17 -------
(In reply to comment #10)
> > For reference, this was Paul's message to the list:
> > http://gcc.gnu.org/ml/fortran/2009-12/msg00164.html
> This was followed by http://gcc.gnu.org/ml/fortran/2009-12/msg00166.html

It seems as if this only occurs on a branch (fortran-dev, see comment 13) thus
I would suggest to go ahead and fix it for the trunk. And worry about the
branch later.

> I would like to commit this patch, so I will try to fix it

Could you also look into:
  pointer%array_component
where the ultimate component ("array_component") is a whole array reference and
_not_ a pointer? In that case one should also have a contiguous array. Cf. PR
36933 and PR 36932.

Note: Your patch seems to also fix PR 41117.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #17 from pault at gcc dot gnu dot org  2010-02-13 12:43 -------
Subject: Bug 41113

Author: pault
Date: Sat Feb 13 12:42:39 2010
New Revision: 156749

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156749
Log:
2010-02-13  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/41113
        PR fortran/41117
        * trans-array.c (gfc_conv_array_parameter): Use
        gfc_full_array_ref_p to detect full and contiguous variable
        arrays. Full array components and contiguous arrays do not need
        internal_pack and internal_unpack.

2010-02-13  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/41113
        PR fortran/41117
        * gfortran.dg/internal_pack_6.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/internal_pack_6.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #18 from pault at gcc dot gnu dot org  2010-02-14 19:25 -------
(In reply to comment #15)
> Created an attachment (id=19824)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19824&action=view) [edit]
> Second  test giving a segmentation fault with the patch applied to fortran-dev
> 

At line 5520:

      if (!sym->attr.pointer
            && sym->as
            && sym->as->type != AS_ASSUMED_SHAPE 
            && !sym->attr.allocatable)

where the test for sym->as has been added, does the job.

I have included this in the fix to PR39632/3, so that the merge goes OK.

Cheers

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #19 from dominiq at lps dot ens dot fr  2010-02-14 19:58 -------
(In reply to comment #18)
> ... where the test for sym->as has been added, does the job.

It does not fix the problem in my tree, I'll try the branch. 

> I have included this in the fix to PR39632/3, so that the merge goes OK.

Probably pr36932/3;-)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #20 from dominiq at lps dot ens dot fr  2010-02-14 20:06 -------
(In reply to comment #18)
> ... where the test for sym->as has been added, does the job.

This works for a clean fortran-dev+ patch in comment #6. Note that my tree
includes patch for pr36932/3.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[dominiq at lps dot ens dot fr]

------- Comment #21 from dominiq at lps dot ens dot fr  2010-02-14 20:16 -------
The ICEs are fixed by the last change in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36932#c8 :

@@ -5548,7 +5550,8 @@ gfc_conv_array_parameter (gfc_se * se, g
     }

   if (contiguous && g77 && !this_array_result
-       && !expr->symtree->n.sym->attr.dummy)
+        && expr->symtree->n.sym->as
+       && expr->symtree->n.sym->as->type != AS_ASSUMED_SHAPE)
     {
       gfc_conv_expr_descriptor (se, expr, ss);
       if (expr->ts.type == BT_CHARACTER)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[burnus at gcc dot gnu dot org]

------- Comment #22 from burnus at gcc dot gnu dot org  2010-02-20 21:51 -------
Can this PR be closed as FIXED?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[paul dot richard dot thomas at gmail dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 751 bytes --]



------- Comment #23 from paul dot richard dot thomas at gmail dot com  2010-02-21 10:43 -------
Subject: Re:  spurious _gfortran_internal_pack

I was going to check out the situation for 4.4.  However, my
inclination is to close it.  I'll be working on it tonight.

Cheers

Paul

On Sat, Feb 20, 2010 at 10:51 PM, burnus at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #22 from burnus at gcc dot gnu dot org  2010-02-20 21:51 -------
> Can this PR be closed as FIXED?
>
>
> --
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113
>
> ------- You are receiving this mail because: -------
> You are the assignee for the bug, or are watching the assignee.
>


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[jv244 at cam dot ac dot uk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 934 bytes --]



------- Comment #24 from jv244 at cam dot ac dot uk  2010-02-21 14:16 -------
(In reply to comment #23)
> Subject: Re:  spurious _gfortran_internal_pack
> 
> I was going to check out the situation for 4.4.  However, my
> inclination is to close it.  I'll be working on it tonight.

this is not stuff to backport to a branch like that. I would just close it. In
that case I'll leave it to you to close.

> 
> Cheers
> 
> Paul
> 
> On Sat, Feb 20, 2010 at 10:51 PM, burnus at gcc dot gnu dot org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> >
> > ------- Comment #22 from burnus at gcc dot gnu dot org  2010-02-20 21:51 -------
> > Can this PR be closed as FIXED?
> >
> >
> > --
> >
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113
> >
> > ------- You are receiving this mail because: -------
> > You are the assignee for the bug, or are watching the assignee.
> >
> 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113

[Bug fortran/41113] spurious _gfortran_internal_pack

[pault at gcc dot gnu dot org]

------- Comment #25 from pault at gcc dot gnu dot org  2010-02-22 05:45 -------
Fixed on trunk.  Thanks for reportimg the problems and all the help, Tobias and
Joost.

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41113