Bug 67170 - PRE can't hoist out a readonly argument
Summary: PRE can't hoist out a readonly argument
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: 6.0
Assignee: Richard Biener
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2015-08-10 11:19 UTC by Martin Liška
Modified: 2021-08-27 02:45 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-08-11 00:00:00


Attachments
Test case (264 bytes, text/plain)
2015-08-10 11:19 UTC, Martin Liška
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2015-08-10 11:19:09 UTC
In attached Fortran source file, we are unable to hoist out the single function argument. The argument is reloaded at the end of loop.

Thanks,
Martin
Comment 1 Martin Liška 2015-08-10 11:19:43 UTC
Created attachment 36160 [details]
Test case
Comment 2 Richard Biener 2015-08-11 09:12:11 UTC
Ah.

  # USE = nonlocal escaped { D.3438 }
  # CLB = nonlocal escaped
  foo (&D.3438);

As it clobbers nonlocal it clobbers the incoming argument :/  Because of course
as the argument is pointing to possibly global memory it can access it via
other means than through the argument - and the function attribute in use
(".r") just means accesses through the first argument do not modify the
contents pointed to.

I think Fortran guarantees that things passed as argument are not modified
in other ways (or it makes that undefined).  Not sure about called functions
though.

Now.  In principle I think C says that with

void foo (int * __restrict p)
{
 ...
 bar ();
 ...
}

bar () can only access *p through the use of p or a derived pointer.  So


int x;
bar()
{
  x = 1;
}
main()
{
  foo (&x); 
}

would invoke undefined behavior.

Of course our restrict implementation isn't able to capture that (calls
don't have a clique/base which they would get from the "memory" passed
as reference arguments).

So - confirmed.  But no easy way out :(

Old idea about special-casing recursive calls also exists but without clear
idea of what would be legal special alias answers here.
Comment 3 Richard Biener 2015-08-12 14:18:17 UTC
Ok, the argument may go like "foo (&D.3438) may not modify *arg_29(D)
because then the fnspec on foo would be incorrect - *arg_29(D) would be modified".

Untested patch (works for the testcase):

Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c        (revision 226807)
+++ gcc/tree-ssa-alias.c        (working copy)
@@ -2153,6 +2153,33 @@ call_may_clobber_ref_p_1 (gcall *call, a
        return false;
     }
 
+  /* If the base of an indirect access is a parameter which by means
+     of the fnspec of ourselves not clobbered by us then it is surely
+     not modified by calls we do.  */
+  tree base_ptr;
+  tree fnspec;
+  if (TREE_CODE (base) == MEM_REF
+      && (base_ptr = TREE_OPERAND (base, 0))
+      && TREE_CODE (base_ptr) == SSA_NAME
+      && SSA_NAME_IS_DEFAULT_DEF (base_ptr)
+      && SSA_NAME_VAR (base_ptr)
+      && TREE_CODE (SSA_NAME_VAR (base_ptr)) == PARM_DECL
+      && (fnspec = lookup_attribute ("fn spec",
+                                    TYPE_ATTRIBUTES (TREE_TYPE (cfun->decl)))))
+    {
+      unsigned i = 0;
+      tree arg;
+      for (arg = DECL_ARGUMENTS (cfun->decl);
+          arg && arg != SSA_NAME_VAR (base_ptr); arg = DECL_CHAIN (arg), ++i)
+       ;
+      gcc_assert (arg == SSA_NAME_VAR (base_ptr));
+      fnspec = TREE_VALUE (TREE_VALUE (fnspec));
+      if ((unsigned) TREE_STRING_LENGTH (fnspec) > i + 1
+         && (TREE_STRING_POINTER (fnspec)[i + 1] == 'R'
+             || TREE_STRING_POINTER (fnspec)[i + 1] == 'r'))
+       return false;
+    }
+
   /* Check if the base variable is call-clobbered.  */
   if (DECL_P (base))
     return pt_solution_includes (gimple_call_clobber_set (call), base);


of course this asks for the argument being marked somehow to avoid the
linear search for its index.  It also requires some thinking on if and
when derived pointers (from the argument) allow similar handling.

A way "simpler" approach would be to change code generation by the Frontend
for scalar pass-by-reference intent-in arguments to load such parameters
at the beginning of the function and further use that load result for
all references to that parameter.

Martin, can you check if the above solves the RA issue it was blocking?
Comment 4 Richard Biener 2015-08-13 08:08:25 UTC
Corrected variant, we hit the assert I put in the first one.

Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c        (revision 226852)
+++ gcc/tree-ssa-alias.c        (working copy)
@@ -2153,6 +2153,37 @@ call_may_clobber_ref_p_1 (gcall *call, a
        return false;
     }
 
+  /* If the base of an indirect access is a parameter which by means
+     of the fnspec of ourselves not clobbered by us then it is surely
+     not modified by calls we do.  */
+  tree base_ptr;
+  tree fnspec;
+  if (TREE_CODE (base) == MEM_REF
+      && (base_ptr = TREE_OPERAND (base, 0))
+      && TREE_CODE (base_ptr) == SSA_NAME
+      && SSA_NAME_IS_DEFAULT_DEF (base_ptr)
+      && SSA_NAME_VAR (base_ptr)
+      && TREE_CODE (SSA_NAME_VAR (base_ptr)) == PARM_DECL
+      && (fnspec = lookup_attribute ("fn spec",
+                                    TYPE_ATTRIBUTES (TREE_TYPE (cfun->decl)))))
+    {
+      unsigned i = 0;
+      tree arg;
+      for (arg = DECL_ARGUMENTS (cfun->decl);
+          arg && arg != SSA_NAME_VAR (base_ptr); arg = DECL_CHAIN (arg), ++i)
+       ;
+      /* The static chain is a PARM_DECL as well for example so we may
+        not be able to find base_ptr in the list of arguments.  */
+      if (arg == SSA_NAME_VAR (base_ptr))
+       {
+         fnspec = TREE_VALUE (TREE_VALUE (fnspec));
+         if ((unsigned) TREE_STRING_LENGTH (fnspec) > i + 1
+             && (TREE_STRING_POINTER (fnspec)[i + 1] == 'R'
+                 || TREE_STRING_POINTER (fnspec)[i + 1] == 'r'))
+           return false;
+       }
+    }
+
   /* Check if the base variable is call-clobbered.  */
   if (DECL_P (base))
     return pt_solution_includes (gimple_call_clobber_set (call), base);
Comment 5 Richard Biener 2015-08-13 08:31:10 UTC
Ok to make the alias-oracle approach more "scalable" we'd need to cache the
fn spec parameter/result bits in the functions PARM/RESULT_DECLs
(thus in tree_decl_common where we have conveniently 24bits left to put
in EAF_* and ERF_* flags).

Now fnspec is a type attribute so I'm not sure we can catch all cases
in the attribute handler or if we need to process each function body
(gimplifying parameters sounds like a good place to me for example).

I still think the FE should simplify the MEs life by doing the copy trick
for all intent-in scalars though.
Comment 6 Richard Biener 2015-08-13 08:54:51 UTC
Mine, for the middle-end part.
Comment 7 Richard Biener 2015-09-16 11:24:03 UTC
We can do what the FE should do at gimplification time as well I guess.
Comment 8 Richard Biener 2015-09-25 12:58:24 UTC
I wonder why IPA SRA doesn't turn the scalar parameter from by-reference to by-value passing btw.  I do see that IPA SRA doesn't handle recursion though,
thus

static int __attribute__((noinline)) foo (int *p)
{
  if (*p)
    return foo (p);
  return *p;
}

int main()
{
  int i = 0;
  return foo (&i);
}

is not transformed.  But in the testcase there isn't a recursive call using
the incoming parameter.

As for the idea to use the alias machinery I'm now leaning towards either
having PTA compute a "points-to-readonly" or SCCVN/LIM pre-compute the parm-decl
property and have its own disambiguator handle the disambiguation.
Comment 9 Richard Biener 2015-09-29 13:04:49 UTC
Author: rguenth
Date: Tue Sep 29 13:04:18 2015
New Revision: 228244

URL: https://gcc.gnu.org/viewcvs?rev=228244&root=gcc&view=rev
Log:
2015-09-29  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/67170
	* tree-ssa-alias.h (get_continuation_for_phi): Adjust
	the translate function pointer parameter to get the
	bool whether to disambiguate only by reference.
	(walk_non_aliased_vuses): Likewise.
	* tree-ssa-alias.c (maybe_skip_until): Adjust.
	(get_continuation_for_phi_1): Likewise.
	(get_continuation_for_phi): Likewise.
	(walk_non_aliased_vuses): Likewise.
	* tree-ssa-sccvn.c (const_parms): New bitmap.
	(vn_reference_lookup_3): Adjust for interface change.
	Disambiguate parameters pointing to readonly memory.
	(free_scc_vn): Free const_parms.
	(run_scc_vn): Initialize const_parms from a fn spec attribute.

	* gfortran.dg/pr67170.f90: New testcase.

Added:
    trunk/gcc/testsuite/gfortran.dg/pr67170.f90
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c
    trunk/gcc/tree-ssa-alias.h
    trunk/gcc/tree-ssa-sccvn.c
Comment 10 Richard Biener 2015-09-29 13:13:37 UTC
Fixed.