public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug fortran/42958]  New: Weird temporary array allocation
@ 2010-02-04 16:56 rguenth at gcc dot gnu dot org
  2010-02-04 19:33 ` [Bug fortran/42958] " burnus at gcc dot gnu dot org
                   ` (23 more replies)
  0 siblings, 24 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-02-04 16:56 UTC (permalink / raw)
  To: gcc-bugs

I see

                            D.1631 = izz.dim[0].lbound;
                            D.1632 = izz.dim[0].ubound;
                            D.1643 = D.1632 - D.1631;
                            D.1647 = D.1643 < 0;
                            D.1648 = D.1643 + 1;
                            D.1649 = D.1647 ? 0 : D.1648 * 8;
                            if (D.1649 < 0)
                              {
                                _gfortran_runtime_error (&"Attempt to allocate
a negative amount of memory."[1]{lb: 1 sz: 1});
                              }
                            D.1650 = (void * restrict) __builtin_malloc
(MAX_EXPR <D.1649, 1>);
                            if (D.1650 == 0B)
                              {
                                _gfortran_os_error (&"Memory allocation
failed"[1]{lb: 1 sz: 1});
                              }
                            D.1651 = D.1650;
                            atmp.5.data = D.1651;

so, we check if the array-size is empty, ubound - lbound < 0.  In that
case we set size to zero.  Otherwise we compute it as (ubound - lbound + 1) *
8.
Then we check whether size is negative and error out.
Then we actually allocate max(1,size).

Why do we at all check for this "negative" amount allocation?  Are you
trying to detect overflow here?  (which won't work, you do the
computation with signed arithmetic so VRP will screw you anyway)

Why not simply do

  size = (ubound - lbound + 1) * 8;
  malloc (size);

which surely fails very quickly on you for negative size and the allocation
fails.

?

Surely for compiler-generated temporary allocations nothing more fancy
should be required (even the check of the allocation could be removed
when optimizing).


-- 
           Summary: Weird temporary array allocation
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: rguenth at gcc dot gnu dot org


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
@ 2010-02-04 19:33 ` burnus at gcc dot gnu dot org
  2010-02-05  5:36 ` pault at gcc dot gnu dot org
                   ` (22 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-02-04 19:33 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #1 from burnus at gcc dot gnu dot org  2010-02-04 19:32 -------
> so, we check if the array-size is empty, ubound - lbound < 0.  In that
> case we set size to zero.  Otherwise we compute it
> as (ubound - lbound + 1) *8.
> Then we check whether size is negative and error out.
> Then we actually allocate max(1,size).

The reason for the setting it to zero is that Fortran allows one to allocate a
zero-sized array:  ALLOCATE ( Array(0:-2) )

The reason for  MAX(1, size)  is to make  ALLOCATE(A(1:0)); if(ALLOCATED(A))
work. (This was added later than "size < 0 ? 0 : size".)

Why there is a negative check? Well, I do not know. I also can speculate about
a poor man's overflow check, which sometimes indeed works, but often fails.

 * * *

> Why not simply do
>   size = (ubound - lbound + 1) * 8;
>   malloc (size);

I think that would be the future: With the array descriptor (dope vector)
reform, we will have an "allocated" field and thus one can do:

   descriptor.allocated = true
   descriptor.data = malloc (max (0,size))

Where "if(allocated(A))" translates into "if(A.allocated)".

Actually, maybe one should better use:
   size = (ubound - lbound + 1)
   if (size > 0)
     {
       descriptor.data = malloc(size)
       if (descriptor.data == NULL)
         error ("allocate failed");
     }
   descriptor.allocated = true

The "allocated" member of the descriptor is also needed in order to make the
following work properly:
   integer, target :: int
   integer, pointer :: ptr
   ptr => int
   deallocate(ptr, stat=i) ! shall return "i != 0" but not crash

Thus, from my side: The negative check should be removed and the simplified
version should be applied after the descriptor update. Before the descriptor
update (planned for 4.6, breaks ABI) one can use:
   size = (ubound - lbound + 1) * 8;
   malloc (max(1,size));


Paul, what do you think?


(PS: POSIX Allows "ptr = malloc(0); free(ptr)", where "malloc(0)" returns
either NULL or a unique pointer.)


-- 

burnus at gcc dot gnu dot org changed:

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


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
  2010-02-04 19:33 ` [Bug fortran/42958] " burnus at gcc dot gnu dot org
@ 2010-02-05  5:36 ` pault at gcc dot gnu dot org
  2010-02-05  9:32 ` rguenther at suse dot de
                   ` (21 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: pault at gcc dot gnu dot org @ 2010-02-05  5:36 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #2 from pault at gcc dot gnu dot org  2010-02-05 05:36 -------
(In reply to comment #1)

> Why there is a negative check? Well, I do not know. I also can speculate about
> a poor man's overflow check, which sometimes indeed works, but often fails.

I suspect that you are being generous and that this is rather a sin of omission
rather than commission.


> Paul, what do you think?

I think that your arguments are correct.

> 
> (PS: POSIX Allows "ptr = malloc(0); free(ptr)", where "malloc(0)" returns
> either NULL or a unique pointer.)

Indeed.

Paul


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
  2010-02-04 19:33 ` [Bug fortran/42958] " burnus at gcc dot gnu dot org
  2010-02-05  5:36 ` pault at gcc dot gnu dot org
@ 2010-02-05  9:32 ` rguenther at suse dot de
  2010-02-05 14:24 ` rguenth at gcc dot gnu dot org
                   ` (20 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenther at suse dot de @ 2010-02-05  9:32 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #3 from rguenther at suse dot de  2010-02-05 09:32 -------
Subject: Re:  Weird temporary array allocation

On Fri, 5 Feb 2010, pault at gcc dot gnu dot org wrote:

> ------- Comment #2 from pault at gcc dot gnu dot org  2010-02-05 05:36 -------
> (In reply to comment #1)
> 
> > Why there is a negative check? Well, I do not know. I also can speculate about
> > a poor man's overflow check, which sometimes indeed works, but often fails.
> 
> I suspect that you are being generous and that this is rather a sin of omission
> rather than commission.
> 
> 
> > Paul, what do you think?
> 
> I think that your arguments are correct.
> 
> > 
> > (PS: POSIX Allows "ptr = malloc(0); free(ptr)", where "malloc(0)" returns
> > either NULL or a unique pointer.)
> 
> Indeed.

Btw, should there be the same error reporting or if (allocated) behavior
on Frontend-generated temporaries?  I see this from the temporaries
generated by the scalarizer and the introduced control-flow makes it
very hard to remove unnecessary temporaries in the middle-end later.

Thx,
Richard.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (2 preceding siblings ...)
  2010-02-05  9:32 ` rguenther at suse dot de
@ 2010-02-05 14:24 ` rguenth at gcc dot gnu dot org
  2010-02-17 20:17 ` jvdelisle at gcc dot gnu dot org
                   ` (19 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-02-05 14:24 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #4 from rguenth at gcc dot gnu dot org  2010-02-05 14:23 -------
(In reply to comment #3)
> Subject: Re:  Weird temporary array allocation
> 
> On Fri, 5 Feb 2010, pault at gcc dot gnu dot org wrote:
> 
> > ------- Comment #2 from pault at gcc dot gnu dot org  2010-02-05 05:36 -------
> > (In reply to comment #1)
> > 
> > > Why there is a negative check? Well, I do not know. I also can speculate about
> > > a poor man's overflow check, which sometimes indeed works, but often fails.
> > 
> > I suspect that you are being generous and that this is rather a sin of omission
> > rather than commission.
> > 
> > 
> > > Paul, what do you think?
> > 
> > I think that your arguments are correct.
> > 
> > > 
> > > (PS: POSIX Allows "ptr = malloc(0); free(ptr)", where "malloc(0)" returns
> > > either NULL or a unique pointer.)
> > 
> > Indeed.
> 
> Btw, should there be the same error reporting or if (allocated) behavior
> on Frontend-generated temporaries?  I see this from the temporaries
> generated by the scalarizer and the introduced control-flow makes it
> very hard to remove unnecessary temporaries in the middle-end later.

Thus, basically adding an argument to gfc_call_malloc () specifying
whether we want to do checking at all and shortcut it from at least
gfc_trans_allocate_array_storage ().


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (3 preceding siblings ...)
  2010-02-05 14:24 ` rguenth at gcc dot gnu dot org
@ 2010-02-17 20:17 ` jvdelisle at gcc dot gnu dot org
  2010-02-20  8:31 ` burnus at gcc dot gnu dot org
                   ` (18 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: jvdelisle at gcc dot gnu dot org @ 2010-02-17 20:17 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #5 from jvdelisle at gcc dot gnu dot org  2010-02-17 20:17 -------
Tobias, reply to your RFC on fortran list.  I think the negative check should
go away.  Then I think we should consider an option of -fcheck-mem-alloc which
we can then make more elaborate and do numerouse useful checks.  Default would
be -fno-check-mem-alloc. (sorry for posting here, but I have no email access
here)


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (4 preceding siblings ...)
  2010-02-17 20:17 ` jvdelisle at gcc dot gnu dot org
@ 2010-02-20  8:31 ` burnus at gcc dot gnu dot org
  2010-03-27 18:55 ` pault at gcc dot gnu dot org
                   ` (17 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-02-20  8:31 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #6 from burnus at gcc dot gnu dot org  2010-02-20 08:31 -------
Subject: Bug 42958

Author: burnus
Date: Sat Feb 20 08:31:25 2010
New Revision: 156923

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156923
Log:
2010-02-20  Tobias Burnus  <burnus@net-b.de>

        PR fortran/42958
        * libgfortran.h: Add GFC_RTCHECK_MEM.
        * invoke.texi (-fcheck=): Document -fcheck=mem.
        * tranc.c (gfc_call_malloc): Remove negative-size run-time error
        and enable malloc-success check only with -fcheck=mem.
        * option.c (gfc_handle_runtime_check_option): Add -fcheck=mem.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/invoke.texi
    trunk/gcc/fortran/libgfortran.h
    trunk/gcc/fortran/options.c
    trunk/gcc/fortran/trans.c


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (5 preceding siblings ...)
  2010-02-20  8:31 ` burnus at gcc dot gnu dot org
@ 2010-03-27 18:55 ` pault at gcc dot gnu dot org
  2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
                   ` (16 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: pault at gcc dot gnu dot org @ 2010-03-27 18:55 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #7 from pault at gcc dot gnu dot org  2010-03-27 18:55 -------
Tobias,

Can this be closed now?

Certainly, it can be confirmed!

Cheers

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         |2010-03-27 18:55:47
               date|                            |


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (6 preceding siblings ...)
  2010-03-27 18:55 ` pault at gcc dot gnu dot org
@ 2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
  2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
                   ` (15 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-03-27 19:06 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #8 from rguenth at gcc dot gnu dot org  2010-03-27 19:05 -------
I suppose I should have provided a testcase.  The FE now produces control-flow
free allocation like:

                            D.1612 = D.1601 - D.1600;
                            atmp.5.dtype = 537;
                            atmp.5.dim[0].stride = 1;
                            atmp.5.dim[0].lbound = 0;
                            atmp.5.dim[0].ubound = D.1612;
                            D.1616 = D.1612 < 0;
                            D.1617 = D.1612 + 1;
                            D.1618 = D.1616 ? 0 : D.1617 * 8;
                            D.1619 = (void * restrict) __builtin_malloc
(MAX_EXPR <D.1618, 1>);
                            D.1620 = D.1619;
                            atmp.5.data = D.1620;
                            atmp.5.offset = 0;

but there's still the conditional on negative D.1612.  Also deallocation is

                              D.1621 = (void *) atmp.5.data;
                              if (D.1621 != 0B)
                                {
                                  __builtin_free (D.1621);
                                }

where the check for NULL is not necessary (though the middle end might
be able to remove that condition).


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (7 preceding siblings ...)
  2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
@ 2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
  2010-03-27 19:08 ` rguenth at gcc dot gnu dot org
                   ` (14 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-03-27 19:06 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #9 from rguenth at gcc dot gnu dot org  2010-03-27 19:06 -------
Created an attachment (id=20225)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20225&action=view)
testcase from tonto


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (8 preceding siblings ...)
  2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
@ 2010-03-27 19:08 ` rguenth at gcc dot gnu dot org
  2010-03-27 19:12 ` rguenth at gcc dot gnu dot org
                   ` (13 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-03-27 19:08 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #10 from rguenth at gcc dot gnu dot org  2010-03-27 19:08 -------
The gimplifier of course transforms the COND_EXPRs back to control flow, thus
the CFG looks like

  D.1616 = D.1612 < 0;
  D.1617 = D.1612 + 1;
  if (D.1616 != 0)
    goto <bb 32>;
  else
    goto <bb 33>;

<bb 32>:
  iftmp.17 = 0;
  goto <bb 34>;

<bb 33>:
  iftmp.17 = D.1617 * 8;

<bb 34>:
  D.1618 = iftmp.17;
  D.1795 = MAX_EXPR <D.1618, 1>;
  D.1619 = __builtin_malloc (D.1795);

the question is if the check for negative size is really necessary for
compiler-generated allocations of array temporaries.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (9 preceding siblings ...)
  2010-03-27 19:08 ` rguenth at gcc dot gnu dot org
@ 2010-03-27 19:12 ` rguenth at gcc dot gnu dot org
  2010-03-28 12:57 ` burnus at gcc dot gnu dot org
                   ` (12 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-03-27 19:12 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #11 from rguenth at gcc dot gnu dot org  2010-03-27 19:12 -------
The middle-end ends up with the following optimized:

  D.1776_95 = D.1775_94 - D.1591_90;
  if (D.1776_95 <= 0)
    goto <bb 34>;
  else
    goto <bb 33>;

<bb 33>:
  D.1620_204 = __builtin_malloc (D.1795_11);
  D.1796_281 = iy.dim[1].stride;
...
  goto <bb 35>;

<bb 34>:
  D.1620_135 = __builtin_malloc (1);
  goto <bb 38>;

<bb 35>:
   the loop code

<bb 38>:
Invalid sum of incoming frequencies 1062, should be 900
  # D.1620_374 = PHI <D.1620_204(37), D.1620_135(34)>
  if (D.1620_374 != 0B)
    goto <bb 39>;
  else
    goto <bb 40>;

<bb 39>:
  __builtin_free (D.1620_374);


so we are able to prove that if we end up allocating 1 byte only then
the loop isn't executed and we just free it (another missed middle-end
optimization, p = malloc(1); free (p); isn't optimized away).


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (10 preceding siblings ...)
  2010-03-27 19:12 ` rguenth at gcc dot gnu dot org
@ 2010-03-28 12:57 ` burnus at gcc dot gnu dot org
  2010-03-28 13:05 ` burnus at gcc dot gnu dot org
                   ` (11 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-03-28 12:57 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #12 from burnus at gcc dot gnu dot org  2010-03-28 12:57 -------
(In reply to comment #8)
>                             atmp.5.dim[0].lbound = 0;
>                             atmp.5.dim[0].ubound = D.1612;
>                             D.1616 = D.1612 < 0;
>                             D.1617 = D.1612 + 1;
>                             D.1618 = D.1616 ? 0 : D.1617 * 8;
>
> but there's still the conditional on negative D.1612.

I think that check is needed: If one has:
   array(1:0) or array(1:-200) or array(1:100:-1)
that means that one has a zero-sized array. If one had no such check one would
allocate "negative memory" which matches - as the malloc argument is unsigned -
a large positive number. Maybe there are cases where one knows before hand that
the array cannot be zero-sized, but in the general case one cannot.

(I am thinking of
   call f(array(...))
where one knows that "f" does not allow for a zero-sized array argument. In the
other known cases, the upper bound should be already known at compile time and
the check should be folded away. Maybe there are more such cases.)


> Also deallocation is
>                               D.1621 = (void *) atmp.5.data;
>                               if (D.1621 != 0B)
>                                  __builtin_free (D.1621);
> 
> where the check for NULL is not necessary (though the middle end might
> be able to remove that condition).

I think one can remove the check as __builtin_free also can deal with NULL
arguments, but it is not known before hand whether the argument is NULL as - in
the general case - one can manually deallocate arrays.

Well, for generated temporary arrays (which the user cannot directly access),
one can know whether the pointer is NULL and thus one could remove the check.
Such a patch should be rather simple.

> another missed middle-end optimization,
> p = malloc(1); free (p); isn't optimized away

I think it would be useful, if the middle end were able to do so.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (11 preceding siblings ...)
  2010-03-28 12:57 ` burnus at gcc dot gnu dot org
@ 2010-03-28 13:05 ` burnus at gcc dot gnu dot org
  2010-03-28 13:57 ` burnus at gcc dot gnu dot org
                   ` (10 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-03-28 13:05 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #13 from burnus at gcc dot gnu dot org  2010-03-28 13:05 -------
(In reply to comment #12)
> (I am thinking of
>    call f(array(...))
> where one knows that "f" does not allow for a zero-sized array argument.

s/where/if/ - in the general case one does not know. Especially for
  dummyArg(n) and dummyArg(*) [and if no interface it known]
one does not know it, while for
  dummyArg(4)
one does. I fear, that this is a rare case.

For "call foo(Ix)" in the Tonto example of comment 9, one does not know as the
explicit interface interface of "foo" is not known.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (12 preceding siblings ...)
  2010-03-28 13:05 ` burnus at gcc dot gnu dot org
@ 2010-03-28 13:57 ` burnus at gcc dot gnu dot org
  2010-03-28 14:11 ` burnus at gcc dot gnu dot org
                   ` (9 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-03-28 13:57 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #14 from burnus at gcc dot gnu dot org  2010-03-28 13:56 -------
Created an attachment (id=20230)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20230&action=view)
Draft patch for NULL check for deallocation

Draft patch for removing the NULL check before __builtin_free. According to
POSIX, passing NULL to free is allowed. The patch removes all the NULL checks
where it is likely that the temporary is not-NULL. For procedure arguments
(_gfortran_pack/_gfortran_unpack) the chance is high that it is NULL - which is
the case if the variable was already contiguous. Thus, I left the check in.

The patch removes 5 of the 15 "!= 0" checks for the test case (attachment
20225).


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (13 preceding siblings ...)
  2010-03-28 13:57 ` burnus at gcc dot gnu dot org
@ 2010-03-28 14:11 ` burnus at gcc dot gnu dot org
  2010-03-28 14:45 ` rguenther at suse dot de
                   ` (8 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-03-28 14:11 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #15 from burnus at gcc dot gnu dot org  2010-03-28 14:10 -------
Actually, I am wondering whether one needs to do
  D.1620_135 = __builtin_malloc (1);
for temporary arrays. For user-accessible ALLOCATABLE arrays one does - because
ALLOCATED(array) needs also to be .TRUE. for zero-sized arrays, but for
temporary arrays one does not. For those one could just do
  D.1620_135 = 0B
Will the middle-end optimize __builtin_free(NULL) away?


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (14 preceding siblings ...)
  2010-03-28 14:11 ` burnus at gcc dot gnu dot org
@ 2010-03-28 14:45 ` rguenther at suse dot de
  2010-04-16  8:18 ` burnus at gcc dot gnu dot org
                   ` (7 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenther at suse dot de @ 2010-03-28 14:45 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #16 from rguenther at suse dot de  2010-03-28 14:45 -------
Subject: Re:  Weird temporary array allocation

On Sun, 28 Mar 2010, burnus at gcc dot gnu dot org wrote:

> ------- Comment #15 from burnus at gcc dot gnu dot org  2010-03-28 14:10 -------
> Actually, I am wondering whether one needs to do
>   D.1620_135 = __builtin_malloc (1);
> for temporary arrays. For user-accessible ALLOCATABLE arrays one does - because
> ALLOCATED(array) needs also to be .TRUE. for zero-sized arrays, but for
> temporary arrays one does not. For those one could just do
>   D.1620_135 = 0B
> Will the middle-end optimize __builtin_free(NULL) away?

Not yet, but that's easy to implement.  Note that I will be looking
in detail into malloc/free optimizations, including moving allocation
out of loops.  So there's no need to rush anything and it can wait
for 4.6.

Richard.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (15 preceding siblings ...)
  2010-03-28 14:45 ` rguenther at suse dot de
@ 2010-04-16  8:18 ` burnus at gcc dot gnu dot org
  2010-04-28 14:52 ` rguenth at gcc dot gnu dot org
                   ` (6 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: burnus at gcc dot gnu dot org @ 2010-04-16  8:18 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #17 from burnus at gcc dot gnu dot org  2010-04-16 08:17 -------
Another case where the "if NULL" check is not needed before the "free" are
automatic arrays:
  subroutine sub(n)
    integer :: a(n)
    a(1) = 0
  end
Additionally, the dump looks overly complicated and a least two variables are
set and not used.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (16 preceding siblings ...)
  2010-04-16  8:18 ` burnus at gcc dot gnu dot org
@ 2010-04-28 14:52 ` rguenth at gcc dot gnu dot org
  2010-04-28 15:16 ` amonakov at gcc dot gnu dot org
                   ` (5 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: rguenth at gcc dot gnu dot org @ 2010-04-28 14:52 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #18 from rguenth at gcc dot gnu dot org  2010-04-28 14:52 -------
Updating the status on this bugreport.  I am working on middle-end support
for hoisting/sinking malloc/free pairs out of loops (in case the size is
loop invariant).  The Fortran FE makes this somewhat difficult.

For

subroutine test0(esss,Ix)
  integer(kind=kind(1)), dimension(:), pointer :: esss
  integer(kind=kind(1)), dimension(:), pointer :: Ix
  esss = Ix + Ix
end subroutine

It creates

    D.1552 = ix->dim[0].lbound;
    D.1553 = ix->dim[0].ubound;
...
    D.1562 = D.1553 - D.1552;
...
    D.1571 = D.1562 < 0;
    D.1572 = D.1562 + 1;
    D.1573 = D.1571 ? 0 : D.1572 * 4;
    D.1574 = (void * restrict) __builtin_malloc (MAX_EXPR <D.1573, 1>);
    D.1575 = D.1574;
    atmp.0.data = D.1575;
...
    {
      ...
      S.1 = 0;
      while (1)
        {
          if (S.1 > D.1562) goto L.1;
...
        }
      L.1:;
      S.1 = 0;
      while (1)
        {
          if (S.1 > D.1562) goto L.2;
...
        }
      L.2:;
    }
    {
      void * D.1576;

      D.1576 = (void *) atmp.0.data;
      if (D.1576 != 0B)
        {
          __builtin_free (D.1576);
        }
    }


two unfortunate facts remain.

  1) __builtin_free is executed conditionally even though free(0) is
well-defined
  2) __builtin_malloc (MAX_EXPR <D.1571 ? 0 : D.1572 * 4, 1>) later causes DOM
to jump-thread this into two malloc calls, one malloc(1) and one malloc(D.1573)

especially the latter is very hard to get rid of at the tree level later
(the conditional free can possibly be made unconditional by optimizers).

Thus I would request that for the purpose of allocating array temporaries
from the scalarizer you

 1) unconditionally free the memory
 2) drop the MAX_EXPR, malloc (0) is well-defined and we can just fold that to
NULL if DOM still thinks to jump-thread that
 3) for the same reason you can also drop the + 1 in computing the allocation
size which is currently (ubound - lbound + 1) * 4

even better would be to guard the allocation by the loop entry check
(thus ubound - lbound >= 0)

If that's all acceptable I will work on this soon.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2010-03-27 18:55:47         |2010-04-28 14:52:15
               date|                            |


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (17 preceding siblings ...)
  2010-04-28 14:52 ` rguenth at gcc dot gnu dot org
@ 2010-04-28 15:16 ` amonakov at gcc dot gnu dot org
  2010-04-28 15:20 ` jv244 at cam dot ac dot uk
                   ` (4 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: amonakov at gcc dot gnu dot org @ 2010-04-28 15:16 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #19 from amonakov at gcc dot gnu dot org  2010-04-28 15:15 -------
(In reply to comment #18)
>  3) for the same reason you can also drop the + 1 in computing the allocation
> size which is currently (ubound - lbound + 1) * 4

Sorry, but isn't +1 needed because bounds are inclusive?


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (18 preceding siblings ...)
  2010-04-28 15:16 ` amonakov at gcc dot gnu dot org
@ 2010-04-28 15:20 ` jv244 at cam dot ac dot uk
  2010-04-28 15:43 ` jb at gcc dot gnu dot org
                   ` (3 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: jv244 at cam dot ac dot uk @ 2010-04-28 15:20 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #20 from jv244 at cam dot ac dot uk  2010-04-28 15:20 -------
(In reply to comment #18)
> 
> If that's all acceptable I will work on this soon.
> 

FYI, this would fix PR38318 and PR21046


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (19 preceding siblings ...)
  2010-04-28 15:20 ` jv244 at cam dot ac dot uk
@ 2010-04-28 15:43 ` jb at gcc dot gnu dot org
  2010-04-29 10:02 ` paul dot richard dot thomas at gmail dot com
                   ` (2 subsequent siblings)
  23 siblings, 0 replies; 25+ messages in thread
From: jb at gcc dot gnu dot org @ 2010-04-28 15:43 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #21 from jb at gcc dot gnu dot org  2010-04-28 15:43 -------
(In reply to comment #19)
> (In reply to comment #18)
> >  3) for the same reason you can also drop the + 1 in computing the allocation
> > size which is currently (ubound - lbound + 1) * 4
> 
> Sorry, but isn't +1 needed because bounds are inclusive?
> 

Yes.

As an aside, for the 4.6 array descriptor update, there has been some
discussion to move from the current (lbound, ubound, stride) triplet for every
dimension to (lbound, stride, extent). Which would change these kinds of
expressions.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (20 preceding siblings ...)
  2010-04-28 15:43 ` jb at gcc dot gnu dot org
@ 2010-04-29 10:02 ` paul dot richard dot thomas at gmail dot com
  2010-04-29 13:19 ` jakub at gcc dot gnu dot org
  2010-04-29 19:09 ` tkoenig at gcc dot gnu dot org
  23 siblings, 0 replies; 25+ messages in thread
From: paul dot richard dot thomas at gmail dot com @ 2010-04-29 10:02 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #22 from paul dot richard dot thomas at gmail dot com  2010-04-29 10:02 -------
Subject: Re:  Weird temporary array allocation

> As an aside, for the 4.6 array descriptor update, there has been some
> discussion to move from the current (lbound, ubound, stride) triplet for every
> dimension to (lbound, stride, extent). Which would change these kinds of
> expressions.

Yes, indeed.  As soon as I can free up fortran-dev from the vtable
implementation of dynamic dispatch, I will start on this reform.

It is my intention to change the array descriptor representation and
to start with an API that provides the (lbound, ubound,stride) values.
 In fact, this API has already been put in place in the FE but is
incompletely implemented in the library (or, at least, last time I
looked).

Following this first step, the performance bottle necks due to the API
will have to be weeded out.  Having the extent to hand will greatly
simplify issues such as this PR.

Then the plethora of new descriptor fields will have to be added and
the various associated PRs dealt with.

At this point, the upgrade should be ready for applying to trunk.

Paul


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (21 preceding siblings ...)
  2010-04-29 10:02 ` paul dot richard dot thomas at gmail dot com
@ 2010-04-29 13:19 ` jakub at gcc dot gnu dot org
  2010-04-29 19:09 ` tkoenig at gcc dot gnu dot org
  23 siblings, 0 replies; 25+ messages in thread
From: jakub at gcc dot gnu dot org @ 2010-04-29 13:19 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #23 from jakub at gcc dot gnu dot org  2010-04-29 13:18 -------
Created an attachment (id=20514)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20514&action=view)
d3.c

BTW, I've timed different variants of the C code for the tonto loop, and
time /tmp/d3 1

real    0m0.667s
user    0m0.666s
sys     0m0.001s

time /tmp/d3 2

real    0m0.640s
user    0m0.639s
sys     0m0.000s

time /tmp/d3 3

real    0m0.616s
user    0m0.614s
sys     0m0.002s

and clearly the last one (versioning the loop if the temporary can be easily
avoided for -O3 at least) is the winner here.  I've started looking into that
yesterday.


-- 


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Bug fortran/42958] Weird temporary array allocation
  2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
                   ` (22 preceding siblings ...)
  2010-04-29 13:19 ` jakub at gcc dot gnu dot org
@ 2010-04-29 19:09 ` tkoenig at gcc dot gnu dot org
  23 siblings, 0 replies; 25+ messages in thread
From: tkoenig at gcc dot gnu dot org @ 2010-04-29 19:09 UTC (permalink / raw)
  To: gcc-bugs



------- Comment #24 from tkoenig at gcc dot gnu dot org  2010-04-29 19:09 -------
(In reply to comment #22)

> It is my intention to change the array descriptor representation and
> to start with an API that provides the (lbound, ubound,stride) values.
>  In fact, this API has already been put in place in the FE but is
> incompletely implemented in the library (or, at least, last time I
> looked).

If you find anything still missing in the library, please let me know.
I thought I had converted everything to the macros, which are fairly
easy to change, but I may be mistaken.


-- 

tkoenig at gcc dot gnu dot org changed:

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


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


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2010-04-29 19:09 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-04 16:56 [Bug fortran/42958] New: Weird temporary array allocation rguenth at gcc dot gnu dot org
2010-02-04 19:33 ` [Bug fortran/42958] " burnus at gcc dot gnu dot org
2010-02-05  5:36 ` pault at gcc dot gnu dot org
2010-02-05  9:32 ` rguenther at suse dot de
2010-02-05 14:24 ` rguenth at gcc dot gnu dot org
2010-02-17 20:17 ` jvdelisle at gcc dot gnu dot org
2010-02-20  8:31 ` burnus at gcc dot gnu dot org
2010-03-27 18:55 ` pault at gcc dot gnu dot org
2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
2010-03-27 19:06 ` rguenth at gcc dot gnu dot org
2010-03-27 19:08 ` rguenth at gcc dot gnu dot org
2010-03-27 19:12 ` rguenth at gcc dot gnu dot org
2010-03-28 12:57 ` burnus at gcc dot gnu dot org
2010-03-28 13:05 ` burnus at gcc dot gnu dot org
2010-03-28 13:57 ` burnus at gcc dot gnu dot org
2010-03-28 14:11 ` burnus at gcc dot gnu dot org
2010-03-28 14:45 ` rguenther at suse dot de
2010-04-16  8:18 ` burnus at gcc dot gnu dot org
2010-04-28 14:52 ` rguenth at gcc dot gnu dot org
2010-04-28 15:16 ` amonakov at gcc dot gnu dot org
2010-04-28 15:20 ` jv244 at cam dot ac dot uk
2010-04-28 15:43 ` jb at gcc dot gnu dot org
2010-04-29 10:02 ` paul dot richard dot thomas at gmail dot com
2010-04-29 13:19 ` jakub at gcc dot gnu dot org
2010-04-29 19:09 ` tkoenig at gcc dot gnu dot org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).