On Tue, 13 Dec 2016, Jakub Jelinek wrote:
> Hi!
>
> Sorry for not getting to this earlier.
>
> On Mon, Nov 28, 2016 at 10:50:26AM +0100, Richard Biener wrote:
> > > + else if (gimple_call_builtin_p (defstmt, BUILT_IN_MEMSET)
> > > + && TREE_CODE (gimple_call_arg (defstmt, 0)) == ADDR_EXPR
> > > + && TREE_CODE (gimple_call_arg (defstmt, 1)) == INTEGER_CST
> > > + && TREE_CODE (gimple_call_arg (defstmt, 2)) == INTEGER_CST)
> > > + {
> > > + HOST_WIDE_INT ssize, max_size, off;
> > > + bool reverse;
> > > + src2 = TREE_OPERAND (gimple_call_arg (defstmt, 0), 0);
> > > + get_ref_base_and_extent (src2, &off, &ssize, &max_size, &reverse);
> > > + if (ssize != max_size
> > > + || (ssize % BITS_PER_UNIT) != 0
> > > + || !wi::eq_p (gimple_call_arg (defstmt, 2), ssize / BITS_PER_UNIT))
> > > + src2 = NULL_TREE;
> >
> > I wonder why you jump through the hoops of get_ref_base_and_extent
> > given the call args will be invariant addresses and thus
> > get_addr_base_and_unit_offset would be more appropriate here.
>
> get_addr_base_and_unit_offset does not give me the size though,
> which is what I wanted to compute. Even if as you suggest I'd
> accept other INTEGER_CST sizes, it would still be better to punt
> if the memset is clearly invalid. And for the case where the
> memset call is followed by assignment, not memcpy (very common, as
> memcpy is often folded into the assignment), the verification needs
> to be done.
>
> > Also not sure why you want to restrict the size with the wi::eq_p
> > (probably for the a = b case where the size isn't given explicitely
> > but then you don't check whether off is 0 ...). I'd say passing
>
> But I'm not comparing the result of get_ref_base_and_extent, but
> the argument as is.
Ah, ok. But then the size of the memset shouldn't be compared
against the get_ref_base_and_extend size from src2 but to the
size of the access of SRC/DEST (clearly looking at the "size" of
the ADDR_EXPR argument is somewhat bogus).
And as you compare src and dest
with operand_equal_p there is no need to reject ssize != max_size
either (you'd of course not see memset (&a[i].x, 0, 400) because
&a[i].x is not invariant, you'd need to lookup the SSA def for a pointer
here).
You can get at the size of an actual access simpler than by
the full-blown get_ref_base_and_extent (just outline a
get_ref_size () from the head part of it.
I still think that using get_addr_base_and_unit_offset on the
address is better and passing decomposed (base, offset, size)
triplets to optimize_memcpy would also save you the
MEM[(char * {ref-all})&b] = MEM[(char * {ref-all})&a]; special-case.
> Perhaps where it does above the src2 = NULL_TREE;
> I could save the size into one var, off into another one and set
> src2 to the result of get_ref_base_and_extent in that case, then
> if those vars are set require the second stmt to be a memset and do the same
> stuff there?
>
> > in base, offset and size for src and dest into this function will
> > simplify things and should allow to handle
> >
> > memset (p+10, 0, 24);
> > memcpy (q, p+10, 24);
> >
> > if you compare bases with operand_equal_p.
> >
> > > + if (refs_may_alias_p (dest, src))
> > > + return;
> >
> > Why's that?
>
> I admit I'm not sure if GIMPLE_ASSIGN may be between overlapping objects or
> not, memset can't.
No, a memory-memory GIMPLE_ASSIGN has to be validly translatable to
memcpy.
Richard.
