Jeff Law <l...@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.sandif...@arm.com> >> Alan Hayward <alan.hayw...@arm.com> >> David Sherwood <david.sherw...@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 Thanks, Richard
