[PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Martin Liška]

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

Hello.

As described in the PR, we can create a PHI node in einline that has no argument.
That can cause ICE in devirtualization and should be thus handled.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

[-- Attachment #2: 0001-Handle-PHI-nodes-w-o-a-argument-PR-ipa-80205.patch --]
[-- Type: text/x-patch, Size: 1848 bytes --]

From d0dc319a8df5d9f00434f54fef13b3dc427061e1 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 27 Mar 2017 13:32:52 +0200
Subject: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

gcc/testsuite/ChangeLog:

2017-03-27  Martin Liska  <mliska@suse.cz>

	* g++.dg/ipa/pr80205.C: New test.

gcc/ChangeLog:

2017-03-27  Martin Liska  <mliska@suse.cz>

	PR ipa/80205
	* ipa-polymorphic-call.c (walk_ssa_copies): Handle phi nodes
	w/o a argument.
---
 gcc/ipa-polymorphic-call.c         |  2 +-
 gcc/testsuite/g++.dg/ipa/pr80205.C | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr80205.C

diff --git a/gcc/ipa-polymorphic-call.c b/gcc/ipa-polymorphic-call.c
index e690d05158d..89c008ee5c0 100644
--- a/gcc/ipa-polymorphic-call.c
+++ b/gcc/ipa-polymorphic-call.c
@@ -828,7 +828,7 @@ walk_ssa_copies (tree op, hash_set<tree> **global_visited = NULL)
 	{
 	  gimple *phi = SSA_NAME_DEF_STMT (op);
 
-	  if (gimple_phi_num_args (phi) > 2)
+	  if (gimple_phi_num_args (phi) == 0 || gimple_phi_num_args (phi) > 2)
 	    goto done;
 	  if (gimple_phi_num_args (phi) == 1)
 	    op = gimple_phi_arg_def (phi, 0);
diff --git a/gcc/testsuite/g++.dg/ipa/pr80205.C b/gcc/testsuite/g++.dg/ipa/pr80205.C
new file mode 100644
index 00000000000..460bdcb02ca
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr80205.C
@@ -0,0 +1,34 @@
+// PR ipa/80205
+// { dg-options "-fnon-call-exceptions --param early-inlining-insns=100 -O2" }
+
+class a
+{
+public:
+  virtual ~a ();
+};
+class b
+{
+public:
+  template <typename c> b (c);
+  ~b () { delete d; }
+  void
+  operator= (b e)
+  {
+    b (e).f (*this);
+  }
+  void
+  f (b &e)
+  {
+    a g;
+    d = e.d;
+    e.d = &g;
+  }
+  a *d;
+};
+void
+h ()
+{
+  b i = int();
+  void j ();
+  i = j;
+}
-- 
2.12.0

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Richard Biener]

On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> As described in the PR, we can create a PHI node in einline that has no argument.
> That can cause ICE in devirtualization and should be thus handled.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?

We shouldn't ever have a PHI w/o argument.

> Martin

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Jeff Law]

On 03/27/2017 08:14 AM, Richard Biener wrote:
> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
>> Hello.
>>
>> As described in the PR, we can create a PHI node in einline that has no argument.
>> That can cause ICE in devirtualization and should be thus handled.
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>
> We shouldn't ever have a PHI w/o argument.
Yea.  Sounds like there's a deeper problem somewhere.

jeff

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Richard Biener]

On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
>> Hello.
>>
>> As described in the PR, we can create a PHI node in einline that has no argument.
>> That can cause ICE in devirtualization and should be thus handled.
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>
> We shouldn't ever have a PHI w/o argument.

;;   basic block 14, loop depth 0
;;    pred:
  # SR.2_19 = PHI <>
<L4> [0.00%]:
  a::~a (&g);
  resx 12
;;    succ:       17

the CFG has not been cleaned up here (this block is unreachable).

Hmm, I see we are called from fold_stmt.  I suppose we process
statements_to_fold before removing unreachable blocks to not
walk over dead stmts...  chicken-and-egg...

Still not creating that PHI should be possible.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c   (revision 246500)
+++ gcc/tree-inline.c   (working copy)
@@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b
       if (!virtual_operand_p (res))
        {
          walk_tree (&new_res, copy_tree_body_r, id, NULL);
+         if (EDGE_COUNT (new_bb->preds) == 0)
+           {
+             /* Technically we'd want a SSA_DEFAULT_DEF here... */
+             SSA_NAME_DEF_STMT (new_res) = gimple_build_nop ();
+           }
+         else
+           {
          new_phi = create_phi_node (new_res, new_bb);
          FOR_EACH_EDGE (new_edge, ei, new_bb->preds)
            {
@@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b

              add_phi_arg (new_phi, new_arg, new_edge, locus);
            }
+           }
        }
     }



>> Martin

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Jeff Law]

On 03/27/2017 08:27 AM, Richard Biener wrote:
> On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
>>> Hello.
>>>
>>> As described in the PR, we can create a PHI node in einline that has no argument.
>>> That can cause ICE in devirtualization and should be thus handled.
>>>
>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>
>>> Ready to be installed?
>>
>> We shouldn't ever have a PHI w/o argument.
>
> ;;   basic block 14, loop depth 0
> ;;    pred:
>   # SR.2_19 = PHI <>
> <L4> [0.00%]:
>   a::~a (&g);
>   resx 12
> ;;    succ:       17
>
> the CFG has not been cleaned up here (this block is unreachable).
>
> Hmm, I see we are called from fold_stmt.  I suppose we process
> statements_to_fold before removing unreachable blocks to not
> walk over dead stmts...  chicken-and-egg...
>
> Still not creating that PHI should be possible.
Can't speak for this specific example, but could it be the case that the 
PHI exists with arguments, then becomes unreachable resulting in a PHI 
with no arguments?

jeff

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Martin Liška]

On 03/27/2017 04:27 PM, Richard Biener wrote:
> On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
>>> Hello.
>>>
>>> As described in the PR, we can create a PHI node in einline that has no argument.
>>> That can cause ICE in devirtualization and should be thus handled.
>>>
>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>
>>> Ready to be installed?
>>
>> We shouldn't ever have a PHI w/o argument.
> 
> ;;   basic block 14, loop depth 0
> ;;    pred:
>   # SR.2_19 = PHI <>
> <L4> [0.00%]:
>   a::~a (&g);
>   resx 12
> ;;    succ:       17
> 
> the CFG has not been cleaned up here (this block is unreachable).
> 
> Hmm, I see we are called from fold_stmt.  I suppose we process
> statements_to_fold before removing unreachable blocks to not
> walk over dead stmts...  chicken-and-egg...
> 
> Still not creating that PHI should be possible.

I see, thanks for help. Patch fixes the issue, may I install it after
regression tested?

Thanks,
Martin

> 
> Index: gcc/tree-inline.c
> ===================================================================
> --- gcc/tree-inline.c   (revision 246500)
> +++ gcc/tree-inline.c   (working copy)
> @@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b
>        if (!virtual_operand_p (res))
>         {
>           walk_tree (&new_res, copy_tree_body_r, id, NULL);
> +         if (EDGE_COUNT (new_bb->preds) == 0)
> +           {
> +             /* Technically we'd want a SSA_DEFAULT_DEF here... */
> +             SSA_NAME_DEF_STMT (new_res) = gimple_build_nop ();
> +           }
> +         else
> +           {
>           new_phi = create_phi_node (new_res, new_bb);
>           FOR_EACH_EDGE (new_edge, ei, new_bb->preds)
>             {
> @@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b
> 
>               add_phi_arg (new_phi, new_arg, new_edge, locus);
>             }
> +           }
>         }
>      }
> 
> 
> 
>>> Martin

Re: [PATCH] Handle PHI nodes w/o a argument (PR ipa/80205).

[Richard Biener]

On Tue, Mar 28, 2017 at 9:45 AM, Martin Liška <mliska@suse.cz> wrote:
> On 03/27/2017 04:27 PM, Richard Biener wrote:
>> On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote:
>>>> Hello.
>>>>
>>>> As described in the PR, we can create a PHI node in einline that has no argument.
>>>> That can cause ICE in devirtualization and should be thus handled.
>>>>
>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>
>>> We shouldn't ever have a PHI w/o argument.
>>
>> ;;   basic block 14, loop depth 0
>> ;;    pred:
>>   # SR.2_19 = PHI <>
>> <L4> [0.00%]:
>>   a::~a (&g);
>>   resx 12
>> ;;    succ:       17
>>
>> the CFG has not been cleaned up here (this block is unreachable).
>>
>> Hmm, I see we are called from fold_stmt.  I suppose we process
>> statements_to_fold before removing unreachable blocks to not
>> walk over dead stmts...  chicken-and-egg...
>>
>> Still not creating that PHI should be possible.
>
> I see, thanks for help. Patch fixes the issue, may I install it after
> regression tested?

I think it's progression, still not 100% safe as we have a SSA name with
a GIMPLE_NOP definition that is not in any BB and the SSA name is not
a default def.

But yes, I've written a comment to that effect ;)

Thus ok.

An improvement would be to not copy unreachable regions at all...

Thanks,
Richard.

> Thanks,
> Martin
>
>>
>> Index: gcc/tree-inline.c
>> ===================================================================
>> --- gcc/tree-inline.c   (revision 246500)
>> +++ gcc/tree-inline.c   (working copy)
>> @@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b
>>        if (!virtual_operand_p (res))
>>         {
>>           walk_tree (&new_res, copy_tree_body_r, id, NULL);
>> +         if (EDGE_COUNT (new_bb->preds) == 0)
>> +           {
>> +             /* Technically we'd want a SSA_DEFAULT_DEF here... */
>> +             SSA_NAME_DEF_STMT (new_res) = gimple_build_nop ();
>> +           }
>> +         else
>> +           {
>>           new_phi = create_phi_node (new_res, new_bb);
>>           FOR_EACH_EDGE (new_edge, ei, new_bb->preds)
>>             {
>> @@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b
>>
>>               add_phi_arg (new_phi, new_arg, new_edge, locus);
>>             }
>> +           }
>>         }
>>      }
>>
>>
>>
>>>> Martin
>