Handle PHI nodes w/o a argument (PR ipa/80205).
diff mbox

Message ID CAFiYyc1wRxXfDjtKa97wtgTdPUQF9B1v3EWoLQWAAFOBFWfwCA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener March 27, 2017, 2:27 p.m. UTC
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.



>> Martin

Comments

Jeff Law March 27, 2017, 3:21 p.m. UTC | #1
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
Martin Liška March 28, 2017, 7:45 a.m. UTC | #2
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
Richard Biener March 28, 2017, 8:12 a.m. UTC | #3
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
>

Patch
diff mbox

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);
            }
+           }
        }
     }