Re: Add a mem_alias_size helper class

[Jeff Law <law@redhat.com>]

On 11/29/2016 03:51 PM, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
>> 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 ;-)
>
> Ah, yeah, hadn't thought about that, but I agree it's confusing
> in hindsight.  Your name doesn't seem too long if we drop the
> "size_was", e.g.
>
> - adjusted_for_alignment_p
> - padded_for_alignment_p
> - padded_for_mask_p
> - padded_for_and_p
padded_for_alignment_p seems good to me.

jeff