On Mon, 30 May 2016, Tom de Vries wrote:
> On 30/05/16 12:56, Richard Biener wrote:
> > On Mon, 30 May 2016, Tom de Vries wrote:
> >
> > > >On 30/05/16 11:46, Richard Biener wrote:
> > > > > > > >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.
> > > >
> > > >Unfortunately, that doesn't work. We run into another assert in
> > > >copy_cond_phi_nodes:
> > > >...
> > > > /* Cond phi nodes should have exactly two arguments. */
> > > > gcc_assert (2 == EDGE_COUNT (bb->preds));
> > > >...
> > Hah. So can we still do my suggested change and bail out conservatively
> > in Cond PHI node handling instead? Because the PHI node is clearly
> > _not_ a loop header PHI and the cond phi assert is also a latent bug.
> >
>
> Agreed. Updated as attached.
>
> OK for trunk, 6 branch?
Ok with the now redundant 2nd check removed
@@ -1075,7 +1075,8 @@ bb_contains_loop_close_phi_nodes (basic_block bb)
static bool
bb_contains_loop_phi_nodes (basic_block bb)
{
- gcc_assert (EDGE_COUNT (bb->preds) <= 2);
+ if (EDGE_COUNT (bb->preds) != 2)
+ return false;
if (bb->preds->length () == 1)
return false;
^^^^^^^^^^
Thanks,
Richard.
> Thanks,
> - Tom
>
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
