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

George Burgess IV via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 30 00:02:21 PDT 2018


george.burgess.iv added inline comments.


================
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;
----------------
george.burgess.iv wrote:
> 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.
> Happy to unwrap V1Size and V2Size into unsigneds directly after the check if you think that'd be clearer.

(I've done this across BasicAA in hopes of making things a bit clearer. Please let me know if you disagree)


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1188
+  return GEPBaseOffset >=
+         ObjectBaseOffset + (int64_t)ObjectAccessSize.getValue();
 }
----------------
Not a BasicAA expert, but since I see "negative offset" and an int64_t cast here, I wanted to highlight that this LocationSize change eats the sign bit of `ObjectAccessSize`. I assume that `ObjectAccessSize` should always be positive and the `int64_t` cast exists just to keep the >= comparison signed, but...


https://reviews.llvm.org/D44748





More information about the llvm-commits mailing list