Re: [PATCH, PR69068] Handle 3-arg phi in copy_bb_and_scalar_dependences

[Richard Biener <rguenther at suse dot de>]

On Mon, 30 May 2016, Tom de Vries wrote:

> Hi,
> 
> this patch fixes graphite PR69068, a 6/7 regression.
> 
> 
> I.
> 
> Consider this test-case:
> ...
> int qo;
> int zh[2];
> 
> void
> td (void)
> {
>   int ly, en;
>   for (ly = 0; ly < 2; ++ly)
>     for (en = 0; en < 2; ++en)
>       zh[en] = ((qo == 0) || (((qo * 2) != 0))) ? 1 : -1;
> }
> ...
> 
> When compiling with -O1 -fgraphite-identity, we run into an assert in
> bb_contains_loop_phi_nodes:
> ...
> pr-graphite.c: In function âtdâ:
> pr-graphite.c:5:1: internal compiler error: in bb_contains_loop_phi_nodes, at
> graphite-isl-ast-to-gimple.c:1078
> td(void)
> ^~
> ...
> 
> 
> II.
> 
> At graphite0, we have a 3 argument phi in bb 7:
> ...
> td ()
> {
>   int en;
>   int ly;
>   int qo.1_1;
>   int _3;
>   int iftmp.0_6;
> 
>   <bb 2>:
>   qo.1_1 = qo;
>   _3 = qo.1_1 * 2;
>   goto <bb 10>;
> 
>   <bb 3>:
> 
>   <bb 4>:
>   # en_19 = PHI <0(10), en_12(3)>
>   if (qo.1_1 == 0)
>     goto <bb 7>;
>   else
>     goto <bb 5>;
> 
>   <bb 5>:
>   if (_3 != 0)
>     goto <bb 6>;
>   else
>     goto <bb 7>;
> 
>   <bb 6>:
> 
>   <bb 7>:
>   # iftmp.0_6 = PHI <1(6), -1(5), 1(4)>
>   zh[en_19] = iftmp.0_6;
>   en_12 = en_19 + 1;
>   if (en_12 <= 1)
>     goto <bb 3>;
>   else
>     goto <bb 8>;
> 
>   <bb 8>:
>   ly_13 = ly_18 + 1;
>   if (ly_13 <= 1)
>     goto <bb 9>;
>   else
>     goto <bb 11>;
> 
>   <bb 9>:
> 
>   <bb 10>:
>   # ly_18 = PHI <ly_13(9), 0(2)>
>   goto <bb 4>;
> 
>   <bb 11>:
>   return;
> 
> }
> ...
> 
> When instantiating the gimple from the graphite ast, we arrive at
> copy_bb_and_scalar_dependences with bb.index == 7, where we execute:
> ...
>       if (num_phis > 0 && bb_contains_loop_phi_nodes (bb))
>         {
> ...
> 
> Subsequently we run into this assert, because
> EDGE_COUNT (bb->preds) == 3:
> ...
> /* Return true when BB contains loop phi nodes.  A loop phi node is the
>    loop header containing phi nodes which has one init-edge and one
>    back-edge.  */
> 
> static bool
> bb_contains_loop_phi_nodes (basic_block bb)
> {
>   gcc_assert (EDGE_COUNT (bb->preds) <= 2);
> ...
> 
> 
> III.
> 
> This patch fixes the assert conservatively by aborting graphite code
> generation when encountering a phi with more than two arguments in
> copy_bb_and_scalar_dependences.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk, 6 branch?

Did you check if simply returning false from bb_contains_loop_phi_nodes
instead of asserting works?  The caller has a 'else' that is supposed
to handle condition PHIs.  After all it already handles one predecessor
specially ...  Thus

  if (EDGE_COUNT (bb->preds) != 2)
    return false;

should work here.  Or replace this function with bb_loop_header_p (bb)
(works w/o loop info, the function seems to depend on loop info and
thus simply checking bb->loop_father->header == bb should work as well).

Thanks,
Richard.