Re: [PATCH] Optimiza aggregate a = b = c = {} (PR c/78408)

[Richard Biener <rguenther at suse dot de>]

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.