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

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

Right. I tried to reduce that as much as possible here, but it's still pretty large. :/

> In particular, the absence of hasValue checks in various updated logic worries me.

Yeah, this is a casualty of the reducing mentioned above. `x.hasValue()` and `x != MemoryLocation::Unknown` are identical at the moment. While there wasn't anything too crazy WRT finding these checks (e.g. I didn't find any "well, the callers of X all verify this, ...", they can be pretty nonlocal even in a function.

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

Works for me! Part one is https://reviews.llvm.org/D45581. Once we're happy with that, I'll rebase this patch on top of it. :)



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


================
Comment at: include/llvm/Analysis/MemoryLocation.h:108
+    if (!hasValue() || !Other.hasValue())
+      return unknown();
+
----------------
reames wrote:
> If the two are precise and equal, this downgrades to imprecise.
Shouldn't the `Other == *this` check handle that case?


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


================
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;
----------------
reames wrote:
> Don't you need to check that we have a value here?
There's a check near the top of the function (line 1002) for `V1Size == MemoryLocation::UnknownSize || V2Size == MemoryLocation::UnknownSize`. It's annoying that it's so far away, though. :/

Happy to unwrap `V1Size` and `V2Size` into `unsigned`s directly after the check if you think that'd be clearer.


================
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()))
----------------
reames wrote:
> Same here.
(Same function, so the above reply applies here)


https://reviews.llvm.org/D44748