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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Mostly minor comments.  I considered giving a conditional LGTM, but decided it warranted one last round.  I think this is very close to landing.



================
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.
+//
----------------
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. 


================
Comment at: include/llvm/Analysis/MemoryLocation.h:112
+  // `Other`.
+  LocationSize combineWith(LocationSize Other) const {
+    if (Other == *this)
----------------
Can we pick a different name?  Maybe: union?  

This is returning a result which is conservatively correct for uses of either.  combineWith doesn't make that obvious.


================
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)) ||
----------------
This is definitely a warranted functional change, but please add a test case which reflects the change.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1779
   if (O1 == O2)
-    if (V1Size != MemoryLocation::UnknownSize &&
-        V2Size != MemoryLocation::UnknownSize &&
+    if (V1Size.isPrecise() && V2Size.isPrecise() &&
         (isObjectSize(O1, V1Size.getValue(), DL, TLI, NullIsValidLocation) ||
----------------
Same testing comment here.


================
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.
----------------
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.  


https://reviews.llvm.org/D44748