Re: Unnecessary check on phi node in tree if-conversion?

[Jeff Law <law at redhat dot com>]

On 04/06/2016 12:21 PM, Bin.Cheng wrote:
> On Wed, Apr 6, 2016 at 5:07 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> > Hi,
> > Function if_convertible_phi_p has below check on virtual PHI nodes:
> >
> >
> >    if (any_mask_load_store)
> >      return true;
> >
> >    /* When there were no if-convertible stores, check
> >       that there are no memory writes in the branches of the loop to be
> >       if-converted.  */
> >    if (virtual_operand_p (gimple_phi_result (phi)))
> >      {
> >        imm_use_iterator imm_iter;
> >        use_operand_p use_p;
> >
> >        if (bb != loop->header)
> >      {
> >        if (dump_file && (dump_flags & TDF_DETAILS))
> >          fprintf (dump_file, "Virtual phi not on loop->header.\n");
> >        return false;
> >      }
> >
> >        FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi))
> >      {
> >        if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI
> >            && USE_STMT (use_p) != phi)
> >          {
> >            if (dump_file && (dump_flags & TDF_DETAILS))
> >          fprintf (dump_file, "Difficult to handle this virtual phi.\n");
> >            return false;
> >          }
> >      }
> >      }
> >
> > Since the check is short-cut by any_mask_load_store, when the check is
> > triggered, it means there is virtual phi node but no conditional
> > memory stores?  Is this possible?  Plus, any_mask_load_store is set by
> > below code in if_convertible_gimple_assign_stmt_p:
> >        if (ifcvt_can_use_mask_load_store (stmt))
> >      {
> >        gimple_set_plf (stmt, GF_PLF_2, true);
> >        *any_mask_load_store = true;
> >        return true;
> >      }
> >
> > So in theory it's possible to skip aforementioned check when only mask
> > load is encountered.
> >
> > Any ideas?
> It's possible to have a loop like:
>
> <bb header>
>    .MEM_2232 = PHI <.MEM_574(179), .MEM_1247(183)>
>    ...
>    if (cond)
>      goto <bb 1>
>    else
>      goto <bb2>
>
> <bb 1>:  //empty
> <bb2>:
>    .MEM_1247 = PHI <.MEM_2232(180), .MEM_2232(181)>
>    if (cond2)
>      goto <bb exit>
>    else
>      goto <bb latch>
>
> <bb latch>:
>    goto <bb header>
>
> So can we handle the PHI which can be degenerated in if-cvt?
One could argue that the uses of MEM_1247 ought to be propagated away 
which would result in:

<bb header>
  .MEM_2232 = PHI <.MEM_574(179), .MEM_2232(183)>
  ...
  if (cond)
    goto <bb 1>
  else
    goto <bb2>

<bb 1>:  //empty
<bb2>:
  if (cond2)
    goto <bb exit>
  else
    goto <bb latch>

<bb latch>:
  goto <bb header>


At which point we ought to be able to propagate away uses of MEM_2232 
(using MEM_574 instead) and remove the now dead PHI node.

So I might start with looking at what's leaving the degenerates and 
whether or not that can be fixed first rather than hacking if-conversion 
to handle this stuff.

jeff