Re: Add a mem_alias_size helper class

[Jeff Law <law at redhat dot com>]

On 11/15/2016 09:04 AM, Richard Sandiford wrote:
> alias.c encodes memory sizes as follows:
>
> size > 0: the exact size is known
> size == 0: the size isn't known
> size < 0: the exact size of the reference itself is known,
>   but the address has been aligned via AND.  In this case
>   "-size" includes the size of the reference and the worst-case
>   number of bytes traversed by the AND.
>
> This patch wraps this up in a helper class and associated
> functions.  The new routines fix what seems to be a hole
> in the old logic: if the size of a reference A was unknown,
> offset_overlap_p would assume that it could conflict with any
> other reference B, even if we could prove that B comes before A.
>
> The fallback CONSTANT_P (x) && CONSTANT_P (y) case looked incorrect.
> Either "c" is trustworthy as a distance between the two constants,
> in which case the alignment handling should work as well there as
> elsewhere, or "c" isn't trustworthy, in which case offset_overlap_p
> is unsafe.  I think the latter's true; AFAICT we have no evidence
> that "c" really is the distance between the two references, so using
> it in the check doesn't make sense.
>
> At this point we've excluded cases for which:
>
> (a) the base addresses are the same
> (b) x and y are SYMBOL_REFs, or SYMBOL_REF-based constants
>     wrapped in a CONST
> (c) x and y are both constant integers
>
> No useful cases should be left.  As things stood, we would
> assume that:
>
>   (mem:SI (const_int X))
>
> could overlap:
>
>   (mem:SI (symbol_ref Y))
>
> but not:
>
>   (mem:SI (const (plus (symbol_ref Y) (const_int 4))))
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>
> Thanks,
> Richard
>
>
> [ This patch is part of the SVE series posted here:
>   https://gcc.gnu.org/ml/gcc/2016-11/msg00030.html ]
>
> gcc/
> 2016-11-15  Richard Sandiford  <richard.sandiford@arm.com>
> 	    Alan Hayward  <alan.hayward@arm.com>
> 	    David Sherwood  <david.sherwood@arm.com>
>
> 	* alias.c (mem_alias_size): New class.
> 	(mem_alias_size::mode): New function.
> 	(mem_alias_size::exact_p): Likewise.
> 	(mem_alias_size::max_size_known_p): Likewise.
> 	(align_to): Likewise.
> 	(alias_may_gt): Likewise.
> 	(addr_side_effect_eval): Change type of size argument to
> 	mem_alias_size.  Use plus_constant.
> 	(offset_overlap_p): Change type of xsize and ysize to
> 	mem_alias_size.  Use alias_may_gt.  Don't assume an overlap
> 	between an access of unknown size and an access that's known
> 	to be earlier than it.
> 	(memrefs_conflict_p): Change type of xsize and ysize to
> 	mem_alias_size.  Remove fallback CONSTANT_P (x) && CONSTANT_P (y)
> 	handling.
OK.  One possible nit below you might want to consider changing.

> +/* Represents the size of a memory reference during alias analysis.
> +   There are three possibilities:
>
> -/* Set up all info needed to perform alias analysis on memory references.  */
> +   (1) the size needs to be treated as completely unknown
> +   (2) the size is known exactly and no alignment is applied to the address
> +   (3) the size is known exactly but an alignment is applied to the address
> +
> +   (3) is used for aligned addresses of the form (and X (const_int -N)),
> +   which can subtract something in the range [0, N) from the original
> +   address X.  We handle this by subtracting N - 1 from X and adding N - 1
> +   to the size, so that the range spans all possible bytes.  */
> +class mem_alias_size {
> +public:
> +  /* Return an unknown size (case (1) above).  */
> +  static mem_alias_size unknown () { return (HOST_WIDE_INT) 0; }
> +
> +  /* Return an exact size (case (2) above).  */
> +  static mem_alias_size exact (HOST_WIDE_INT size) { return size; }
> +
> +  /* Return a worst-case size after alignment (case (3) above).
> +     SIZE includes the maximum adjustment applied by the alignment.  */
> +  static mem_alias_size aligned (HOST_WIDE_INT size) { return -size; }
> +
> +  /* Return the size of memory reference X.  */
> +  static mem_alias_size mem (const_rtx x) { return MEM_SIZE (x); }
> +
> +  static mem_alias_size mode (machine_mode m);
> +
> +  /* Return true if the exact size of the memory is known.  */
> +  bool exact_p () const { return m_value > 0; }
> +  bool exact_p (HOST_WIDE_INT *) const;
> +
> +  /* Return true if an upper bound on the memory size is known;
> +     i.e. not case (1) above.  */
> +  bool max_size_known_p () const { return m_value != 0; }
> +  bool max_size_known_p (HOST_WIDE_INT *) const;
> +
> +  /* Return true if the size is subject to alignment.  */
> +  bool aligned_p () const { return m_value < 0; }
> +
> +private:
> +  mem_alias_size (HOST_WIDE_INT value) : m_value (value) {}
> +
> +  HOST_WIDE_INT m_value;
> +};
If I were to see a call to the aligned_p method, my first thought is 
testing if an object is properly aligned.  This method actually tells us 
something different -- was the size adjusted to account for alignment 
issues.

In fact, when I was reading the memrefs_conflict_p changes that's the 
mistake I nearly called the code out as wrong.  Then I went back to the 
class definition and realized my mistake.

ISTM that a better name would avoid someone else making a similar 
mistake.  So consider changing aligned_p to something else 
(size_was_adjusted_for_alignment seems overly verbose ;-)


Jeff