On Thu, Nov 11, 2021 at 1:07 PM Jan Hubicka <hubi...@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))
> > > {
