On Thu, Jun 6, 2019 at 10:54 AM Feng Xue OS <f...@os.amperecomputing.com> wrote:
>
> > I don't think your PHI handling works correct. If you have
>
> > agg.part1 = 0;
> > if ()
> > agg.part2 = 1;
> > call (agg);
>
> > then you seem to end up above for agg.part2 because you push that onto the
> > worklist below.
>
> It is correct.
>
> o. worklist is used to collect all non-dom virtual operands. agg.part2 is
> collected
> into worklist, after it is taken from worklist, and processed, we find the
> next dom
> virtual operands agg.part1.
> [the statement "dom_vuse = vuse"].
>
> o. Execution transfers to the start of main loop, which processes dom virtual
> operands, and does overlap analysis between agg.part1 and agg.part2.
> [the statement if (!clobber_by_agg_contents_list_p())]
>
> >> + }
> >> + append.safe_push (gimple_vuse (stmt));
> >> + }
> >> + else
> >> + {
> >> + for (unsigned i = 0; i < gimple_phi_num_args (stmt); ++i)
> >> + {
> >> + tree phi_arg = gimple_phi_arg_def (stmt, i);
> >> +
> >> + if (SSA_NAME_IS_DEFAULT_DEF (phi_arg))
> >> + goto finish;
> >> +
> >> + append.safe_push (phi_arg);
>
> > Not sure why you first push to append and then move parts to the
> > worklist below - it seems to only complicate code.
>
> It is just a code trick. I do not want to duplicate below codes for both
> normal virtual SSA and PHI virtual SSA.
>
> >> + {
> >> + if (visited.add (vuse = append[i]))
> >> + continue;
> >> +
> >> + if (SSA_NAME_IS_DEFAULT_DEF (vuse)
> >> + || strictly_dominated_by_ssa_p (call_bb, vuse))
> >> + {
> >> + /* Found a new dom virtual operand, stop going further
> >> until
> >> + all pending non-dom virtual operands are processed.
> >> */
> >> + gcc_assert (!dom_vuse);
> >> + dom_vuse = vuse;
> >> + }
> >> + else
> >> + worklist.safe_push (vuse);
> >> + }
>
>
> > What you want to do is (probably) use get_continuation_for_phi
> > which for a virtual PHI node and a reference computes a
> > virtual operand you can continue your processing with. Thus sth
> > like
>
> I looked though the function. Yes, it does nearly same thing as this patch
> requires.
> But a minor difference, get_continuation_for_phi() does not translate virtual
> operands in back edge. Then, if rewriting with the function, we can not
> handle the
> following case.
>
> /* assume no overlap between agg.part1 and agg.part2 */
>
> __attribute__((pure)) call();
> agg.part1 = 0;
> for (...)
> {
> call(agg);
> agg.part2 = 1;
> }
>
> No sure this restrict is to prevent revisit the start virtual SSA or due to
> some other consideration.
It certainly can handle this situation, you do not need the "translate" hook.
Richard.
>
> Thanks for comments.
>
> Feng
