[PATCH] D45581: Add a LocationSize type to represent the size of MemoryLocations

[Philip Reames via Phabricator via llvm-commits]

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

Ok, I think you missed my point in the other review.  The entire idea was to have something which was so obviously correct it didn't need further review.  Specifically, to avoid waiting for me or anyone else to review it.  Unfortunately, we're now in the worst of both worlds: this change does not appear to be fully NFC and we had a long review delay.

Let me now try to be a bit more specific.

Please declare a *typedef* of uint64_t (not a class) called LocationSize in the MemoryLocation.h header within the llvm namespace.  Update all the parameters and fields which are currently uint64_t, but will eventually be a LocationSize.  Do not change the meaning of the bit-patterns in any way.  Make sure it builds and passes make check, then submit without further review.

Once done, discard this change and rebase your original change over the typedef.  This should greatly reduce the LOC changed.



================
Comment at: include/llvm/Analysis/AliasSetTracker.h:55
     AliasSet *AS = nullptr;
-    uint64_t Size = 0;
+    LocationSize Size = LocationSize::mapEmpty();
     AAMDNodes AAInfo;
----------------
To preserve the old behavior, doesn't this need to be 0?


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:59
+    // Whether the size for this record has been set at all. This makes no
+    // guarantees about the size being known.
+    bool isSizeSet() const { return Size != LocationSize::mapEmpty(); }
----------------
I recommend leaving this out in the first patch.  This part starts changing semantics.  Please do the large change *without* changing semantics, then a small patch with the semantic change.


https://reviews.llvm.org/D45581