Re: [PATCH] Improve store merging to handle load+store or bitwise logicals (PR tree-optimization/78821)

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, Nov 02, 2017 at 03:38:45PM +0000, Kyrill Tkachov wrote:
> this looks great! I have a couple of comments.
> * Can you please extend file comments for gimple-ssa-store-merging.c ?
> Currently it mostly describes how we merge constants together. Once we start
> accepting non-constant members
> we should mention it in there.

If you mean the file comment, yeah, I can try to adjust it/extend it.

> * Can we also handle BIT_NOT_EXPRESSIONS? i.e. Copying memory locations but
> but with a unary op applied on top.
> Don't know how often that comes up though. Maybe it will complicate
> store_operand_info and its handling too much
> to be worth it...

I guess we can.  If we only supported s.a = ~t.a; s.b = ~t.b;, it could be
handled very easily by adding support for BIT_NOT_EXPR as rhs_code with
a single load operand.  But, perhaps we want to also handle
s.a = t.a & ~u.a; s.b = t.b & ~u.b; and similar, in which case it would be
better to have a bool flag for BIT_NOT_EXPR on the load in
store_operand_info.  In any case, I'd prefer to handle this incrementally,
as I wrote, I want to also handle the aliasing issue.
> > +bool
> > +stmts_may_clobber_ref_p (gimple *first, gimple *last, tree ref)
> > +{
> > +  ao_ref r;
> > +  ao_ref_init (&r, ref);
> > +  unsigned int count = 0;
> > +  tree vop = gimple_vdef (last);
> > +  gimple *stmt;
> > +
> > +  gcc_checking_assert (gimple_bb (first) == gimple_bb (last));
> > +  do
> > +    {
> > +      stmt = SSA_NAME_DEF_STMT (vop);
> > +      if (stmt_may_clobber_ref_p_1 (stmt, &r))
> > +	return true;
> > +      if (++count > 64)
> > +	return true;
> 
> Magic number 64? Don't know if it's worth having a PARAM for it, but at
> least a comment saying we're bailing out
> for compile time considerations would be good.

I guess I could add a #define MAX_STORE_ALIAS_CHECKS 64 next to the other
two defines in the file.  Or go for a param.
> > +		  if (!integer_zerop (mask))
> > +		    TREE_NO_WARNING (ops[j]) = 1;
> 
> Please add a comment here as to why we need the TREE_NO_WARNING here.

There is a comment on the other TREE_NO_WARNING, but this one comes
first, I'm not against copying that comment here.

	Jakub