Bug 43665 - INTENT(IN) etc. optimization of calls: function annotations for noclobber/noescape arguments
INTENT(IN) etc. optimization of calls: function annotations for noclobber/noe...
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: fortran
4.5.0
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
: missed-optimization
Depends on:
Blocks: 44589 45579
  Show dependency treegraph
 
Reported: 2010-04-06 13:29 UTC by Tobias Burnus
Modified: 2013-06-25 09:01 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-05-13 10:31:22


Attachments
Draft patch for external/user procs with INTENT (mostly OK, but breaks existing test) (2.58 KB, patch)
2010-07-20 15:06 UTC, Tobias Burnus
Details | Diff
Patch to address IPA-CP parameter removal issues (839 bytes, patch)
2010-09-06 18:25 UTC, Martin Jambor
Details | Diff
Updated patch (2.72 KB, patch)
2010-09-10 12:09 UTC, Tobias Burnus
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2010-04-06 13:29:50 UTC
After the support goes into the middle end, cf. http://gcc.gnu.org/ml/fortran/2010-04/msg00012.html and http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01257.html , one should use the the new function call argument/return value attributes.

Note: One needs to be careful about this in terms of multi-image coarrays (SYNC ALL, array single-side access) and true asynchronous I/O (WRITE ... WAIT) as otherwise the middle end optimizes too much. (Telling this the middle end via a specifically tailored attribute might be better, though.)

+ /* Call argument flags.  */
+ 
+ /* Nonzero if the argument is not dereferenced recursively, thus only
+    directly reachable memory is read or written.  */
+ #define EAF_DIRECT		(1 << 0)
+ /* Nonzero if memory reached by the argument is not clobbered.  */
+ #define EAF_NOCLOBBER		(1 << 1)
+ /* Nonzero if the argument does not escape.  */
+ #define EAF_NOESCAPE		(1 << 2)
+ /* Nonzero if the argument is not used by the function.  */
+ #define EAF_UNUSED		(1 << 3)
+
+ /* Call return flags.  */
+ 
+ /* Mask for the argument number that is returned.  Lower two bits of
+    the return flags, encodes argument slots zero to three.  */
+ #define ERF_RETURN_ARG_MASK	(3)
+ /* Nonzero if the return value is equal to the argument number
+    flags & ERF_RETURN_ARG_MASK.  */
+ #define ERF_RETURNS_ARG		(1 << 2)
+ /* Nonzero if the return value does not alias with anything.  Functions
+    with the malloc attribute have this set on their return value.  */
+ #define ERF_NOALIAS		(1 << 3)
Comment 1 Tobias Burnus 2010-04-06 13:50:42 UTC
Some more quotes:

> You can mark parameters 1) unused, 2) pointed-to read-only,
> 3) not escaping, 4) only once dereferenced (thus, access only
> *p, not **p)
>
> You can mark return values as being a direct copy of arguments
> or as non-aliasing (similar to the malloc attribute).

+       attr = lookup_attribute ("fnspec", TYPE_ATTRIBUTES (type));

+   switch (TREE_STRING_POINTER (attr)[1 + arg])
+     {
+     case 'x':
+     case 'X':
+       return EAF_UNUSED;
+ 
+     case 'R':
+       return EAF_DIRECT | EAF_NOCLOBBER | EAF_NOESCAPE;
+ 
+     case 'r':
+       return EAF_NOCLOBBER | EAF_NOESCAPE;
+ 
+     case 'W':
+       return EAF_DIRECT | EAF_NOESCAPE;
+ 
+     case 'w':
+       return EAF_NOESCAPE;
Comment 2 Tobias Burnus 2010-05-10 10:10:39 UTC
I/O: Currently both READ an WRITE map to the same function; this should be changed such that for WRITE the arguments are marked as EAF_NOCLOBBER. This can be done using aliases, cf. http://gcc.gnu.org/ml/fortran/2010-05/msg00048.html

For the committal of the middle-end / front end, see
- http://gcc.gnu.org/ml/fortran/2010-05/msg00092.html (committal)
- http://gcc.gnu.org/ml/fortran/2010-05/msg00032.html (patch)
Comment 3 Daniel Franke 2010-05-13 10:31:22 UTC
Initial patch (trans-decl.c, trans.io.c) here:
    http://gcc.gnu.org/ml/fortran/2010-05/msg00124.html

Mapping formal arguments to fnspec should be doable, but I'm experienced enough in tree-things to continue.
Comment 4 Tobias Burnus 2010-07-13 13:21:11 UTC
Subject: Bug 43665

Author: burnus
Date: Tue Jul 13 13:20:52 2010
New Revision: 162140

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162140
Log:
2010-07-13  Daniel Franke  <franke.daniel@gmail.com>
            Tobias Burnus  <burnus@net-b.de>

        PR fortran/43665
        * trans-decl.c (gfc_build_intrinsic_function_decls): Add
        noclobber/noescape annotations to function calls.
        (gfc_build_builtin_function_decls): Likewise.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-decl.c

Comment 5 Tobias Burnus 2010-07-13 17:26:23 UTC
Subject: Bug 43665

Author: burnus
Date: Tue Jul 13 17:26:02 2010
New Revision: 162147

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162147
Log:
2010-07-13  Tobias Burnus  <burnus@net-b.de>
            Daniel Franke  <franke.daniel@gmail.com>

        PR fortran/43665
        * trans.h (gfc_build_library_function_decl_with_spec): New
        prototype.
        * trans-decl.c (gfc_build_library_function_decl_with_spec):
        Removed static.
        * trans-io (gfc_build_io_library_fndecls): Add "fn spec"
        annotations.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/fortran/trans-io.c
    trunk/gcc/fortran/trans.h

Comment 6 Tobias Burnus 2010-07-13 17:32:31 UTC
Main library calls are done.

TODO:
- Intrinsic calls such as "call ctime()" which do not have a function declaration
- Nonintrinsic functions with non-pointer INTENT(IN/OUT); handle also (non)clobber [target/pointer ...]
- I/O transfer: Split somehow to make it possible to distinguish READ from WRITE.
- Possibly, handle somehow unused arguments / not-explicitly given INTENT(in)s  by saving this information
Comment 7 Tobias Burnus 2010-07-20 15:06:17 UTC
Created attachment 21265 [details]
Draft patch for external/user procs with INTENT (mostly OK, but breaks existing test)

The attached patch mostly works, except for gfortran.dg/allocatable_scalar_4.f90.

The latter file works fine with -O1 but with -O2 it crashes. At -O1 with -fdump-tree-original, essentially only one "if" is optimized away:

  b = 7482
  call checkOptional(.false.,.true., 7482)
  if (b /= 7482) call abort() ! <<<< This line is removed for -O1
  call checkOptional(.true., .true., 7482, b)

I could not see anything suspicious for -O{2,1} -fdump-tree-optimized; one should check -fdump-tree-optimized-all. I somehow have the feeling that some other DECL is wrong. Cf. PR 44945 for another manifestation of DECL problems (though there for derived types).

Note: Using -fwhole-file does not help.
Comment 8 Tobias Burnus 2010-07-20 15:26:34 UTC
Reduced test case (of allocatable_scalar_4.f90) - fails with "ERROR STOP 1":

program test
  implicit none
  integer, allocatable :: b
  allocate(b)
  b = 7482
  call checkOptional(.false.,.true., 7482)
  call checkOptional(.true., .true., 7482, b)
  if (b /= 46) error stop 1
contains
  subroutine checkOptional(prsnt, alloc, val, x)
    logical, intent(in) :: prsnt, alloc
    integer, allocatable, optional :: x
    integer, intent(in) :: val
    if (present(x)) then
      if (allocated(x) .neqv. alloc) error stop 2
    end if
    if (present(x)) then
      if (allocated(x)) then
        if (x /= val) error stop 3
      end if
    end if
    if (present(x)) then
      x = 46
    end if
  end subroutine checkOptional
end program test
Comment 9 Tobias Burnus 2010-07-20 15:58:07 UTC
The procedure 'checkoptional' gets '.rrrw' set as the fn spec.

With -O1, the if after the first "checkoptional" gets properly optimized away (cf. comment 7 and original test case).

With -O2, the number of arguments is reduced to one (NULL vs. &b) and the condition
  if (b /= 46) error stop 1
is changed to
  error stop 1
(cf. -fdump-tree-optimized)

For me it looks as if the "fn spec" information is not moved along when arguments are eliminated but remain at the previous position. It works if one swaps the order (i.e. it works if the optional argument is at position one).

Richard, does this make sense or am I completely off track?
Comment 10 Richard Biener 2010-07-20 16:01:00 UTC
No, this problem was present for ipa-sra, but I thought it was fixed.

Does -fno-ipa-sra help?  Martin?
Comment 11 Tobias Burnus 2010-07-20 16:12:28 UTC
(In reply to comment #10)
> No, this problem was present for ipa-sra, but I thought it was fixed.
> Does -fno-ipa-sra help?

No, it doesn't. But with that option, the number of arguments still reduces from 4 to 2 (before: 1). But again, the code works if one reorders the arguments such that the "w" attribute matches the "x" argument (now at position 2 instead of 1).
Comment 12 rguenther@suse.de 2010-07-21 08:09:40 UTC
Subject: Re:  INTENT(IN) etc. optimization of calls:
 function annotations for noclobber/noescape arguments

On Tue, 20 Jul 2010, burnus at gcc dot gnu dot org wrote:

> ------- Comment #11 from burnus at gcc dot gnu dot org  2010-07-20 16:12 -------
> (In reply to comment #10)
> > No, this problem was present for ipa-sra, but I thought it was fixed.
> > Does -fno-ipa-sra help?
> 
> No, it doesn't. But with that option, the number of arguments still reduces
> from 4 to 2 (before: 1). But again, the code works if one reorders the
> arguments such that the "w" attribute matches the "x" argument (now at position
> 2 instead of 1).

So I wonder what code removes the arguments then.

Richard.
Comment 13 Martin Jambor 2010-07-21 08:27:09 UTC
(In reply to comment #12)
> So I wonder what code removes the arguments then.
> 

IPA-CP can do that for quite some time please try with -fno-ipa-cp.

(I don't have a trunk built with enabled fortran at hand and I am a
bit overwhelmed with bugs and other stuff recently so I can have a
look at this but it will take at least a few days before I get to it.)

Comment 14 Tobias Burnus 2010-07-22 15:36:25 UTC
(In reply to comment #13)
> (In reply to comment #12)
> IPA-CP can do that for quite some time please try with -fno-ipa-cp.

As expected: It works with -fno-ipa-cp.


> (I don't have a trunk built with enabled fortran at hand and I am a
> bit overwhelmed with bugs and other stuff recently so I can have a
> look at this but it will take at least a few days before I get to it.)

Thanks.

Some pre-analysis: The "fn spec" (the space is there to prevent other than internal use - thus I cannot create a C test case) is converted in gimple.c's gimple_call_arg_flags to EAF_* constants.

tree-ssa-structalias.c's handle_rhs_call uses them via:
  for (i = 0; i < gimple_call_num_args (stmt); ++i)
      [...]
      int flags = gimple_call_arg_flags (stmt, i);
thus, the position seems to matter here.

The latter is called by find_func_aliases in an "if (!in_ipa_mode ..." block.
Comment 15 Martin Jambor 2010-09-06 14:12:44 UTC
I tried compiling the testcase from comment #8 and it did not fail for
me either on i686-linux ox x86_64-linux.  Can you please check that it
still fails for you?
Comment 16 Martin Jambor 2010-09-06 18:25:50 UTC
Created attachment 21714 [details]
Patch to address IPA-CP parameter removal issues

This patch makes IPA-CP to refrain from modifying a function when it
sees a any type attributes.  It fixes the test case.  I do not expect
it to cause any problems elsewhere but I have not yet bootstrapped or
tested it (I have just scheduled both for tonight).

As far as I understand it there is already a test in our testsuite
that fails (with the patch from comment #7 applied) and so I am not
going to add an extra one.

I think it's best to check this in separately and I will submit it for
approval tomorrow if there are no unforeseen problems.
Comment 17 Tobias Burnus 2010-09-06 18:43:52 UTC
(In reply to comment #16)
> This patch makes IPA-CP to refrain from modifying a function when it
> sees a any type attributes.

In a way that's unfortunate: Both "fn attr" and argument removal are optimization options.

> As far as I understand it there is already a test in our testsuite
> that fails (with the patch from comment #7 applied) and so I am not
> going to add an extra one.

OK.

> I think it's best to check this in separately and I will submit it for
> approval tomorrow if there are no unforeseen problems.

I will also submit my patch - for committal after yours is in.
Comment 18 Martin Jambor 2010-09-07 17:01:20 UTC
Subject: Bug 43665

Author: jamborm
Date: Tue Sep  7 17:00:44 2010
New Revision: 163960

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163960
Log:
2010-09-07  Martin Jambor  <mjambor@suse.cz>

	PR fortran/43665
	* ipa-cp.c (ipcp_versionable_function_p): Return false if there
	are any type attributes.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-cp.c

Comment 19 Tobias Burnus 2010-09-08 06:25:20 UTC
Reviewed patch ("OK") available at
  http://gcc.gnu.org/ml/fortran/2010-09/msg00198.html
however, it causes regressions as some of the intrinsics (in intrinsic.c) have the wrong intents - which causes wrong code (too much optimized away). Thus, one first needs to audit and fix intrinsic.c before this patch can be committed.
Comment 20 Tobias Burnus 2010-09-09 08:43:16 UTC
Subject: Bug 43665

Author: burnus
Date: Thu Sep  9 08:42:52 2010
New Revision: 164052

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

        PR fortran/43665
        * intrincic.texi (FGET, FGETC, FPUT, FPUTC, FSTAT, GETCWD, KILL,
        STAT): Show also syntax for the function version.
        * intrinsic.c (add_sym_1s_intent, add_sym_2s_intent,
         add_sym_3s_intent): Remove function.
        (add_sym_1s, add_sym_2s, add_sym_3s): Take always the intent
        as argument.
        (add_sym_2_intent): New function.
        (add_functions): Set intent for functions which modify
        the argument: fstat, fgetc, fget, hostnm, lstat, stat. Change
        argument name of hostnm from "a" to "c"
        (add_subroutines): Change add_sym_*s_intent to
        add_sym_*s and add intent to the add_sym_*s calls. 


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/intrinsic.c
    trunk/gcc/fortran/intrinsic.texi

Comment 21 Tobias Burnus 2010-09-10 12:09:15 UTC
Created attachment 21765 [details]
Updated patch

Updated patch to fix review issues and the Cray patch issue. I won't be able to work on this (i.e. regtest + submit) for the next days thus I put it here.

Cf. http://gcc.gnu.org/ml/fortran/2010-09/msg00198.html
and http://gcc.gnu.org/ml/fortran/2010-09/msg00234.html
Comment 22 Tobias Burnus 2010-09-10 12:12:33 UTC
(In reply to comment #21)
> Created an attachment (id=21765) [edit]
> Updated patch

Note: I forgot to include the test case from attachment 21265 [details]
Comment 23 Dominique d'Humieres 2010-09-11 15:12:55 UTC
I have applied the patch in comment #21 without regression, but the test case from attachment 21265 [details] fails:

FAIL: gfortran.dg/intent_optimize_1.f90  -O  scan-tree-dump-times optimized "does_not_exist" 0
Comment 24 Tobias Burnus 2010-09-12 09:32:19 UTC
(In reply to comment #23)
> I have applied the patch in comment #21 without regression, but the test case
> from attachment 21265 [details] [edit] fails:

-  create_fn_spec (sym, type);
+  type = create_fn_spec (sym, type);

as in the original patch.
Comment 25 Dominique d'Humieres 2010-09-12 10:13:51 UTC
> -  create_fn_spec (sym, type);
> +  type = create_fn_spec (sym, type);
> 
> as in the original patch.

With this change the test succeeds.
Comment 26 Tobias Burnus 2010-09-16 21:30:20 UTC
Subject: Bug 43665

Author: burnus
Date: Thu Sep 16 21:30:05 2010
New Revision: 164348

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

        PR fortran/43665
        * trans-types.c (create_fn_spec): New function.
        (gfc_get_function_type): Call it.

2010-09-16  Tobias Burnus  <burnus@net-b.de>

        PR fortran/43665
        * gfortran.dg/cray_pointers_2.f90: Disable inlining to avoid
        optimizations.
        * gfortran.dg/intent_optimize_1.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/intent_optimize_1.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-types.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/cray_pointers_2.f90

Comment 27 Thomas Koenig 2010-10-16 16:06:16 UTC
Author: tkoenig
Date: Sat Oct 16 16:06:07 2010
New Revision: 165559

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=165559
Log:
2010-10-16  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/20165
	PR fortran/31593
	PR fortran/43665
	* gfortran.map:  Add _gfortran_transfer_array_write,
	_gfortran_transfer_array_write, _gfortran_transfer_character_write,
	_gfortran_transfer_character_wide_write,
	_gfortran_transfer_complex_write,
	_gfortran_transfer_integer_write,
	_gfortran_transfer_logical_write and
	_gfortran_transfer_real_write.
	* io/transfer.c (transfer_integer_write):  Add prototype and
	function body as call to the original function, without the
	_write.
	(transfer_real_write):  Likewise.
	(transfer_logical_write):  Likewise.
	(transfer_character_write):  Likewise.
	(transfer_character_wide_write):  Likewise.
	(transfer_complex_write):  Likewise.
	(transfer_array_write):  Likewise.

2010-10-16  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/20165
	PR fortran/31593
	PR fortran/43665
	* trans-io.c (enum iocall): Add IOCALL_X_INTEGER_WRITE,
	IOCALL_X_LOGICAL_WRITE, IOCALL_X_CHARACTER_WRITE,
	IOCALL_X_CHARACTER_WIDE_WRIE, IOCALL_X_REAL_WRITE,
	IOCALL_X_COMPLEX_WRITE and IOCALL_X_ARRAY_WRITE.
	(gfc_build_io_library_fndecls):  Add corresponding function
	decls.
	(transfer_expr):  If the current transfer is a READ, use
	the iocall with the original version, otherwise the version
	with _WRITE.
	(transfer_array_desc):  Likewise.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-io.c
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/gfortran.map
    trunk/libgfortran/io/transfer.c
Comment 28 Mikael Morin 2012-06-29 17:31:19 UTC
Can this be closed?
Comment 29 Dominique d'Humieres 2013-06-25 09:01:49 UTC
> Can this be closed?

No answer since almost a year. Closing as FIXED.