On Wed, 5 Aug 2015, Tom de Vries wrote:
> [ was: Re: Expand oacc kernels after pass_fre ]
> On 04/06/15 18:02, Tom de Vries wrote:
> > > Please move this out of the class body.
> > >
> >
> > Fixed and committed (ommitting patch as trivial).
> >
> > > > + {
> > > > + unsigned res = execute_expand_omp ();
> > > > +
> > > > + /* After running pass_expand_omp_ssa to expand the oacc kernels
> > > > + directive, we are left in the original function with anonymous
> > > > + SSA_NAMEs, with a defining statement that has been deleted. This
> > > > + pass finds those SSA_NAMEs and releases them.
> > > > + TODO: Either fix this elsewhere, or make the fix unnecessary. */
> > > > + unsigned int i;
> > > > + for (i = 1; i < num_ssa_names; ++i)
> > > > + {
> > > > + tree name = ssa_name (i);
> > > > + if (name == NULL_TREE)
> > > > + continue;
> > > > +
> > > > + gimple stmt = SSA_NAME_DEF_STMT (name);
> > > > + bool found = false;
> > > > +
> > > > + ssa_op_iter op_iter;
> > > > + def_operand_p def_p;
> > > > + FOR_EACH_PHI_OR_STMT_DEF (def_p, stmt, op_iter, SSA_OP_ALL_DEFS)
> > > > + {
> > > > + tree def = DEF_FROM_PTR (def_p);
> > > > + if (def == name)
> > > > + {
> > > > + found = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!found)
> > > > + {
> > > > + if (dump_file)
> > > > + fprintf (dump_file, "Released dangling ssa name %u\n", i);
> > > > + release_ssa_name (name);
> > > > + }
> > > > + }
> > > > +
> > > > + return res;
> > > > + }
>
> This patch implements the TODO.
>
> The cause of the problems is that in replace_ssa_name, we create a new
> ssa_name with the def stmt of the old ssa_name, but do not reset the def stmt
> of the old ssa_name, leaving the ssa_name in the original function having a
> def stmt in the split-off function.
>
> [ And if we don't do anything about that, at some point in another pass we use
> 'gimple_bb (SSA_NAME_DEF_STMT (name))->index' (a bb index in the split-off
> function) as an index into an array with as length the number of bbs in the
> original function. So the index may be out of bounds. ]
>
> This patch fixes that by making sure we reset the def stmt to NULL. This means
> we can simplify release_dangling_ssa_names to just test for NULL def stmts.
Not sure if I understand the problem correctly but why are you not simply
releasing the SSA name when you remove its definition?
Richard.
> Default defs are skipped by release_ssa_name, so setting the def stmt for
> default defs to NULL does not result in the name being released, but in an
> ssa-verification error. So instead, we keep the def stmt nop, and create a new
> nop for the copy in the split-off function.
>
> [ The default def bit seems only to be triggered for the default def created
> by expand_omp_target:
> ...
> /* If we are in ssa form, we must load the value from the default
> definition of the argument. That should not be defined now,
> since the argument is not used uninitialized. */
> gcc_assert (ssa_default_def (cfun, arg) == NULL);
> narg = make_ssa_name (arg, gimple_build_nop ());
> set_ssa_default_def (cfun, arg, narg);
> ...
> ]
>
> Bootstrapped and reg-tested on x86_64.
>
> Committed to gomp-4_0-branch.
>
> Thanks,
> - Tom
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
