Re: [PATCH, PR68716] Fix GOMP/GOACC_parallel handling in find_func_clobbers

[Jakub Jelinek]

On Wed, Dec 09, 2015 at 11:01:31AM +0100, Tom de Vries wrote:
>   PR tree-optimization/68716
>   * tree-ssa-structalias.c (find_func_clobbers): Fix handling of
>   BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.

Pasto in ChangeLog entry?

Jakub

Re: [PATCH, PR68716] Fix GOMP/GOACC_parallel handling in find_func_clobbers

[Tom de Vries]

On 09/12/15 11:03, Jakub Jelinek wrote:

On Wed, Dec 09, 2015 at 11:01:31AM +0100, Tom de Vries wrote:

PR tree-optimization/68716
* tree-ssa-structalias.c (find_func_clobbers): Fix handling of
BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.


Pasto in ChangeLog entry?



Indeed, thanks for noticing.

- Tom

Fix GOMP/GOACC_parallel handling in find_func_clobbers

2015-12-08  Tom de Vries  

	PR tree-optimization/68716
	* tree-ssa-structalias.c (find_func_clobbers): Fix handling of
	BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOACC_PARALLEL.

	* testsuite/libgomp.c/omp-nested-2.c: New test.

---
 gcc/tree-ssa-structalias.c | 47 +-
 libgomp/testsuite/libgomp.c/omp-nested-2.c |  4 +++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 060ff3e..15e351e 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -5082,7 +5082,52 @@ find_func_clobbers (struct function *fn, gimple *origt)
 	return;
 	  case BUILT_IN_GOMP_PARALLEL:
 	  case BUILT_IN_GOACC_PARALLEL:
-	return;
+	{
+	  unsigned int fnpos, argpos;
+	  switch (DECL_FUNCTION_CODE (decl))
+		{
+		case BUILT_IN_GOMP_PARALLEL:
+		  /* __builtin_GOMP_parallel (fn, data, num_threads, flags).  */
+		  fnpos = 0;
+		  argpos = 1;
+		  break;
+		case BUILT_IN_GOACC_PARALLEL:
+		  /* __builtin_GOACC_parallel (device, fn, mapnum, hostaddrs,
+	   sizes, kinds, ...).  */
+		  fnpos = 1;
+		  argpos = 3;
+		  break;
+		default:
+		  gcc_unreachable ();
+		}
+
+	  tree fnarg = gimple_call_arg (t, fnpos);
+	  gcc_assert (TREE_CODE (fnarg) == ADDR_EXPR);
+	  tree fndecl = TREE_OPERAND (fnarg, 0);
+	  varinfo_t cfi = get_vi_for_tree (fndecl);
+
+	  tree arg = gimple_call_arg (t, argpos);
+
+	  /* Parameter passed by value is used.  */
+	  lhs = get_function_part_constraint (fi, fi_uses);
+	  struct constraint_expr *rhsp;
+	  get_constraint_for (arg, );
+	  FOR_EACH_VEC_ELT (rhsc, j, rhsp)
+		process_constraint (new_constraint (lhs, *rhsp));
+	  rhsc.truncate (0);
+
+	  /* The caller clobbers what the callee does.  */
+	  lhs = get_function_part_constraint (fi, fi_clobbers);
+	  rhs = get_function_part_constraint (cfi, fi_clobbers);
+	  process_constraint (new_constraint (lhs, rhs));
+
+	  /* The caller uses what the callee does.  */
+	  lhs = get_function_part_constraint (fi, fi_uses);
+	  rhs = get_function_part_constraint (cfi, fi_uses);
+	  process_constraint (new_constraint (lhs, rhs));
+
+	  return;
+	}
 	  /* printf-style functions may have hooks to set pointers to
 	 point to somewhere into the generated string.  Leave them
 	 for a later exercise...  */
diff --git a/libgomp/testsuite/libgomp.c/omp-nested-2.c b/libgomp/testsuite/libgomp.c/omp-nested-2.c
new file mode 100644
index 000..7495afb
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/omp-nested-2.c
@@ -0,0 +1,4 @@
+// { dg-do run }
+// { dg-additional-options "-fipa-pta" }
+
+#include "omp-nested-1.c"

Re: [PATCH, PR68716] Fix GOMP/GOACC_parallel handling in find_func_clobbers

[Richard Biener]

On Wed, 9 Dec 2015, Jakub Jelinek wrote:

> On Wed, Dec 09, 2015 at 11:01:31AM +0100, Tom de Vries wrote:
> > PR tree-optimization/68716
> > * tree-ssa-structalias.c (find_func_clobbers): Fix handling of
> > BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.
> 
> Pasto in ChangeLog entry?

Ok with that fixed.

Richard.

[PATCH, PR68716] Fix GOMP/GOACC_parallel handling in find_func_clobbers

[Tom de Vries]

[ was: Re: [PATCH, PR46032] Handle BUILT_IN_GOMP_PARALLEL in ipa-pta ]

On 30/11/15 13:11, Tom de Vries wrote:

On 30/11/15 10:16, Richard Biener wrote:

On Mon, 30 Nov 2015, Tom de Vries wrote:


Hi,

this patch fixes PR46032.

It handles a call:
...
   __builtin_GOMP_parallel (fn, data, num_threads, flags)
...
as:
...
   fn (data)
...
in ipa-pta.

This improves ipa-pta alias analysis in the parallelized function fn,
and
allows vectorization in the testcase without a runtime alias test.

Bootstrapped and reg-tested on x86_64.

OK for stage3 trunk?


+ /* Assign the passed argument to the appropriate incoming
+parameter of the function.  */
+ struct constraint_expr lhs ;
+ lhs = get_function_part_constraint (fi, fi_parm_base + 0);
+ auto_vec rhsc;
+ struct constraint_expr *rhsp;
+ get_constraint_for_rhs (arg, );
+ while (rhsc.length () != 0)
+   {
+ rhsp =  ();
+ process_constraint (new_constraint (lhs, *rhsp));
+ rhsc.pop ();
+   }

please use style used elsewhere with

  FOR_EACH_VEC_ELT (rhsc, j, rhsp)
process_constraint (new_constraint (lhs, *rhsp));
  rhsc.truncate (0);



That code was copied from find_func_aliases_for_call.
I've factored out the bit that I copied as
find_func_aliases_for_call_arg, and fixed the style there (and dropped
'rhsc.truncate (0)' since AFAIU it's redundant at the end of a function).


+ /* Parameter passed by value is used.  */
+ lhs = get_function_part_constraint (fi, fi_uses);
+ struct constraint_expr *rhsp;
+ get_constraint_for_address_of (arg, );

This isn't correct - you want to use get_constraint_for (arg, ).
After all rhs is already an ADDR_EXPR.



Can we add an assert somewhere to detect this incorrect usage?


+ FOR_EACH_VEC_ELT (rhsc, j, rhsp)
+   process_constraint (new_constraint (lhs, *rhsp));
+ rhsc.truncate (0);
+
+ /* The caller clobbers what the callee does.  */
+ lhs = get_function_part_constraint (fi, fi_clobbers);
+ rhs = get_function_part_constraint (cfi, fi_clobbers);
+ process_constraint (new_constraint (lhs, rhs));
+
+ /* The caller uses what the callee does.  */
+ lhs = get_function_part_constraint (fi, fi_uses);
+ rhs = get_function_part_constraint (cfi, fi_uses);
+ process_constraint (new_constraint (lhs, rhs));

I don't see why you need those.  The solver should compute these
in even better precision (context sensitive on the call side).

The same is true for the function parameter.  That is, the only
needed part of the patch should be that making sure we see
the "direct" call and assign parameters correctly.



Dropped this bit.


Dropping the find_func_clobbers bit (in particular, the part copying the 
clobbers) caused PR68716.


Basically the find_func_clobbers bit tries to emulate the generic 
handling of calls in find_func_clobbers, but pretends the call is

  fn (data)
when handling
  __builtin_GOMP_parallel (fn, data, num_threads, flags)
.

I used the same comments as in the generic call handling to make it 
clear what part of the generic call handling is emulated.


Compared to the originally posted patch, the changes are:
- removed 'gcc_assert (TREE_CODE (arg) == ADDR_EXPR)'. Ran into a case
  where arg is NULL.
- use get_constraint_for instead of get_constraint_for_address_of as
  requested in a review comment above.
- handle GOACC_parallel as well

Bootstrapped and reg-tested on x86_64.

OK for stage3 trunk?

Thanks,
- Tom

Fix GOMP/GOACC_parallel handling in find_func_clobbers

2015-12-08  Tom de Vries  

	PR tree-optimization/68716
	* tree-ssa-structalias.c (find_func_clobbers): Fix handling of
	BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.

	* testsuite/libgomp.c/omp-nested-2.c: New test.

---
 gcc/tree-ssa-structalias.c | 47 +-
 libgomp/testsuite/libgomp.c/omp-nested-2.c |  4 +++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 060ff3e..15e351e 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -5082,7 +5082,52 @@ find_func_clobbers (struct function *fn, gimple *origt)
 	return;
 	  case BUILT_IN_GOMP_PARALLEL:
 	  case BUILT_IN_GOACC_PARALLEL:
-	return;
+	{
+	  unsigned int fnpos, argpos;
+	  switch (DECL_FUNCTION_CODE (decl))
+		{
+		case BUILT_IN_GOMP_PARALLEL:
+		  /* __builtin_GOMP_parallel (fn, data, num_threads, flags).  */
+		  fnpos = 0;
+		  argpos = 1;
+		  break;
+		case BUILT_IN_GOACC_PARALLEL:
+		  /* __builtin_GOACC_parallel (device, fn, mapnum, hostaddrs,
+	   sizes, kinds, ...).  */
+		  fnpos = 1;
+		  argpos = 3;
+