[NFC] [AAPointerInfo] OffsetAndSize is no longer an std::pair

[sameerds]

Summary

The struct OffsetAndSize is a simple tuple of two int64_t. Treating it as a
derived class of std::pair has no special benefit, but it makes the code
verbose since we need get/set functions that avoid using "first" and "second" in
client code. Eliminating the std::pair makes this more readable.

[jdoerfert]

It's a nice cleanup. I have one concern below.

llvm/include/llvm/Transforms/IPO/Attributor.h

266

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

732

Do we need to ensure empty and tombstone key do not clash with unknown/unassigned?

[sameerds]

Addressed comments about special constants and operator!=

[sameerds]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

732

That kinda goes without saying whenever a DenseMap is used. But I've now added comments to the place where Unknown and Unsigned are declared.

[sameerds]

llvm/include/llvm/Transforms/IPO/Attributor.h

256

Note: I mixed up MAX and MIN here, but I've fixed it in my local change that is ready to commit.

[jdoerfert]

LG

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

732

In case these are all compile time constants we could even use a static_assert.

llvm/include/llvm/Transforms/IPO/Attributor.h

 /// not_none + undef => not_none
 ///          V1 + V2 => nullptr
 Optional<Value *>
 combineOptionalValuesInAAValueLatice(const Optional<Value *> &A,
                                      const Optional<Value *> &B, Type *Ty);
 
 /// Helper to represent an access offset and size, with logic to deal with
 /// uncertainty and check for overlapping accesses.
-struct OffsetAndSize : public std::pair<int64_t, int64_t> {
-  using BaseTy = std::pair<int64_t, int64_t>;
-  OffsetAndSize(int64_t Offset, int64_t Size) : BaseTy(Offset, Size) {}
-  OffsetAndSize(const BaseTy &P) : BaseTy(P) {}
-  int64_t getOffset() const { return first; }
-  int64_t getSize() const { return second; }
-  static OffsetAndSize getUnknown() { return OffsetAndSize(Unknown, Unknown); }
-  static OffsetAndSize getUnassigned() {
-    return OffsetAndSize(Unassigned, Unassigned);
-  }
+struct OffsetAndSize {
+  int64_t Offset = Unassigned;
+  int64_t Size = Unassigned;
+
+  OffsetAndSize(int64_t Offset, int64_t Size) : Offset(Offset), Size(Size) {}
+  OffsetAndSize() = default;
+  static OffsetAndSize getUnknown() { return OffsetAndSize{Unknown, Unknown}; }
 
   /// Return true if offset or size are unknown.
   bool offsetOrSizeAreUnknown() const {
-    return getOffset() == OffsetAndSize::Unknown ||
-           getSize() == OffsetAndSize::Unknown;
+    return Offset == OffsetAndSize::Unknown || Size == OffsetAndSize::Unknown;
   }
 
   /// Return true if offset and size are unknown, thus this is the default
   /// unknown object.
   bool offsetAndSizeAreUnknown() const {
-    return getOffset() == OffsetAndSize::Unknown &&
-           getSize() == OffsetAndSize::Unknown;
+    return Offset == OffsetAndSize::Unknown && Size == OffsetAndSize::Unknown;
   }
 
   /// Return true if the offset and size are unassigned.
   bool isUnassigned() const {
-    assert((getOffset() == OffsetAndSize::Unassigned) ==
-               (getSize() == OffsetAndSize::Unassigned) &&
+    assert((Offset == OffsetAndSize::Unassigned) ==
+               (Size == OffsetAndSize::Unassigned) &&
            "Inconsistent state!");
-    return getOffset() == OffsetAndSize::Unassigned;
+    return Offset == OffsetAndSize::Unassigned;
   }
 
   /// Return true if this offset and size pair might describe an address that
   /// overlaps with \p OAS.
   bool mayOverlap(const OffsetAndSize &OAS) const {
     // Any unknown value and we are giving up -> overlap.
     if (offsetOrSizeAreUnknown() || OAS.offsetOrSizeAreUnknown())
       return true;
 
     // Check if one offset point is in the other interval [offset,
     // offset+size].
-    return OAS.getOffset() + OAS.getSize() > getOffset() &&
-           OAS.getOffset() < getOffset() + getSize();
+    return OAS.Offset + OAS.Size > Offset && OAS.Offset < Offset + Size;
   }
 
   /// Constants used to represent special offsets or sizes.
+  /// - This assumes that Offset and Size are non-negative.
+  /// - The constants should not clash with DenseMapInfo, such as EmptyKey
+  ///   (INT64_MIN) and TombstoneKey (INT64_MAX).
   static constexpr int64_t Unassigned = -1;
   static constexpr int64_t Unknown = -2;
 };
 
+inline bool operator==(const OffsetAndSize &A, const OffsetAndSize &B) {
+  return A.Offset == B.Offset && A.Size == B.Size;
+}
+
+inline bool operator!=(const OffsetAndSize &A, const OffsetAndSize &B) {
+  return !(A == B);
+}
+
 /// Return the initial value of \p Obj with type \p Ty if that is a constant.
 Constant *getInitialValueForObj(Value &Obj, Type &Ty,
                                 const TargetLibraryInfo *TLI,
                                 const DataLayout &DL,
                                 OffsetAndSize *OASPtr = nullptr);
 
 /// Collect all potential underlying objects of \p Ptr at position \p CtxI in
 /// \p Objects. Assumed information is used and dependences onto \p QueryingAA

llvm/lib/Transforms/IPO/Attributor.cpp

Constant *AA::getInitialValueForObj(Value &Obj, Type &Ty,
   if (!GV)
     return nullptr;
   if (!GV->hasLocalLinkage() && !(GV->isConstant() && GV->hasInitializer()))
     return nullptr;
   if (!GV->hasInitializer())
     return UndefValue::get(&Ty);
 
   if (OASPtr && !OASPtr->offsetOrSizeAreUnknown()) {
-    APInt Offset = APInt(64, OASPtr->getOffset());
+    APInt Offset = APInt(64, OASPtr->Offset);
     return ConstantFoldLoadFromConst(GV->getInitializer(), &Ty, Offset, DL);
   }
 
   return ConstantFoldLoadFromUniformValue(GV->getInitializer(), &Ty);
 }
 
 bool AA::isValidInScope(const Value &V, const Function *Scope) {
   if (isa<Constant>(V))
auto CheckAccess = [&](const AAPointerInfo::Access &Acc, bool IsExact) {
       }
       return true;
     };
 
     // If the value has been written to we don't need the initial value of the
     // object.
     bool HasBeenWrittenTo = false;
 
-    AA::OffsetAndSize OAS = AA::OffsetAndSize::getUnassigned();
+    AA::OffsetAndSize OAS;
     auto &PI = A.getAAFor<AAPointerInfo>(QueryingAA, IRPosition::value(*Obj),
                                          DepClassTy::NONE);
     if (!PI.forallInterferingAccesses(A, QueryingAA, I, CheckAccess,
                                       HasBeenWrittenTo, &OAS)) {
       LLVM_DEBUG(
           dbgs()
           << "Failed to verify all interfering accesses for underlying object: "
           << *Obj << "\n");

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

struct DenseMapInfo<AAPointerInfo::Access> : DenseMapInfo<Instruction *> {
   using Access = AAPointerInfo::Access;
   static inline Access getEmptyKey();
   static inline Access getTombstoneKey();
   static unsigned getHashValue(const Access &A);
   static bool isEqual(const Access &LHS, const Access &RHS);
 };
 
 /// Helper that allows OffsetAndSize as a key in a DenseMap.
-template <>
-struct DenseMapInfo<AA::OffsetAndSize>
-    : DenseMapInfo<std::pair<int64_t, int64_t>> {};
+template <> struct DenseMapInfo<AA::OffsetAndSize> {
+  static inline AA::OffsetAndSize getEmptyKey() {
+    auto EmptyKey = DenseMapInfo<int64_t>::getEmptyKey();
+    return AA::OffsetAndSize{EmptyKey, EmptyKey};
+  }
+
+  static inline AA::OffsetAndSize getTombstoneKey() {
+    auto TombstoneKey = DenseMapInfo<int64_t>::getTombstoneKey();
+    return AA::OffsetAndSize{TombstoneKey, TombstoneKey};
+  }
+
+  static unsigned getHashValue(const AA::OffsetAndSize &OAS) {
+    return detail::combineHashValue(
+        DenseMapInfo<int64_t>::getHashValue(OAS.Offset),
+        DenseMapInfo<int64_t>::getHashValue(OAS.Size));
+  }
+
+  static bool isEqual(const AA::OffsetAndSize &A, const AA::OffsetAndSize B) {
+    return A == B;
+  }
+};
 
 /// Helper for AA::PointerInfo::Access DenseMap/Set usage ignoring everythign
 /// but the instruction
 struct AccessAsInstructionInfo : DenseMapInfo<Instruction *> {
   using Base = DenseMapInfo<Instruction *>;
   using Access = AAPointerInfo::Access;
   static inline Access getEmptyKey();
   static inline Access getTombstoneKey();
protected:
   bool forallInterferingAccesses(
       Instruction &I,
       function_ref<bool(const AAPointerInfo::Access &, bool)> CB,
       AA::OffsetAndSize *OASPtr) const {
     if (!isValidState())
       return false;
 
     // First find the offset and size of I.
-    AA::OffsetAndSize OAS = AA::OffsetAndSize::getUnassigned();
+    AA::OffsetAndSize OAS;
     for (const auto &It : AccessBins) {
       for (auto &Access : *It.getSecond()) {
         if (Access.getRemoteInst() == &I) {
           OAS = It.getFirst();
           break;
         }
       }
-      if (OAS.getSize() != AA::OffsetAndSize::Unassigned)
+      if (OAS.Size != AA::OffsetAndSize::Unassigned)
         break;
     }
 
     if (OASPtr)
       *OASPtr = OAS;
 
     // No access for I was found, we are done.
-    if (OAS.getSize() == AA::OffsetAndSize::Unassigned)
+    if (OAS.Size == AA::OffsetAndSize::Unassigned)
       return true;
 
     // Now that we have an offset and size, find all overlapping ones and use
     // the callback on the accesses.
     return forallInterferingAccesses(OAS, CB);
   }
 
 private:
ChangeStatus translateAndAddState(Attributor &A, const AAPointerInfo &OtherAA,
     bool IsByval =
         FromCallee && OtherAAImpl.getAssociatedArgument()->hasByValAttr();
 
     // Combine the accesses bin by bin.
     ChangeStatus Changed = ChangeStatus::UNCHANGED;
     for (const auto &It : OtherAAImpl.getState()) {
       AA::OffsetAndSize OAS = AA::OffsetAndSize::getUnknown();
       if (Offset != AA::OffsetAndSize::Unknown)
-        OAS = AA::OffsetAndSize(It.first.getOffset() + Offset,
-                                It.first.getSize());
+        OAS = AA::OffsetAndSize(It.first.Offset + Offset, It.first.Size);
       Accesses *Bin = AccessBins.lookup(OAS);
       for (const AAPointerInfo::Access &RAcc : *It.second) {
         if (IsByval && !RAcc.isRead())
           continue;
         bool UsedAssumedInformation = false;
         AccessKind AK = RAcc.getKind();
         Optional<Value *> Content = RAcc.getContent();
         if (FromCallee) {
           Content = A.translateArgumentToCallSiteContent(
               RAcc.getContent(), CB, *this, UsedAssumedInformation);
           AK =
               AccessKind(AK & (IsByval ? AccessKind::AK_R : AccessKind::AK_RW));
           AK = AccessKind(AK | (RAcc.isMayAccess() ? AK_MAY : AK_MUST));
         }
         Changed =
-            Changed | addAccess(A, OAS.getOffset(), OAS.getSize(), CB, Content,
-                                AK, RAcc.getType(), RAcc.getRemoteInst(), Bin);
+            Changed | addAccess(A, OAS.Offset, OAS.Size, CB, Content, AK,
+                                RAcc.getType(), RAcc.getRemoteInst(), Bin);
       }
     }
     return Changed;
   }
 
   /// Statistic tracking for all AAPointerInfo implementations.
   /// See AbstractAttribute::trackStatistics().
   void trackPointerInfoStatistics(const IRPosition &IRP) const {}
 
   /// Dump the state into \p O.
   void dumpState(raw_ostream &O) {
     for (auto &It : AccessBins) {
-      O << "[" << It.first.getOffset() << "-"
-        << It.first.getOffset() + It.first.getSize()
+      O << "[" << It.first.Offset << "-" << It.first.Offset + It.first.Size
         << "] : " << It.getSecond()->size() << "\n";
       for (auto &Acc : *It.getSecond()) {
         O << "     - " << Acc.getKind() << " - " << *Acc.getLocalInst() << "\n";
         if (Acc.getLocalInst() != Acc.getRemoteInst())
           O << "     -->                         " << *Acc.getRemoteInst()
             << "\n";
         if (!Acc.isWrittenValueYetUndetermined()) {
           if (Acc.getWrittenValue())