Bug 43173 - Unnecessary array temporary: Passing contiguous array as actual argument
Summary: Unnecessary array temporary: Passing contiguous array as actual argument
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2010-02-25 11:26 UTC by Tobias Burnus
Modified: 2010-03-02 12:10 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.4.3
Last reconfirmed: 2010-02-26 05:28:34


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2010-02-25 11:26:35 UTC
Related to some other missed-optimization PRs, but particularly simple.

  REAL, allocatable :: ot(:)
  integer :: time_steps

  call foo (ot) ! OK, no temporary
  call foo (ot(0:5:1)) ! Unnecessary temporary
  call foo (ot(0:time_steps)) ! Unnecessary temporary
  end

Note: ot is contiguous (as it is allocatable), stride = 1, and thus all arguments are simple contiguous - unless the stride is non-unity or one has, e.g., "ot( [1,3,5])".

Note 2: If one has an explicit-size array, it works - there is no temporary for:
  REAL :: ot(5)
  integer :: time_steps
  call foo (ot(1:time_steps))
I think the issue is that one mixes up the (:) of an allocatable array with the (:) of a non-allocatable, assumed-shape dummy argument or a pointer.

The temporary seems to be generated in trans-arrays.c's gfc_conv_array_parameter; the issue seems to be that "full_array_var" is false and then later there is either no_pack or contiguous set to false.
Comment 1 Paul Thomas 2010-02-25 12:48:23 UTC
This does not occur on i386.  I will try on a x86_64 tonight.

Paul
Comment 2 Tobias Burnus 2010-02-25 12:56:50 UTC
(In reply to comment #1)
> This does not occur on i386.  I will try on a x86_64 tonight.

The warning for -Warray-temporaries is shown here for both -m32 and -m64 -- and I also see "_gfortran_internal_pack" in the -fdump-tree-original. Do you have by chance local changes which fix this issue?
Comment 3 Paul Thomas 2010-02-26 05:28:34 UTC
> The warning for -Warray-temporaries is shown here for both -m32 and -m64 -- and
> I also see "_gfortran_internal_pack" in the -fdump-tree-original. Do you have
> by chance local changes which fix this issue?

      CALL foo ((MAIN__:ot(FULL)))
      CALL foo ((MAIN__:ot(0_8:5_8:1_8)))
      CALL foo ((MAIN__:ot(0_8:__convert_i4_i8[[((MAIN__:time_steps))]])))

I was distracted by the convert_i4_i8.  Not only does it not appear for the second line but it does not appear at all with -fdefault-integer-8.

I will go back and confirm that the tree on my machine at work is clean.

Confirmed

Cheers

Paul
Comment 4 Paul Thomas 2010-02-26 16:06:31 UTC
(In reply to comment #3)

> > by chance local changes which fix this issue?

> I will go back and confirm that the tree on my machine at work is clean.

No, it wasn't, so my comment was incorrect.

Cheers

Paul
Comment 5 Thomas Koenig 2010-02-27 11:35:51 UTC
I do not see the temporaries with

gcc-Version 4.5.0 20100214 (experimental) (GCC)

but I see this with

gcc-Version 4.5.0 20100227 (experimental) (GCC)

on x86_64-unknown-linux-gnu

This makes this a regression.
Comment 6 Richard Biener 2010-03-01 11:54:37 UTC
Not a regression (we track regressions only against releases).  I see both
temporaries also on the 4.4 branch.
Comment 7 Tobias Burnus 2010-03-02 09:16:53 UTC
> I do not see the temporaries with [...]  4.5.0 20100214
> but I see this with [...]                4.5.0 20100227

I think the "regression" is due to:
http://gcc.gnu.org/viewcvs?view=revision&revision=156926

Namely due to the added "no_pack" logic in trans-arrays.c's
gfc_conv_array_parameter (as suggested in comment 0).
http://gcc.gnu.org/viewcvs/trunk/gcc/fortran/trans-array.c?r1=156926&r2=156925

That committal fixed PR 36932, PR 36933, PR 43072, and PR 43111.
Comment 8 Tobias Burnus 2010-03-02 09:24:08 UTC
Completely untested patch:

--- trans-array.c       (revision 157160)
+++ trans-array.c       (working copy)
@@ -5555,5 +5555,5 @@ gfc_conv_array_parameter (gfc_se * se, g
   no_pack = ((sym && sym->as
                  && !sym->attr.pointer
-                 && sym->as->type != AS_DEFERRED
+                 && (sym->as->type != AS_DEFERRED || sym->attr.allocatable)
                  && sym->as->type != AS_ASSUMED_SHAPE)
                      ||
Comment 9 Tobias Burnus 2010-03-02 10:01:37 UTC
(In reply to comment #8)
> Completely untested patch:

Still untested, but I think it might lead to wrong code with either
  allocatable(1)%pointer(1:2)  ! Potentially non-contiguous
or
  pointer%allocatable(1:2) ! Cannot alias & is contiguous
Though I have not really studied the source code.

Actually, also for other parts in this function I am unsure whether it really takes derived-type components into account.
Comment 10 Paul Thomas 2010-03-02 11:58:41 UTC
Subject: Bug 43173

Author: pault
Date: Tue Mar  2 11:58:02 2010
New Revision: 157163

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

	PR fortran/43180
	* trans-array.c (gfc_conv_array_parameter): A full array of
	derived type need not be restricted to a symbol without an
	array spec to use the call to gfc_conv_expr_descriptor.

	PR fortran/43173
	* trans-array.c (gfc_conv_array_parameter): Contiguous refs to
	allocatable arrays do not need temporaries.

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

	PR fortran/43180
	* gfortran.dg/internal_pack_10.f90: New test.

	PR fortran/43173
	* gfortran.dg/internal_pack_11.f90: New test.

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

Comment 11 Paul Thomas 2010-03-02 12:10:40 UTC
Fixed on trunk, thanks for the report!

Paul