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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: include/llvm/Analysis/MemoryLocation.h:35
+// it contains, N, is 'precise'. Precise, in this context, means that we know
+// that the MemoryLocation we're referencing has to be at least N bytes large.
+//
----------------
reames wrote:
> george.burgess.iv wrote:
> > reames wrote:
> > > I think you mean: must be exactly N bytes.  If not, I'm confused.
> > Yeah, I could've worded this better. I was trying to say "block of bytes that the `MemoryLocation` references" (e.g. a `store i32 1, i32* %a` implies that `%a` must be *at least* 4 bytes large). Will update with the rebase.
> Looks like this update got lost?  
> 
> JFYI, I consider clarity on this point essential. 
Good catch; sorry for dropping this. Please let me know if you'd like me to expand further.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1718
   bool NullIsValidLocation = NullPointerIsDefined(&F);
-  if ((V1Size != MemoryLocation::UnknownSize &&
-       isObjectSmallerThan(O2, V1Size.getValue(), DL, TLI,
-                           NullIsValidLocation)) ||
-      (V2Size != MemoryLocation::UnknownSize &&
-       isObjectSmallerThan(O1, V2Size.getValue(), DL, TLI,
-                           NullIsValidLocation)))
+  if ((V1Size.isPrecise() && isObjectSmallerThan(O2, V1Size.getValue(), DL, TLI,
+                                                 NullIsValidLocation)) ||
----------------
reames wrote:
> This is definitely a warranted functional change, but please add a test case which reflects the change.
Added tests for both of these. It's not obvious to me how to get an imprecise size when directly testing BasicAA, so I opted for unittests. I'll do the newly-added `// FIXME: This is duplicated between this file and MemorySSATest. Refactor.` in a follow-up commit, in the interest of keeping this diff small.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:1140
+      } else {
+        // This query's Size is less than the cached one. Conservatively restart
+        // the query using the greater size.
----------------
reames wrote:
> This looks to have very different behaviour than previously.  Unless I'm misreading nesting, you’re recomputing any time the sizes are equal which is odd.  
Yeah, this is one level of nesting deeper than it was previously (it's odd to me that phab doesn't point this out). In particular, the `CacheInfo->Size != Loc.Size` check guards this


https://reviews.llvm.org/D44748