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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I am concerned that you doing too many things in one patch and that the mix of refactoring and functional changes may contain hard to spot bugs.  In particular, the absence of hasValue checks in various updated logic worries me.

I strongly request that you separate a patch which simply factors out a LocationSize type with the exact same semantics as the existing code.  Once landed, you can introduce the new precise concept on top of that.



================
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.
+//
----------------
I think you mean: must be exactly N bytes.  If not, I'm confused.


================
Comment at: include/llvm/Analysis/MemoryLocation.h:108
+    if (!hasValue() || !Other.hasValue())
+      return unknown();
+
----------------
If the two are precise and equal, this downgrades to imprecise.


================
Comment at: lib/Analysis/AliasSetTracker.cpp:631
       I.getPointer()->printAsOperand(OS << "(");
-      OS << ", " << I.getSize() << ")";
+      OS << ", " << I.getSize().toRaw() << ")";
     }
----------------
looks like we probably need a print(OS) method on LocationSize.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1054
         DL.getTypeStoreSize(cast<SequentialType>(Ty)->getElementType());
-    if (V1Size != ElementSize || V2Size != ElementSize)
+    if (V1Size.getValue() != ElementSize || V2Size.getValue() != ElementSize)
       return MayAlias;
----------------
Don't you need to check that we have a value here?


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1122
 
-  if (EltsDontOverlap(V1Off, V1Size, V2Off, V2Size) ||
-      EltsDontOverlap(V2Off, V2Size, V1Off, V1Size))
+  if (EltsDontOverlap(V1Off, V1Size.getValue(), V2Off, V2Size.getValue()) ||
+      EltsDontOverlap(V2Off, V2Size.getValue(), V1Off, V1Size.getValue()))
----------------
Same here.


https://reviews.llvm.org/D44748