[PATCH] D44748: Track whether the size of a MemoryLocation is precise

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.

Thanks for the comments!



================
Comment at: include/llvm/Analysis/AliasSetTracker.h:74
       bool SizeChanged = false;
-      if (NewSize > Size) {
-        Size = NewSize;
-        SizeChanged = true;
+      if (NewSize != Size && Size.hasValue()) {
+        LocationSize OldSize = Size;
----------------
nlopes wrote:
> This is changing the semantics slightly. Before it would update Size if Newsize=-1. Is this intended?
I don't see how it's a functionality change. If `!Size.hasValue()`, that's the equivalent to `Size == UINT64_MAX` in the old code, which would result in updates never happening. If `NewSize != Size`, we'll conditionally update to the greater size (with the upper-bound bit definitely set). Am I missing something?

In any case, I removed the `hasValue()` check here, since we should handle `!hasValue()` correctly in `Size.combineWith(NewSize)`.


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:78
+        // we're getting. Don't treat it as an upper-bound.
+        Size = Size.isZero() ? NewSize : Size.combineWith(NewSize);
+        SizeChanged = OldSize != Size;
----------------
nlopes wrote:
> Access size can be zero. Is it safe to do the above?  You could use the map tombstone instead perhaps?
Good catch. I used the empty key since that's what we appear to use for `AAInfo`


https://reviews.llvm.org/D44748