[PATCH, PR66432] Handle PARM_DECL in remap_gimple_op_r

[PATCH, PR66432] Handle PARM_DECL in remap_gimple_op_r

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]

Hi,

I.

When running test libgomp.c/appendix-a/a.29.1.c with '--target_board 
unix/-O2/-g', we run into this failure:
...
FAIL: libgomp.c/appendix-a/a.29.1.c (test for excess errors)
Excess errors:
src/libgomp/testsuite/libgomp.c/appendix-a/a.29.1.c:6:1: error: type 
mismatch between an SSA_NAME and its symbol
...

Without -g, the testcase passes.


II.

The scenario for the failure is as follows:

At fnsplit, we split off f.part.0 from f, which at source level looks 
like this:
...
void
f (int n, int B[n][n], int C[])
{
   int D[2][2] = { 1, 2, 3, 4 };
   int E[n][n];
   assert (n >= 2);
   E[1][1] = 4;
#pragma omp parallel firstprivate(B, C, D, E)
   {
     assert (sizeof (B) == sizeof (int (*)[n]));
     assert (sizeof (C) == sizeof (int *));
     assert (sizeof (D) == 4 * sizeof (int));
     assert (sizeof (E) == n * n * sizeof (int));
     /* Private B and C have values of original B and C. */
     assert (&B[1][1] == &A[1][1]);
     assert (&C[3] == &A[1][1]);
     assert (D[1][1] == 4);
     assert (E[1][1] == 4);
   }
}
...

The split introduces a debug_insn and ssa-name that references param B in f:
...
   # DEBUG D#4ptD.0 => B_3(D)
..

And a debug_insn that references param B in f.part.0:
...
   # DEBUG D#7ptD.0 s=> BD.1846
...

At this point, the type of the ssa name and the param are the same.

Then at inline, we decide to inline f.part.0 back into function f.

For inlining, we rewrite the body of inlined function f.part.0. While 
rewriting the debug insn mentioned above, we hit this code for param B:
...
       if (TREE_CODE (*tp) != OMP_CLAUSE)
         TREE_TYPE (*tp) = remap_type (TREE_TYPE (*tp), id);
...
And since it's a variable-sized type, the type of param is changed.

Now the type of the ssa name and the param are no longer the same, and a 
bit later we hit the ICE.


III.

Attached patch fixes the ICE by handling PARM_DECL in remap_gimple_op_r.
[ I'm not confident though this is the right fix ]

Bootstrapped and reg-tested on x86_64.

OK for trunk, or to be fixed otherwise?

Thanks,
- Tom

[-- Attachment #2: 0001-Handle-PARM_DECL-in-remap_gimple_op_r.patch --]
[-- Type: text/x-patch, Size: 2682 bytes --]

Handle PARM_DECL in remap_gimple_op_r

2015-06-30  Tom de Vries  <tom@codesourcery.com>

	PR middle-end/66432
	* tree-inline.c (remap_gimple_op_r): Handle PARM_DECL in
	remap_gimple_op_r.

	* testsuite/libgomp.c/appendix-a/a.29.1-g.c: New test.
---
 gcc/tree-inline.c                                 | 10 ++++++--
 libgomp/testsuite/libgomp.c/appendix-a/a.29.1-g.c | 31 +++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 libgomp/testsuite/libgomp.c/appendix-a/a.29.1-g.c

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 6f138ed..422fd33 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -919,10 +919,16 @@ remap_gimple_op_r (tree *tp, int *walk_subtrees, void *data)
 	       || decl_function_context (*tp) == id->src_fn))
     /* These may need to be remapped for EH handling.  */
     *tp = remap_decl (*tp, id);
-  else if (TREE_CODE (*tp) == FIELD_DECL)
+  else if (TREE_CODE (*tp) == FIELD_DECL
+	  || TREE_CODE (*tp) == PARM_DECL)
     {
       /* If the enclosing record type is variably_modified_type_p, the field
-	 has already been remapped.  Otherwise, it need not be.  */
+	has already been remapped.  Otherwise, it need not be.
+	Likewise, if the PARM_DECL is a param of the inlined function, it will
+	already have been remapped.  Otherwise, it might be a param of the
+	inlining function.  This can happen if a code block in a function is
+	first split off by fnsplit, and then inlined back into the same
+	function.  In that case, we don't remap.  */
       tree *n = id->decl_map->get (*tp);
       if (n)
 	*tp = *n;
diff --git a/libgomp/testsuite/libgomp.c/appendix-a/a.29.1-g.c b/libgomp/testsuite/libgomp.c/appendix-a/a.29.1-g.c
new file mode 100644
index 0000000..140f89f
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/appendix-a/a.29.1-g.c
@@ -0,0 +1,31 @@
+/* { dg-do run } */
+/* { dg-additional-options "-g" } */
+
+#include <assert.h>
+int A[2][2] = { 1, 2, 3, 4 };
+void
+f (int n, int B[n][n], int C[])
+{
+  int D[2][2] = { 1, 2, 3, 4 };
+  int E[n][n];
+  assert (n >= 2);
+  E[1][1] = 4;
+#pragma omp parallel firstprivate(B, C, D, E)
+  {
+    assert (sizeof (B) == sizeof (int (*)[n])); /* { dg-warning "on array function parameter" } */
+    assert (sizeof (C) == sizeof (int *)); /* { dg-warning "on array function parameter" } */
+    assert (sizeof (D) == 4 * sizeof (int));
+    assert (sizeof (E) == n * n * sizeof (int));
+    /* Private B and C have values of original B and C.  */
+    assert (&B[1][1] == &A[1][1]);
+    assert (&C[3] == &A[1][1]);
+    assert (D[1][1] == 4);
+    assert (E[1][1] == 4);
+  }
+}
+int
+main ()
+{
+  f (2, A, A[0]);
+  return 0;
+}
-- 
1.9.1

Re: [PATCH, PR66432] Handle PARM_DECL in remap_gimple_op_r

[Richard Biener]

On Wed, Jul 1, 2015 at 1:43 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> I.
>
> When running test libgomp.c/appendix-a/a.29.1.c with '--target_board
> unix/-O2/-g', we run into this failure:
> ...
> FAIL: libgomp.c/appendix-a/a.29.1.c (test for excess errors)
> Excess errors:
> src/libgomp/testsuite/libgomp.c/appendix-a/a.29.1.c:6:1: error: type
> mismatch between an SSA_NAME and its symbol
> ...
>
> Without -g, the testcase passes.
>
>
> II.
>
> The scenario for the failure is as follows:
>
> At fnsplit, we split off f.part.0 from f, which at source level looks like
> this:
> ...
> void
> f (int n, int B[n][n], int C[])
> {
>   int D[2][2] = { 1, 2, 3, 4 };
>   int E[n][n];
>   assert (n >= 2);
>   E[1][1] = 4;
> #pragma omp parallel firstprivate(B, C, D, E)
>   {
>     assert (sizeof (B) == sizeof (int (*)[n]));
>     assert (sizeof (C) == sizeof (int *));
>     assert (sizeof (D) == 4 * sizeof (int));
>     assert (sizeof (E) == n * n * sizeof (int));
>     /* Private B and C have values of original B and C. */
>     assert (&B[1][1] == &A[1][1]);
>     assert (&C[3] == &A[1][1]);
>     assert (D[1][1] == 4);
>     assert (E[1][1] == 4);
>   }
> }
> ...
>
> The split introduces a debug_insn and ssa-name that references param B in f:
> ...
>   # DEBUG D#4ptD.0 => B_3(D)
> ..
>
> And a debug_insn that references param B in f.part.0:
> ...
>   # DEBUG D#7ptD.0 s=> BD.1846
> ...
>
> At this point, the type of the ssa name and the param are the same.

With the same PARM_DECL?  I think that's the bug.

> Then at inline, we decide to inline f.part.0 back into function f.
>
> For inlining, we rewrite the body of inlined function f.part.0. While
> rewriting the debug insn mentioned above, we hit this code for param B:
> ...
>       if (TREE_CODE (*tp) != OMP_CLAUSE)
>         TREE_TYPE (*tp) = remap_type (TREE_TYPE (*tp), id);
> ...
> And since it's a variable-sized type, the type of param is changed.

The param of the inlined body but that should be unrelated to that refered to by

  # DEBUG D#4ptD.0 => B_3(D)

in f.

> Now the type of the ssa name and the param are no longer the same, and a bit
> later we hit the ICE.
>
>
> III.
>
> Attached patch fixes the ICE by handling PARM_DECL in remap_gimple_op_r.
> [ I'm not confident though this is the right fix ]
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk, or to be fixed otherwise?
>
> Thanks,
> - Tom

Re: [PATCH, PR66432] Handle PARM_DECL in remap_gimple_op_r

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

On 01/07/15 13:58, Richard Biener wrote:
> On Wed, Jul 1, 2015 at 1:43 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> Hi,
>>
>> I.
>>
>> When running test libgomp.c/appendix-a/a.29.1.c with '--target_board
>> unix/-O2/-g', we run into this failure:
>> ...
>> FAIL: libgomp.c/appendix-a/a.29.1.c (test for excess errors)
>> Excess errors:
>> src/libgomp/testsuite/libgomp.c/appendix-a/a.29.1.c:6:1: error: type
>> mismatch between an SSA_NAME and its symbol
>> ...
>>
>> Without -g, the testcase passes.
>>
>>
>> II.
>>
>> The scenario for the failure is as follows:
>>
>> At fnsplit, we split off f.part.0 from f, which at source level looks like
>> this:
>> ...
>> void
>> f (int n, int B[n][n], int C[])
>> {
>>    int D[2][2] = { 1, 2, 3, 4 };
>>    int E[n][n];
>>    assert (n >= 2);
>>    E[1][1] = 4;
>> #pragma omp parallel firstprivate(B, C, D, E)
>>    {
>>      assert (sizeof (B) == sizeof (int (*)[n]));
>>      assert (sizeof (C) == sizeof (int *));
>>      assert (sizeof (D) == 4 * sizeof (int));
>>      assert (sizeof (E) == n * n * sizeof (int));
>>      /* Private B and C have values of original B and C. */
>>      assert (&B[1][1] == &A[1][1]);
>>      assert (&C[3] == &A[1][1]);
>>      assert (D[1][1] == 4);
>>      assert (E[1][1] == 4);
>>    }
>> }
>> ...
>>
>> The split introduces a debug_insn and ssa-name that references param B in f:
>> ...
>>    # DEBUG D#4ptD.0 => B_3(D)
>> ..
>>
>> And a debug_insn that references param B in f.part.0:
>> ...
>>    # DEBUG D#7ptD.0 s=> BD.1846
>> ...
>>
>> At this point, the type of the ssa name and the param are the same.
>
> With the same PARM_DECL?  I think that's the bug.
>

Attached patch also fixes the ICE, by copying the PARM_DECL using in the 
debug insn. Does this look ok for testing?

Thanks,
- Tom

[-- Attachment #2: 0001-Copy-PARM_DECL-for-use-in-split-off-function.patch --]
[-- Type: text/x-patch, Size: 1338 bytes --]

Copy PARM_DECL for use in split off function.

2015-06-30  Tom de Vries  <tom@codesourcery.com>

	PR middle-end/66432
	* ipa-split.c (split_function): Copy PARM_DECL for use in split off
	function.

	* testsuite/libgomp.c/pr66432.c: New test.
---
 gcc/ipa-split.c                       | 2 +-
 libgomp/testsuite/libgomp.c/pr66432.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 libgomp/testsuite/libgomp.c/pr66432.c

diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 13d9a64..e923cee 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -1458,7 +1458,7 @@ split_function (basic_block return_bb, struct split_point *split_point,
 	  DECL_ARTIFICIAL (ddecl) = 1;
 	  TREE_TYPE (ddecl) = TREE_TYPE (parm);
 	  DECL_MODE (ddecl) = DECL_MODE (parm);
-	  vec_safe_push (*debug_args, DECL_ORIGIN (parm));
+	  vec_safe_push (*debug_args, copy_node (DECL_ORIGIN (parm)));
 	  vec_safe_push (*debug_args, ddecl);
 	  def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg),
 					      call);
diff --git a/libgomp/testsuite/libgomp.c/pr66432.c b/libgomp/testsuite/libgomp.c/pr66432.c
new file mode 100644
index 0000000..2259a69
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/pr66432.c
@@ -0,0 +1,4 @@
+/* { dg-do run } */
+/* { dg-additional-options "-g" } */
+
+#include "appendix-a/a.29.1.c"
-- 
1.9.1

Re: [PATCH, PR66432] Handle PARM_DECL in remap_gimple_op_r

[Richard Biener]

On Wed, Jul 1, 2015 at 7:09 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 01/07/15 13:58, Richard Biener wrote:
>>
>> On Wed, Jul 1, 2015 at 1:43 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> I.
>>>
>>> When running test libgomp.c/appendix-a/a.29.1.c with '--target_board
>>> unix/-O2/-g', we run into this failure:
>>> ...
>>> FAIL: libgomp.c/appendix-a/a.29.1.c (test for excess errors)
>>> Excess errors:
>>> src/libgomp/testsuite/libgomp.c/appendix-a/a.29.1.c:6:1: error: type
>>> mismatch between an SSA_NAME and its symbol
>>> ...
>>>
>>> Without -g, the testcase passes.
>>>
>>>
>>> II.
>>>
>>> The scenario for the failure is as follows:
>>>
>>> At fnsplit, we split off f.part.0 from f, which at source level looks
>>> like
>>> this:
>>> ...
>>> void
>>> f (int n, int B[n][n], int C[])
>>> {
>>>    int D[2][2] = { 1, 2, 3, 4 };
>>>    int E[n][n];
>>>    assert (n >= 2);
>>>    E[1][1] = 4;
>>> #pragma omp parallel firstprivate(B, C, D, E)
>>>    {
>>>      assert (sizeof (B) == sizeof (int (*)[n]));
>>>      assert (sizeof (C) == sizeof (int *));
>>>      assert (sizeof (D) == 4 * sizeof (int));
>>>      assert (sizeof (E) == n * n * sizeof (int));
>>>      /* Private B and C have values of original B and C. */
>>>      assert (&B[1][1] == &A[1][1]);
>>>      assert (&C[3] == &A[1][1]);
>>>      assert (D[1][1] == 4);
>>>      assert (E[1][1] == 4);
>>>    }
>>> }
>>> ...
>>>
>>> The split introduces a debug_insn and ssa-name that references param B in
>>> f:
>>> ...
>>>    # DEBUG D#4ptD.0 => B_3(D)
>>> ..
>>>
>>> And a debug_insn that references param B in f.part.0:
>>> ...
>>>    # DEBUG D#7ptD.0 s=> BD.1846
>>> ...
>>>
>>> At this point, the type of the ssa name and the param are the same.
>>
>>
>> With the same PARM_DECL?  I think that's the bug.
>>
>
> Attached patch also fixes the ICE, by copying the PARM_DECL using in the
> debug insn. Does this look ok for testing?

Hmm, it looks like it would break the purpose of this strange code.
It looks like
Jakub added this so CCing him for comments.

What should probably be done is to indeed copy the PARM_DECL for the reference
in the callee but make it have an abstract origin refering to the
PARM_DECL in the
caller?

Jakub did add guality tests - gcc.dg/guality/pr54519-?.c so you might want to
check whether that still passes after any change (and first check it still tests
what it is supposed to test, that is, ipa-split still applying and
removing a parameter)

Richard.

> Thanks,
> - Tom