[PATCH] Find constant definition for by-ref argument using dominance information (PR ipa/90401)

Feng Xue OS fxue@os.amperecomputing.com
Thu Jun 6 08:54:00 GMT 2019


> 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.

Thanks for comments.

Feng


More information about the Gcc-patches mailing list