Use modref summary to DSE calls to non-pure functions

[Richard Biener]

On Thu, Nov 11, 2021 at 1:07 PM Jan Hubicka <hubicka@kam.mff.cuni.cz> wrote:
>
> > > +  /* Unlike alias oracle we can not skip subtrees based on TBAA check.
> > > +     Count the size of the whole tree to verify that we will not need too many
> > > +     tests.  */
> > > +  FOR_EACH_VEC_SAFE_ELT (summary->stores->bases, i, base_node)
> > > +    FOR_EACH_VEC_SAFE_ELT (base_node->refs, j, ref_node)
> > > +      FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node)
> > > +       if (num_tests++ > max_tests)
> > > +         return false;
> >
> > at least the innermost loop can be done as
> >
> >           if (num_tests += ref_node->accesses.length () > max_tests)
> >
> > no?
>
> Yep that was stupid, sorry for that ;))
> >
> > > +
> > > +  /* Walk all memory writes and verify that they are dead.  */
> > > +  FOR_EACH_VEC_SAFE_ELT (summary->stores->bases, i, base_node)
> > > +    FOR_EACH_VEC_SAFE_ELT (base_node->refs, j, ref_node)
> > > +      FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node)
> > > +       {
> > > +         /* ??? if offset is unkonwn it may be negative.  Not sure
> > > +            how to construct ref here.  */
> >
> > I think you can't, you could use -poly_int64_max or so.
>
> I need a ref to give to dse_classify_store. It needs base to track live
> bytes etc which is not very useful if I do not know the range.  However
> DSE is still useful since I can hit free or end of lifetime of the decl.
> I was wondering if I should simply implement a lightweight version of
> dse_clasify_store that handles this case?

No, I think if it turns out useful then we want a way to have such ref
represented by an ao_ref.  Note that when we come from a
ref tree we know handled-components only will increase offset,
only the base MEM_REF can contain a pointer subtraction (but
the result of that is the base then).

In what cases does parm_offset_known end up false?  Is that
when seeing a POINTER_PLUS_EXPR with unknown offset?
So yes, that's a case we cannot capture right now - the only
thing that remains is a pointer with a known points-to-set - a
similar problem as with the pure call PRE.  You could in theory
allocate a scratch SSA name and attach points-to-info
to it.  And when the call argument is &decl based then you could set
offset to zero.

> >
> > > +         if (!access_node->parm_offset_known)
> > > +           return false;
> >
> > But you could do this check in the loop computing num_tests ...
> > (we could also cache the count and whether any of the refs have unknown offset
> > in the summary?)
>
> Yep, I plan to add cache for bits like this (and the check for accessing
> global memory).  Just want to push bit more of the cleanups I have in my
> local tree.
> >
> > > +         tree arg;
> > > +         if (access_node->parm_index == MODREF_STATIC_CHAIN_PARM)
> > > +           arg = gimple_call_chain (stmt);
> > > +         else
> > > +           arg = gimple_call_arg (stmt, access_node->parm_index);
> > > +
> > > +         ao_ref ref;
> > > +         poly_offset_int off = (poly_offset_int)access_node->offset
> > > +               + ((poly_offset_int)access_node->parm_offset
> > > +                  << LOG2_BITS_PER_UNIT);
> > > +         poly_int64 off2;
> > > +         if (!off.to_shwi (&off2))
> > > +           return false;
> > > +         ao_ref_init_from_ptr_and_range
> > > +                (&ref, arg, true, off2, access_node->size,
> > > +                 access_node->max_size);
> > > +         ref.ref_alias_set = ref_node->ref;
> > > +         ref.base_alias_set = base_node->base;
> > > +
> > > +         bool byte_tracking_enabled
> > > +             = setup_live_bytes_from_ref (&ref, live_bytes);
> > > +         enum dse_store_status store_status;
> > > +
> > > +         store_status = dse_classify_store (&ref, stmt,
> > > +                                            byte_tracking_enabled,
> > > +                                            live_bytes, &by_clobber_p);
> > > +         if (store_status != DSE_STORE_DEAD)
> > > +           return false;
> > > +       }
> > > +  /* Check also value stored by the call.  */
> > > +  if (gimple_store_p (stmt))
> > > +    {
> > > +      ao_ref ref;
> > > +
> > > +      if (!initialize_ao_ref_for_dse (stmt, &ref))
> > > +       gcc_unreachable ();
> > > +      bool byte_tracking_enabled
> > > +         = setup_live_bytes_from_ref (&ref, live_bytes);
> > > +      enum dse_store_status store_status;
> > > +
> > > +      store_status = dse_classify_store (&ref, stmt,
> > > +                                        byte_tracking_enabled,
> > > +                                        live_bytes, &by_clobber_p);
> > > +      if (store_status != DSE_STORE_DEAD)
> > > +       return false;
> > > +    }
> > > +  delete_dead_or_redundant_assignment (gsi, "dead", need_eh_cleanup);
> > > +  return true;
> > > +}
> > > +
> > >  namespace {
> > >
> > >  const pass_data pass_data_dse =
> > > @@ -1235,7 +1363,14 @@ pass_dse::execute (function *fun)
> > >           gimple *stmt = gsi_stmt (gsi);
> > >
> > >           if (gimple_vdef (stmt))
> > > -           dse_optimize_stmt (fun, &gsi, live_bytes);
> > > +           {
> > > +             gcall *call = dyn_cast <gcall *> (stmt);
> > > +
> > > +             if (call && dse_optimize_call (&gsi, live_bytes))
> > > +               /* We removed a dead call.  */;
> > > +             else
> > > +               dse_optimize_store (fun, &gsi, live_bytes);
> >
> > I think we want to refactor both functions, dse_optimize_stmt has some
> > early outs that apply generally, and it handles some builtin calls
> > that we don't want to re-handle with dse_optimize_call.
> >
> > So I wonder if it is either possible to call the new function from
> > inside dse_optimize_stmt instead, after we handled the return
> > value of call for example or different refactoring can make the flow
> > more obvious.
>
> It was my initial plan. However I was not sure how much I would get from
> that.
>
> The function starts with:
>
>   /* Don't return early on *this_2(D) ={v} {CLOBBER}.  */
>   if (gimple_has_volatile_ops (stmt)
>       && (!gimple_clobber_p (stmt)
>           || TREE_CODE (gimple_assign_lhs (stmt)) != MEM_REF))
>     return;
>
>   ao_ref ref;
>   if (!initialize_ao_ref_for_dse (stmt, &ref))
>     return;
>
> The check about clobber does not apply to calls and then it gives up on
> functions not returning aggregates (that is a common case).
>
> For functions returing aggregates it tries to prove that retval is dead
> and replace it.
>
> I guess I can simply call my analysis from the second return above and
> from the code removing dead LHS call instead of doing it from the main
> walker and drop the LHS handling?

Yeah, something like that.

Richard.

> Thank you,
> Honza
> >
> > Thanks,
> > Richard.
> >
> > > +           }
> > >           else if (def_operand_p
> > >                      def_p = single_ssa_def_operand (stmt, SSA_OP_DEF))
> > >             {