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