Re: [gimplefe] Parsing PHI functions

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 4, 2016 at 8:58 PM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote:
> On 4 July 2016 at 15:17, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Sun, Jul 3, 2016 at 9:34 AM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote:
>>> In this patch, I am passing labels and vars with internal function and
>>> handling them in tree-cfg for parsing PHI.
>>> For first label, label_to_block() gives correct basic block and I am
>>> getting proper edges but for other labels the function is giving NULL.
>>>
>>> test-case :
>>>
>>> void __GIMPLE () foo()
>>> {
>>>   int a;
>>>   int a_2;
>>>   int a_3;
>>>   int a_1;
>>>
>>> bb_2:
>>>   a_2 = 3;
>>>   goto bb_4;
>>>
>>> bb_3:
>>>   a_3 = 6;
>>>   goto bb_4;
>>>
>>> bb_4:
>>>   a_1 = __PHI (bb_2: a_2, bb_3: a_3);
>>>   a_1 = a_1 + 1;
>>>   return;
>>> }
>>>
>>
>> The issue is probably you lower PHIs after cleanup_tree_cfg is run which may
>> end up removing labels.  Or it is cleanup_dead_labels in build_gimple_cfg.
>
> Yes, that was due to cleanup_dead_label. It is working after calling
> lower_phi_internal_fn just after make_edges
>
> So test case like:
>
> void __GIMPLE () foo()
> {
>   int a;
>   int a_2;
>   int a_3;
>   int a_1;
>   int a_0;
>
> bb_2:
>   if (a > 4)
>     goto bb_3;
>   else
>     goto bb_4;
>
> bb_3:
>   a_2 = 6;
>   goto bb_5;
>
> bb_4:
>   a_3 = 99;
>   goto bb_5;
>
> bb_5:
>   a_1 = __PHI (bb_3: a_2, bb_4: a_3);
>   a_0 = a_1 + 1;
>   return;
> }
>
> produces ssa dump as
>
> ;; Function foo (foo, funcdef_no=0, decl_uid=1744, cgraph_uid=0, symbol_order=0)
>
> foo ()
> {
>   int a_0;
>   int a_1;
>   int a_3;
>   int a_2;
>   int a;
>
> bb_2:
>   if (a_2(D) > 4)
>     goto <bb 3> (bb_3);
>   else
>     goto <bb 4> (bb_4);
>
> bb_3:
>   a_2_3 = 6;
>   goto <bb 5> (bb_5);
>
> bb_4:
>   a_3_5 = 99;
>
>   # a_1_1 = PHI <a_1_4(D)(3), a_1_4(D)(4)>
> bb_5:
>   a_0_6 = a_1_1 + 1;
>   return;
>
> }
>
>
> and test case like :
>
> void __GIMPLE () foo()
> {
>   int a;
>   int a_2;
>   int a_3;
>   int a_1;
>   int a_0;
>
> bb_3:
>   a_2 = 6;
>   goto bb_5;
>
> bb_4:
>   a_3 = 99;
>   goto bb_5;
>
> bb_5:
>   a_1 = __PHI (bb_3: a_2, bb_4: a_3);
>   a_0 = a_1 + 1;
>   return;
> }
>
> gets its block with bb_4 label removed as it is unreachable and
> produces ssa dump as -
>
> ;; Function foo (foo, funcdef_no=0, decl_uid=1744, cgraph_uid=0, symbol_order=0)
>
> foo ()
> {
>   int a_0;
>   int a_1;
>   int a_3;
>   int a_2;
>   int a;
>
> bb_3:
>   a_2_2 = 6;
>
>   # a_1_1 = PHI <a_1_3(D)(2)>
> bb_5:
>   a_0_4 = a_1_1 + 1;
>   return;
>
> }
>
> I think which is fine(?).

Yes, that is fine.  What is not is that the into-SSA rewrite phase picks up
bogus current-defs for the PHIs.  This is due to several things I think - first
of all create_phi_node creating a SSA def (a_1_1 rather than a_1) which
seems to do no harm, second by us sticking non-SSA uses into the PHI
which seems to confuse renaming somehow as it expects the same base
variable in the args as the result (this is how the SSA renamer produces
things itself).

See tree-into-ssa.c:rewrite_add_phi_arguments

The following fixes this for me:

diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
index f83cad2..8d962ea 100644
--- a/gcc/tree-into-ssa.c
+++ b/gcc/tree-into-ssa.c
@@ -1378,12 +1378,18 @@ rewrite_add_phi_arguments (basic_block bb)
       for (gsi = gsi_start_phis (e->dest); !gsi_end_p (gsi);
           gsi_next (&gsi))
        {
-         tree currdef, res;
+         tree currdef, res, argvar;
          location_t loc;

          phi = gsi.phi ();
          res = gimple_phi_result (phi);
-         currdef = get_reaching_def (SSA_NAME_VAR (res));
+         /* If we have a pre-existing PHI its args may be different
+            vars than the result var.  */
+         argvar = gimple_phi_arg_def (phi, e->dest_idx);
+         gcc_assert (! argvar || TREE_CODE (argvar) != SSA_NAME);
+         if (! argvar)
+           argvar = SSA_NAME_VAR (res);
+         currdef = get_reaching_def (argvar);
          /* Virtual operand PHI args do not need a location.  */
          if (virtual_operand_p (res))
            loc = UNKNOWN_LOCATION;

and then generates the expected

foo ()
{
  int a_0;
  int a_1;
  int a_3;
  int a_2;
  int a;

bb_3:
  a_2_2 = 6;

  # a_1_1 = PHI <a_2_2(2)>
bb_5:
  a_0_3 = a_1_1 + 1;
  return;

}

Richard.

> Now ssa pass is producing ssa names for the vars because it expects
> them with tree code SSA_NAME. How can we prevent this? Can we convert
> VAR_DECL to SSA_NAME ?
>
>>
>> Richard.
>>
>>>
>>>
>>>
>>> On 1 July 2016 at 17:33, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> On Fri, Jul 1, 2016 at 1:57 PM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote:
>>>>> On 29 June 2016 at 12:42, Richard Biener <richard.guenther@gmail.com> wrote:
>>>>>> On Tue, Jun 28, 2016 at 4:16 PM, Prasad Ghangal
>>>>>> <prasad.ghangal@gmail.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> For handling PHI, it expects cfg to be built before. So I was
>>>>>>> wondering how are we going to handle this? Do we need to build cfg
>>>>>>> while parsing only?
>>>>>>
>>>>>> For handling PHIs we need to have a CFG in the sense that the GIMPLE PHI
>>>>>> data structures are built in a way to have the PHI argument index correspond
>>>>>> to CFG predecessor edge index.  As we'd like to parse phis with args
>>>>>> corresponding
>>>>>> to predecessor block labels, like
>>>>>>
>>>>>> a:
>>>>>>   i_1 = 1;
>>>>>>   goto p;
>>>>>>
>>>>>> b:
>>>>>>   i_2 = 2;
>>>>>>   goto p;
>>>>>>
>>>>>> p:
>>>>>>   i_3 = __PHI (a: i_1, b: i_2);
>>>>>>
>>>>>> I think that a possibility is to leave those PHIs as internal function
>>>>>> with label / arg
>>>>>> pairs and have CFG construction lower them to real PHIs.
>>>>>>
>>>>>> Of course the parser could as well build a CFG on its own but I think
>>>>>> we should use
>>>>>> the easy way out for now.
>>>>>>
>>>>>> Thus you'd have to modify CFG construction a bit to lower the internal
>>>>>> function calls.
>>>>>
>>>>> Currently I am just building internal call using
>>>>> gimple_build_call_internal_vec (), and detecting (and removing for
>>>>> now) it after edge creation in tree-cfg. I was observing
>>>>> internal-fn.def, do I need to make entry in internal-fn.def and write
>>>>> expand function?
>>>>
>>>> You should add an entry and a stub expand function (like those
>>>> others that simply have gcc_unreachable in them).
>>>>
>>>> Richard.
>>>>
>>>>>>
>>>>>> Richard.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Prasad