Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][ObjectSizeOffset] Use classes instead of std::pair #76882

Merged
merged 10 commits into from
Jan 6, 2024

Conversation

bwendling
Copy link
Collaborator

The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)

The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Bill Wendling (bwendling)

Changes

The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)


Patch is 40.34 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76882.diff

5 Files Affected:

  • (modified) llvm/include/llvm/Analysis/MemoryBuiltins.h (+120-72)
  • (modified) llvm/lib/Analysis/MemoryBuiltins.cpp (+163-151)
  • (modified) llvm/lib/Transforms/IPO/AttributorAttributes.cpp (+4-4)
  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+8-4)
  • (modified) llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/MemoryBuiltins.h b/llvm/include/llvm/Analysis/MemoryBuiltins.h
index 827b5081b2ce75..56faa32fb0b226 100644
--- a/llvm/include/llvm/Analysis/MemoryBuiltins.h
+++ b/llvm/include/llvm/Analysis/MemoryBuiltins.h
@@ -187,80 +187,146 @@ Value *lowerObjectSizeCall(
     const TargetLibraryInfo *TLI, AAResults *AA, bool MustSucceed,
     SmallVectorImpl<Instruction *> *InsertedInstructions = nullptr);
 
-using SizeOffsetType = std::pair<APInt, APInt>;
+/// SizeOffsetType - A base template class for the object size visitors. Used
+/// here as a self-documenting way to handle the values rather than using a
+/// \p std::pair.
+template <typename T> struct SizeOffsetType {
+  T Size;
+  T Offset;
+
+  bool knownSize() const;
+  bool knownOffset() const;
+  bool anyKnown() const;
+  bool bothKnown() const;
+};
+
+/// SizeOffsetType<APInt> - Used by \p ObjectSizeOffsetVisitor, which works
+/// with \p APInts.
+template <> struct SizeOffsetType<APInt> {
+  APInt Size;
+  APInt Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(APInt Size, APInt Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.getBitWidth() > 1; }
+  bool knownOffset() const { return Offset.getBitWidth() > 1; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<APInt> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<APInt> &RHS) { return !(*this == RHS); }
+};
+using SizeOffsetAPInt = SizeOffsetType<APInt>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*
 /// statically. Fails if size or offset are not known at compile time.
 class ObjectSizeOffsetVisitor
-  : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {
+    : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetAPInt> {
   const DataLayout &DL;
   const TargetLibraryInfo *TLI;
   ObjectSizeOpts Options;
   unsigned IntTyBits;
   APInt Zero;
-  SmallDenseMap<Instruction *, SizeOffsetType, 8> SeenInsts;
+  SmallDenseMap<Instruction *, SizeOffsetAPInt, 8> SeenInsts;
   unsigned InstructionsVisited;
 
   APInt align(APInt Size, MaybeAlign Align);
 
-  SizeOffsetType unknown() {
-    return std::make_pair(APInt(), APInt());
-  }
+  static SizeOffsetAPInt unknown;
 
 public:
   ObjectSizeOffsetVisitor(const DataLayout &DL, const TargetLibraryInfo *TLI,
                           LLVMContext &Context, ObjectSizeOpts Options = {});
 
-  SizeOffsetType compute(Value *V);
-
-  static bool knownSize(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.first.getBitWidth() > 1;
-  }
-
-  static bool knownOffset(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.second.getBitWidth() > 1;
-  }
-
-  static bool bothKnown(const SizeOffsetType &SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetAPInt compute(Value *V);
 
   // These are "private", except they can't actually be made private. Only
   // compute() should be used by external users.
-  SizeOffsetType visitAllocaInst(AllocaInst &I);
-  SizeOffsetType visitArgument(Argument &A);
-  SizeOffsetType visitCallBase(CallBase &CB);
-  SizeOffsetType visitConstantPointerNull(ConstantPointerNull&);
-  SizeOffsetType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetType visitGlobalAlias(GlobalAlias &GA);
-  SizeOffsetType visitGlobalVariable(GlobalVariable &GV);
-  SizeOffsetType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetType visitLoadInst(LoadInst &I);
-  SizeOffsetType visitPHINode(PHINode&);
-  SizeOffsetType visitSelectInst(SelectInst &I);
-  SizeOffsetType visitUndefValue(UndefValue&);
-  SizeOffsetType visitInstruction(Instruction &I);
+  SizeOffsetAPInt visitAllocaInst(AllocaInst &I);
+  SizeOffsetAPInt visitArgument(Argument &A);
+  SizeOffsetAPInt visitCallBase(CallBase &CB);
+  SizeOffsetAPInt visitConstantPointerNull(ConstantPointerNull &);
+  SizeOffsetAPInt visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetAPInt visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetAPInt visitGlobalAlias(GlobalAlias &GA);
+  SizeOffsetAPInt visitGlobalVariable(GlobalVariable &GV);
+  SizeOffsetAPInt visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetAPInt visitLoadInst(LoadInst &I);
+  SizeOffsetAPInt visitPHINode(PHINode &);
+  SizeOffsetAPInt visitSelectInst(SelectInst &I);
+  SizeOffsetAPInt visitUndefValue(UndefValue &);
+  SizeOffsetAPInt visitInstruction(Instruction &I);
 
 private:
-  SizeOffsetType findLoadSizeOffset(
+  SizeOffsetAPInt findLoadSizeOffset(
       LoadInst &LoadFrom, BasicBlock &BB, BasicBlock::iterator From,
-      SmallDenseMap<BasicBlock *, SizeOffsetType, 8> &VisitedBlocks,
+      SmallDenseMap<BasicBlock *, SizeOffsetAPInt, 8> &VisitedBlocks,
       unsigned &ScannedInstCount);
-  SizeOffsetType combineSizeOffset(SizeOffsetType LHS, SizeOffsetType RHS);
-  SizeOffsetType computeImpl(Value *V);
-  SizeOffsetType computeValue(Value *V);
+  SizeOffsetAPInt combineSizeOffset(SizeOffsetAPInt LHS, SizeOffsetAPInt RHS);
+  SizeOffsetAPInt computeImpl(Value *V);
+  SizeOffsetAPInt computeValue(Value *V);
   bool CheckedZextOrTrunc(APInt &I);
 };
 
-using SizeOffsetEvalType = std::pair<Value *, Value *>;
+template <> struct SizeOffsetType<WeakTrackingVH>;
+
+/// SizeOffsetType<Value *> - Used by \p ObjectSizeOffsetEvaluator, which works
+/// with \p Values.
+template <> struct SizeOffsetType<Value *> {
+  Value *Size;
+  Value *Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+  SizeOffsetType(SizeOffsetType<WeakTrackingVH> &SOT);
+
+  bool knownSize() const { return Size != nullptr; }
+  bool knownOffset() const { return Offset != nullptr; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetValue = SizeOffsetType<Value *>;
+
+/// SizeOffsetType<WeakTrackingVH> - Used by \p ObjectSizeOffsetEvaluator in a
+/// \p DenseMap.
+template <> struct SizeOffsetType<WeakTrackingVH> {
+  WeakTrackingVH Size;
+  WeakTrackingVH Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.pointsToAliveValue(); }
+  bool knownOffset() const { return Offset.pointsToAliveValue(); }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return (Value *)Size == (Value *)RHS.Size &&
+           (Value *)Offset == (Value *)RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetWeakTrackingVH = SizeOffsetType<WeakTrackingVH>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*.
 /// May create code to compute the result at run-time.
 class ObjectSizeOffsetEvaluator
-  : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetEvalType> {
+    : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetValue> {
   using BuilderTy = IRBuilder<TargetFolder, IRBuilderCallbackInserter>;
-  using WeakEvalType = std::pair<WeakTrackingVH, WeakTrackingVH>;
+  using WeakEvalType = SizeOffsetType<WeakTrackingVH>;
   using CacheMapTy = DenseMap<const Value *, WeakEvalType>;
   using PtrSetTy = SmallPtrSet<const Value *, 8>;
 
@@ -275,45 +341,27 @@ class ObjectSizeOffsetEvaluator
   ObjectSizeOpts EvalOpts;
   SmallPtrSet<Instruction *, 8> InsertedInstructions;
 
-  SizeOffsetEvalType compute_(Value *V);
+  SizeOffsetValue compute_(Value *V);
 
 public:
-  static SizeOffsetEvalType unknown() {
-    return std::make_pair(nullptr, nullptr);
-  }
-
   ObjectSizeOffsetEvaluator(const DataLayout &DL, const TargetLibraryInfo *TLI,
                             LLVMContext &Context, ObjectSizeOpts EvalOpts = {});
 
-  SizeOffsetEvalType compute(Value *V);
+  static SizeOffsetValue unknown;
 
-  bool knownSize(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.first;
-  }
-
-  bool knownOffset(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.second;
-  }
-
-  bool anyKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) || knownOffset(SizeOffset);
-  }
-
-  bool bothKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetValue compute(Value *V);
 
   // The individual instruction visitors should be treated as private.
-  SizeOffsetEvalType visitAllocaInst(AllocaInst &I);
-  SizeOffsetEvalType visitCallBase(CallBase &CB);
-  SizeOffsetEvalType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetEvalType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetEvalType visitGEPOperator(GEPOperator &GEP);
-  SizeOffsetEvalType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetEvalType visitLoadInst(LoadInst &I);
-  SizeOffsetEvalType visitPHINode(PHINode &PHI);
-  SizeOffsetEvalType visitSelectInst(SelectInst &I);
-  SizeOffsetEvalType visitInstruction(Instruction &I);
+  SizeOffsetValue visitAllocaInst(AllocaInst &I);
+  SizeOffsetValue visitCallBase(CallBase &CB);
+  SizeOffsetValue visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetValue visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetValue visitGEPOperator(GEPOperator &GEP);
+  SizeOffsetValue visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetValue visitLoadInst(LoadInst &I);
+  SizeOffsetValue visitPHINode(PHINode &PHI);
+  SizeOffsetValue visitSelectInst(SelectInst &I);
+  SizeOffsetValue visitInstruction(Instruction &I);
 };
 
 } // end namespace llvm
diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 9e6811f3bf8815..8cc2d070d1d8e7 100644
--- a/llvm/lib/Analysis/MemoryBuiltins.cpp
+++ b/llvm/lib/Analysis/MemoryBuiltins.cpp
@@ -577,10 +577,12 @@ Value *llvm::getFreedOperand(const CallBase *CB, const TargetLibraryInfo *TLI) {
 //===----------------------------------------------------------------------===//
 //  Utility functions to compute size of objects.
 //
-static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
-  if (Data.second.isNegative() || Data.first.ult(Data.second))
-    return APInt(Data.first.getBitWidth(), 0);
-  return Data.first - Data.second;
+static APInt getSizeWithOverflow(const SizeOffsetAPInt &Data) {
+  APInt Size = Data.Size;
+  APInt Offset = Data.Offset;
+  if (Offset.isNegative() || Size.ult(Size))
+    return APInt(Size.getBitWidth(), 0);
+  return Size - Offset;
 }
 
 /// Compute the size of the object pointed by Ptr. Returns true and the
@@ -590,8 +592,8 @@ static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
 bool llvm::getObjectSize(const Value *Ptr, uint64_t &Size, const DataLayout &DL,
                          const TargetLibraryInfo *TLI, ObjectSizeOpts Opts) {
   ObjectSizeOffsetVisitor Visitor(DL, TLI, Ptr->getContext(), Opts);
-  SizeOffsetType Data = Visitor.compute(const_cast<Value*>(Ptr));
-  if (!Visitor.bothKnown(Data))
+  SizeOffsetAPInt Data = Visitor.compute(const_cast<Value *>(Ptr));
+  if (!Data.bothKnown())
     return false;
 
   Size = getSizeWithOverflow(Data).getZExtValue();
@@ -640,10 +642,9 @@ Value *llvm::lowerObjectSizeCall(
   } else {
     LLVMContext &Ctx = ObjectSize->getFunction()->getContext();
     ObjectSizeOffsetEvaluator Eval(DL, TLI, Ctx, EvalOptions);
-    SizeOffsetEvalType SizeOffsetPair =
-        Eval.compute(ObjectSize->getArgOperand(0));
+    SizeOffsetValue SizeOffsetPair = Eval.compute(ObjectSize->getArgOperand(0));
 
-    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown()) {
+    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown) {
       IRBuilder<TargetFolder, IRBuilderCallbackInserter> Builder(
           Ctx, TargetFolder(DL), IRBuilderCallbackInserter([&](Instruction *I) {
             if (InsertedInstructions)
@@ -651,19 +652,19 @@ Value *llvm::lowerObjectSizeCall(
           }));
       Builder.SetInsertPoint(ObjectSize);
 
+      Value *Size = SizeOffsetPair.Size;
+      Value *Offset = SizeOffsetPair.Offset;
+
       // If we've outside the end of the object, then we can always access
       // exactly 0 bytes.
-      Value *ResultSize =
-          Builder.CreateSub(SizeOffsetPair.first, SizeOffsetPair.second);
-      Value *UseZero =
-          Builder.CreateICmpULT(SizeOffsetPair.first, SizeOffsetPair.second);
+      Value *ResultSize = Builder.CreateSub(Size, Offset);
+      Value *UseZero = Builder.CreateICmpULT(Size, Offset);
       ResultSize = Builder.CreateZExtOrTrunc(ResultSize, ResultType);
       Value *Ret = Builder.CreateSelect(
           UseZero, ConstantInt::get(ResultType, 0), ResultSize);
 
       // The non-constant size expression cannot evaluate to -1.
-      if (!isa<Constant>(SizeOffsetPair.first) ||
-          !isa<Constant>(SizeOffsetPair.second))
+      if (!isa<Constant>(Size) || !isa<Constant>(Offset))
         Builder.CreateAssumption(
             Builder.CreateICmpNE(Ret, ConstantInt::get(ResultType, -1)));
 
@@ -682,6 +683,8 @@ STATISTIC(ObjectVisitorArgument,
 STATISTIC(ObjectVisitorLoad,
           "Number of load instructions with unsolved size and offset");
 
+SizeOffsetAPInt ObjectSizeOffsetVisitor::unknown;
+
 APInt ObjectSizeOffsetVisitor::align(APInt Size, MaybeAlign Alignment) {
   if (Options.RoundToAlign && Alignment)
     return APInt(IntTyBits, alignTo(Size.getZExtValue(), *Alignment));
@@ -697,12 +700,12 @@ ObjectSizeOffsetVisitor::ObjectSizeOffsetVisitor(const DataLayout &DL,
   // a different address space.
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::compute(Value *V) {
   InstructionsVisited = 0;
   return computeImpl(V);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   unsigned InitialIntTyBits = DL.getIndexTypeSizeInBits(V->getType());
 
   // Stripping pointer casts can strip address space casts which can change the
@@ -719,7 +722,7 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   IntTyBits = DL.getIndexTypeSizeInBits(V->getType());
   Zero = APInt::getZero(IntTyBits);
 
-  SizeOffsetType SOT = computeValue(V);
+  SizeOffsetAPInt SOT = computeValue(V);
 
   bool IndexTypeSizeChanged = InitialIntTyBits != IntTyBits;
   if (!IndexTypeSizeChanged && Offset.isZero())
@@ -729,27 +732,28 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   // accumulated some constant offset (or both). Readjust the bit width to match
   // the argument index type size and apply the offset, as required.
   if (IndexTypeSizeChanged) {
-    if (knownSize(SOT) && !::CheckedZextOrTrunc(SOT.first, InitialIntTyBits))
-      SOT.first = APInt();
-    if (knownOffset(SOT) && !::CheckedZextOrTrunc(SOT.second, InitialIntTyBits))
-      SOT.second = APInt();
+    if (SOT.knownSize() && !::CheckedZextOrTrunc(SOT.Size, InitialIntTyBits))
+      SOT.Size = APInt();
+    if (SOT.knownOffset() &&
+        !::CheckedZextOrTrunc(SOT.Offset, InitialIntTyBits))
+      SOT.Offset = APInt();
   }
   // If the computed offset is "unknown" we cannot add the stripped offset.
-  return {SOT.first,
-          SOT.second.getBitWidth() > 1 ? SOT.second + Offset : SOT.second};
+  return {SOT.Size,
+          SOT.Offset.getBitWidth() > 1 ? SOT.Offset + Offset : SOT.Offset};
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeValue(Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V)) {
     // If we have already seen this instruction, bail out. Cycles can happen in
     // unreachable code after constant propagation.
-    auto P = SeenInsts.try_emplace(I, unknown());
+    auto P = SeenInsts.try_emplace(I, ObjectSizeOffsetVisitor::unknown);
     if (!P.second)
       return P.first->second;
     ++InstructionsVisited;
     if (InstructionsVisited > ObjectSizeOffsetVisitorMaxVisitInstructions)
-      return unknown();
-    SizeOffsetType Res = visit(*I);
+      return ObjectSizeOffsetVisitor::unknown;
+    SizeOffsetAPInt Res = visit(*I);
     // Cache the result for later visits. If we happened to visit this during
     // the above recursion, we would consider it unknown until now.
     SeenInsts[I] = Res;
@@ -768,55 +772,55 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
 
   LLVM_DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: "
                     << *V << '\n');
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
 bool ObjectSizeOffsetVisitor::CheckedZextOrTrunc(APInt &I) {
   return ::CheckedZextOrTrunc(I, IntTyBits);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
   TypeSize ElemSize = DL.getTypeAllocSize(I.getAllocatedType());
   if (ElemSize.isScalable() && Options.EvalMode != ObjectSizeOpts::Mode::Min)
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   APInt Size(IntTyBits, ElemSize.getKnownMinValue());
   if (!I.isArrayAllocation())
-    return std::make_pair(align(Size, I.getAlign()), Zero);
+    return SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
 
   Value *ArraySize = I.getArraySize();
   if (const ConstantInt *C = dyn_cast<ConstantInt>(ArraySize)) {
     APInt NumElems = C->getValue();
     if (!CheckedZextOrTrunc(NumElems))
-      return unknown();
+      return ObjectSizeOffsetVisitor::unknown;
 
     bool Overflow;
     Size = Size.umul_ov(NumElems, Overflow);
-    return Overflow ? unknown()
-                    : std::make_pair(align(Size, I.getAlign()), Zero);
+    return Overflow ? ObjectSizeOffsetVisitor::unknown
+                    : SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
   }
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
   Type *MemoryTy = A.getPointeeInMemoryValueType();
   // No interprocedural analysis is done at the moment.
   if (!MemoryTy|| !MemoryTy->isSized()) {
     ++ObjectVisitorArgument;
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   }
 
   APInt Size(IntTyBits, DL.getTypeAllocSize(MemoryTy));
-  return std::make_pair(align(Size, A.getParamAlign()), Zero);
+  return SizeOffsetAPInt(align(Size, A.getParamAlign()), Zero);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
   if (std::optional<APInt> Size = getAllocSize(&CB, TLI))
-    return std::make_pair(*Size, Zero);
-  return unknown();
+    return SizeOffsetAPInt(*Size, Zero);
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull &CPN) {
   // If null is unknown, there's nothing we can do. Additionally, non-zero
   // address spaces can make use of null, so we don't presume to know anything
   // about that.
@@ -825,45 +829,46 @@ ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
   // them on the floor, but it's unclear what we should do when a NULL from
   // addrspace(1) gets casted to addrspace(0) (or vice-versa).
   if (Options.NullIsUnknownSize || CPN.getType()->getAddressSpace())
-    return unknown();
-  return std::make_pair(Zero, Zero);
+    return ObjectSizeOffsetVisitor::unknown;
+  return SizeOffsetAPInt(Zero, Zero);
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst&) {
-  return unknown();
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst &) {
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractValueInst(ExtractValueInst&) {
+Size...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Bill Wendling (bwendling)

Changes

The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)


Patch is 40.34 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76882.diff

5 Files Affected:

  • (modified) llvm/include/llvm/Analysis/MemoryBuiltins.h (+120-72)
  • (modified) llvm/lib/Analysis/MemoryBuiltins.cpp (+163-151)
  • (modified) llvm/lib/Transforms/IPO/AttributorAttributes.cpp (+4-4)
  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+8-4)
  • (modified) llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/MemoryBuiltins.h b/llvm/include/llvm/Analysis/MemoryBuiltins.h
index 827b5081b2ce75..56faa32fb0b226 100644
--- a/llvm/include/llvm/Analysis/MemoryBuiltins.h
+++ b/llvm/include/llvm/Analysis/MemoryBuiltins.h
@@ -187,80 +187,146 @@ Value *lowerObjectSizeCall(
     const TargetLibraryInfo *TLI, AAResults *AA, bool MustSucceed,
     SmallVectorImpl<Instruction *> *InsertedInstructions = nullptr);
 
-using SizeOffsetType = std::pair<APInt, APInt>;
+/// SizeOffsetType - A base template class for the object size visitors. Used
+/// here as a self-documenting way to handle the values rather than using a
+/// \p std::pair.
+template <typename T> struct SizeOffsetType {
+  T Size;
+  T Offset;
+
+  bool knownSize() const;
+  bool knownOffset() const;
+  bool anyKnown() const;
+  bool bothKnown() const;
+};
+
+/// SizeOffsetType<APInt> - Used by \p ObjectSizeOffsetVisitor, which works
+/// with \p APInts.
+template <> struct SizeOffsetType<APInt> {
+  APInt Size;
+  APInt Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(APInt Size, APInt Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.getBitWidth() > 1; }
+  bool knownOffset() const { return Offset.getBitWidth() > 1; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<APInt> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<APInt> &RHS) { return !(*this == RHS); }
+};
+using SizeOffsetAPInt = SizeOffsetType<APInt>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*
 /// statically. Fails if size or offset are not known at compile time.
 class ObjectSizeOffsetVisitor
-  : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {
+    : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetAPInt> {
   const DataLayout &DL;
   const TargetLibraryInfo *TLI;
   ObjectSizeOpts Options;
   unsigned IntTyBits;
   APInt Zero;
-  SmallDenseMap<Instruction *, SizeOffsetType, 8> SeenInsts;
+  SmallDenseMap<Instruction *, SizeOffsetAPInt, 8> SeenInsts;
   unsigned InstructionsVisited;
 
   APInt align(APInt Size, MaybeAlign Align);
 
-  SizeOffsetType unknown() {
-    return std::make_pair(APInt(), APInt());
-  }
+  static SizeOffsetAPInt unknown;
 
 public:
   ObjectSizeOffsetVisitor(const DataLayout &DL, const TargetLibraryInfo *TLI,
                           LLVMContext &Context, ObjectSizeOpts Options = {});
 
-  SizeOffsetType compute(Value *V);
-
-  static bool knownSize(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.first.getBitWidth() > 1;
-  }
-
-  static bool knownOffset(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.second.getBitWidth() > 1;
-  }
-
-  static bool bothKnown(const SizeOffsetType &SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetAPInt compute(Value *V);
 
   // These are "private", except they can't actually be made private. Only
   // compute() should be used by external users.
-  SizeOffsetType visitAllocaInst(AllocaInst &I);
-  SizeOffsetType visitArgument(Argument &A);
-  SizeOffsetType visitCallBase(CallBase &CB);
-  SizeOffsetType visitConstantPointerNull(ConstantPointerNull&);
-  SizeOffsetType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetType visitGlobalAlias(GlobalAlias &GA);
-  SizeOffsetType visitGlobalVariable(GlobalVariable &GV);
-  SizeOffsetType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetType visitLoadInst(LoadInst &I);
-  SizeOffsetType visitPHINode(PHINode&);
-  SizeOffsetType visitSelectInst(SelectInst &I);
-  SizeOffsetType visitUndefValue(UndefValue&);
-  SizeOffsetType visitInstruction(Instruction &I);
+  SizeOffsetAPInt visitAllocaInst(AllocaInst &I);
+  SizeOffsetAPInt visitArgument(Argument &A);
+  SizeOffsetAPInt visitCallBase(CallBase &CB);
+  SizeOffsetAPInt visitConstantPointerNull(ConstantPointerNull &);
+  SizeOffsetAPInt visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetAPInt visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetAPInt visitGlobalAlias(GlobalAlias &GA);
+  SizeOffsetAPInt visitGlobalVariable(GlobalVariable &GV);
+  SizeOffsetAPInt visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetAPInt visitLoadInst(LoadInst &I);
+  SizeOffsetAPInt visitPHINode(PHINode &);
+  SizeOffsetAPInt visitSelectInst(SelectInst &I);
+  SizeOffsetAPInt visitUndefValue(UndefValue &);
+  SizeOffsetAPInt visitInstruction(Instruction &I);
 
 private:
-  SizeOffsetType findLoadSizeOffset(
+  SizeOffsetAPInt findLoadSizeOffset(
       LoadInst &LoadFrom, BasicBlock &BB, BasicBlock::iterator From,
-      SmallDenseMap<BasicBlock *, SizeOffsetType, 8> &VisitedBlocks,
+      SmallDenseMap<BasicBlock *, SizeOffsetAPInt, 8> &VisitedBlocks,
       unsigned &ScannedInstCount);
-  SizeOffsetType combineSizeOffset(SizeOffsetType LHS, SizeOffsetType RHS);
-  SizeOffsetType computeImpl(Value *V);
-  SizeOffsetType computeValue(Value *V);
+  SizeOffsetAPInt combineSizeOffset(SizeOffsetAPInt LHS, SizeOffsetAPInt RHS);
+  SizeOffsetAPInt computeImpl(Value *V);
+  SizeOffsetAPInt computeValue(Value *V);
   bool CheckedZextOrTrunc(APInt &I);
 };
 
-using SizeOffsetEvalType = std::pair<Value *, Value *>;
+template <> struct SizeOffsetType<WeakTrackingVH>;
+
+/// SizeOffsetType<Value *> - Used by \p ObjectSizeOffsetEvaluator, which works
+/// with \p Values.
+template <> struct SizeOffsetType<Value *> {
+  Value *Size;
+  Value *Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+  SizeOffsetType(SizeOffsetType<WeakTrackingVH> &SOT);
+
+  bool knownSize() const { return Size != nullptr; }
+  bool knownOffset() const { return Offset != nullptr; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetValue = SizeOffsetType<Value *>;
+
+/// SizeOffsetType<WeakTrackingVH> - Used by \p ObjectSizeOffsetEvaluator in a
+/// \p DenseMap.
+template <> struct SizeOffsetType<WeakTrackingVH> {
+  WeakTrackingVH Size;
+  WeakTrackingVH Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.pointsToAliveValue(); }
+  bool knownOffset() const { return Offset.pointsToAliveValue(); }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return (Value *)Size == (Value *)RHS.Size &&
+           (Value *)Offset == (Value *)RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetWeakTrackingVH = SizeOffsetType<WeakTrackingVH>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*.
 /// May create code to compute the result at run-time.
 class ObjectSizeOffsetEvaluator
-  : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetEvalType> {
+    : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetValue> {
   using BuilderTy = IRBuilder<TargetFolder, IRBuilderCallbackInserter>;
-  using WeakEvalType = std::pair<WeakTrackingVH, WeakTrackingVH>;
+  using WeakEvalType = SizeOffsetType<WeakTrackingVH>;
   using CacheMapTy = DenseMap<const Value *, WeakEvalType>;
   using PtrSetTy = SmallPtrSet<const Value *, 8>;
 
@@ -275,45 +341,27 @@ class ObjectSizeOffsetEvaluator
   ObjectSizeOpts EvalOpts;
   SmallPtrSet<Instruction *, 8> InsertedInstructions;
 
-  SizeOffsetEvalType compute_(Value *V);
+  SizeOffsetValue compute_(Value *V);
 
 public:
-  static SizeOffsetEvalType unknown() {
-    return std::make_pair(nullptr, nullptr);
-  }
-
   ObjectSizeOffsetEvaluator(const DataLayout &DL, const TargetLibraryInfo *TLI,
                             LLVMContext &Context, ObjectSizeOpts EvalOpts = {});
 
-  SizeOffsetEvalType compute(Value *V);
+  static SizeOffsetValue unknown;
 
-  bool knownSize(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.first;
-  }
-
-  bool knownOffset(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.second;
-  }
-
-  bool anyKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) || knownOffset(SizeOffset);
-  }
-
-  bool bothKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetValue compute(Value *V);
 
   // The individual instruction visitors should be treated as private.
-  SizeOffsetEvalType visitAllocaInst(AllocaInst &I);
-  SizeOffsetEvalType visitCallBase(CallBase &CB);
-  SizeOffsetEvalType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetEvalType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetEvalType visitGEPOperator(GEPOperator &GEP);
-  SizeOffsetEvalType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetEvalType visitLoadInst(LoadInst &I);
-  SizeOffsetEvalType visitPHINode(PHINode &PHI);
-  SizeOffsetEvalType visitSelectInst(SelectInst &I);
-  SizeOffsetEvalType visitInstruction(Instruction &I);
+  SizeOffsetValue visitAllocaInst(AllocaInst &I);
+  SizeOffsetValue visitCallBase(CallBase &CB);
+  SizeOffsetValue visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetValue visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetValue visitGEPOperator(GEPOperator &GEP);
+  SizeOffsetValue visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetValue visitLoadInst(LoadInst &I);
+  SizeOffsetValue visitPHINode(PHINode &PHI);
+  SizeOffsetValue visitSelectInst(SelectInst &I);
+  SizeOffsetValue visitInstruction(Instruction &I);
 };
 
 } // end namespace llvm
diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 9e6811f3bf8815..8cc2d070d1d8e7 100644
--- a/llvm/lib/Analysis/MemoryBuiltins.cpp
+++ b/llvm/lib/Analysis/MemoryBuiltins.cpp
@@ -577,10 +577,12 @@ Value *llvm::getFreedOperand(const CallBase *CB, const TargetLibraryInfo *TLI) {
 //===----------------------------------------------------------------------===//
 //  Utility functions to compute size of objects.
 //
-static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
-  if (Data.second.isNegative() || Data.first.ult(Data.second))
-    return APInt(Data.first.getBitWidth(), 0);
-  return Data.first - Data.second;
+static APInt getSizeWithOverflow(const SizeOffsetAPInt &Data) {
+  APInt Size = Data.Size;
+  APInt Offset = Data.Offset;
+  if (Offset.isNegative() || Size.ult(Size))
+    return APInt(Size.getBitWidth(), 0);
+  return Size - Offset;
 }
 
 /// Compute the size of the object pointed by Ptr. Returns true and the
@@ -590,8 +592,8 @@ static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
 bool llvm::getObjectSize(const Value *Ptr, uint64_t &Size, const DataLayout &DL,
                          const TargetLibraryInfo *TLI, ObjectSizeOpts Opts) {
   ObjectSizeOffsetVisitor Visitor(DL, TLI, Ptr->getContext(), Opts);
-  SizeOffsetType Data = Visitor.compute(const_cast<Value*>(Ptr));
-  if (!Visitor.bothKnown(Data))
+  SizeOffsetAPInt Data = Visitor.compute(const_cast<Value *>(Ptr));
+  if (!Data.bothKnown())
     return false;
 
   Size = getSizeWithOverflow(Data).getZExtValue();
@@ -640,10 +642,9 @@ Value *llvm::lowerObjectSizeCall(
   } else {
     LLVMContext &Ctx = ObjectSize->getFunction()->getContext();
     ObjectSizeOffsetEvaluator Eval(DL, TLI, Ctx, EvalOptions);
-    SizeOffsetEvalType SizeOffsetPair =
-        Eval.compute(ObjectSize->getArgOperand(0));
+    SizeOffsetValue SizeOffsetPair = Eval.compute(ObjectSize->getArgOperand(0));
 
-    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown()) {
+    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown) {
       IRBuilder<TargetFolder, IRBuilderCallbackInserter> Builder(
           Ctx, TargetFolder(DL), IRBuilderCallbackInserter([&](Instruction *I) {
             if (InsertedInstructions)
@@ -651,19 +652,19 @@ Value *llvm::lowerObjectSizeCall(
           }));
       Builder.SetInsertPoint(ObjectSize);
 
+      Value *Size = SizeOffsetPair.Size;
+      Value *Offset = SizeOffsetPair.Offset;
+
       // If we've outside the end of the object, then we can always access
       // exactly 0 bytes.
-      Value *ResultSize =
-          Builder.CreateSub(SizeOffsetPair.first, SizeOffsetPair.second);
-      Value *UseZero =
-          Builder.CreateICmpULT(SizeOffsetPair.first, SizeOffsetPair.second);
+      Value *ResultSize = Builder.CreateSub(Size, Offset);
+      Value *UseZero = Builder.CreateICmpULT(Size, Offset);
       ResultSize = Builder.CreateZExtOrTrunc(ResultSize, ResultType);
       Value *Ret = Builder.CreateSelect(
           UseZero, ConstantInt::get(ResultType, 0), ResultSize);
 
       // The non-constant size expression cannot evaluate to -1.
-      if (!isa<Constant>(SizeOffsetPair.first) ||
-          !isa<Constant>(SizeOffsetPair.second))
+      if (!isa<Constant>(Size) || !isa<Constant>(Offset))
         Builder.CreateAssumption(
             Builder.CreateICmpNE(Ret, ConstantInt::get(ResultType, -1)));
 
@@ -682,6 +683,8 @@ STATISTIC(ObjectVisitorArgument,
 STATISTIC(ObjectVisitorLoad,
           "Number of load instructions with unsolved size and offset");
 
+SizeOffsetAPInt ObjectSizeOffsetVisitor::unknown;
+
 APInt ObjectSizeOffsetVisitor::align(APInt Size, MaybeAlign Alignment) {
   if (Options.RoundToAlign && Alignment)
     return APInt(IntTyBits, alignTo(Size.getZExtValue(), *Alignment));
@@ -697,12 +700,12 @@ ObjectSizeOffsetVisitor::ObjectSizeOffsetVisitor(const DataLayout &DL,
   // a different address space.
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::compute(Value *V) {
   InstructionsVisited = 0;
   return computeImpl(V);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   unsigned InitialIntTyBits = DL.getIndexTypeSizeInBits(V->getType());
 
   // Stripping pointer casts can strip address space casts which can change the
@@ -719,7 +722,7 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   IntTyBits = DL.getIndexTypeSizeInBits(V->getType());
   Zero = APInt::getZero(IntTyBits);
 
-  SizeOffsetType SOT = computeValue(V);
+  SizeOffsetAPInt SOT = computeValue(V);
 
   bool IndexTypeSizeChanged = InitialIntTyBits != IntTyBits;
   if (!IndexTypeSizeChanged && Offset.isZero())
@@ -729,27 +732,28 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   // accumulated some constant offset (or both). Readjust the bit width to match
   // the argument index type size and apply the offset, as required.
   if (IndexTypeSizeChanged) {
-    if (knownSize(SOT) && !::CheckedZextOrTrunc(SOT.first, InitialIntTyBits))
-      SOT.first = APInt();
-    if (knownOffset(SOT) && !::CheckedZextOrTrunc(SOT.second, InitialIntTyBits))
-      SOT.second = APInt();
+    if (SOT.knownSize() && !::CheckedZextOrTrunc(SOT.Size, InitialIntTyBits))
+      SOT.Size = APInt();
+    if (SOT.knownOffset() &&
+        !::CheckedZextOrTrunc(SOT.Offset, InitialIntTyBits))
+      SOT.Offset = APInt();
   }
   // If the computed offset is "unknown" we cannot add the stripped offset.
-  return {SOT.first,
-          SOT.second.getBitWidth() > 1 ? SOT.second + Offset : SOT.second};
+  return {SOT.Size,
+          SOT.Offset.getBitWidth() > 1 ? SOT.Offset + Offset : SOT.Offset};
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeValue(Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V)) {
     // If we have already seen this instruction, bail out. Cycles can happen in
     // unreachable code after constant propagation.
-    auto P = SeenInsts.try_emplace(I, unknown());
+    auto P = SeenInsts.try_emplace(I, ObjectSizeOffsetVisitor::unknown);
     if (!P.second)
       return P.first->second;
     ++InstructionsVisited;
     if (InstructionsVisited > ObjectSizeOffsetVisitorMaxVisitInstructions)
-      return unknown();
-    SizeOffsetType Res = visit(*I);
+      return ObjectSizeOffsetVisitor::unknown;
+    SizeOffsetAPInt Res = visit(*I);
     // Cache the result for later visits. If we happened to visit this during
     // the above recursion, we would consider it unknown until now.
     SeenInsts[I] = Res;
@@ -768,55 +772,55 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
 
   LLVM_DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: "
                     << *V << '\n');
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
 bool ObjectSizeOffsetVisitor::CheckedZextOrTrunc(APInt &I) {
   return ::CheckedZextOrTrunc(I, IntTyBits);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
   TypeSize ElemSize = DL.getTypeAllocSize(I.getAllocatedType());
   if (ElemSize.isScalable() && Options.EvalMode != ObjectSizeOpts::Mode::Min)
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   APInt Size(IntTyBits, ElemSize.getKnownMinValue());
   if (!I.isArrayAllocation())
-    return std::make_pair(align(Size, I.getAlign()), Zero);
+    return SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
 
   Value *ArraySize = I.getArraySize();
   if (const ConstantInt *C = dyn_cast<ConstantInt>(ArraySize)) {
     APInt NumElems = C->getValue();
     if (!CheckedZextOrTrunc(NumElems))
-      return unknown();
+      return ObjectSizeOffsetVisitor::unknown;
 
     bool Overflow;
     Size = Size.umul_ov(NumElems, Overflow);
-    return Overflow ? unknown()
-                    : std::make_pair(align(Size, I.getAlign()), Zero);
+    return Overflow ? ObjectSizeOffsetVisitor::unknown
+                    : SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
   }
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
   Type *MemoryTy = A.getPointeeInMemoryValueType();
   // No interprocedural analysis is done at the moment.
   if (!MemoryTy|| !MemoryTy->isSized()) {
     ++ObjectVisitorArgument;
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   }
 
   APInt Size(IntTyBits, DL.getTypeAllocSize(MemoryTy));
-  return std::make_pair(align(Size, A.getParamAlign()), Zero);
+  return SizeOffsetAPInt(align(Size, A.getParamAlign()), Zero);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
   if (std::optional<APInt> Size = getAllocSize(&CB, TLI))
-    return std::make_pair(*Size, Zero);
-  return unknown();
+    return SizeOffsetAPInt(*Size, Zero);
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull &CPN) {
   // If null is unknown, there's nothing we can do. Additionally, non-zero
   // address spaces can make use of null, so we don't presume to know anything
   // about that.
@@ -825,45 +829,46 @@ ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
   // them on the floor, but it's unclear what we should do when a NULL from
   // addrspace(1) gets casted to addrspace(0) (or vice-versa).
   if (Options.NullIsUnknownSize || CPN.getType()->getAddressSpace())
-    return unknown();
-  return std::make_pair(Zero, Zero);
+    return ObjectSizeOffsetVisitor::unknown;
+  return SizeOffsetAPInt(Zero, Zero);
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst&) {
-  return unknown();
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst &) {
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractValueInst(ExtractValueInst&) {
+Size...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Bill Wendling (bwendling)

Changes

The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)


Patch is 40.34 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76882.diff

5 Files Affected:

  • (modified) llvm/include/llvm/Analysis/MemoryBuiltins.h (+120-72)
  • (modified) llvm/lib/Analysis/MemoryBuiltins.cpp (+163-151)
  • (modified) llvm/lib/Transforms/IPO/AttributorAttributes.cpp (+4-4)
  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+8-4)
  • (modified) llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/MemoryBuiltins.h b/llvm/include/llvm/Analysis/MemoryBuiltins.h
index 827b5081b2ce75..56faa32fb0b226 100644
--- a/llvm/include/llvm/Analysis/MemoryBuiltins.h
+++ b/llvm/include/llvm/Analysis/MemoryBuiltins.h
@@ -187,80 +187,146 @@ Value *lowerObjectSizeCall(
     const TargetLibraryInfo *TLI, AAResults *AA, bool MustSucceed,
     SmallVectorImpl<Instruction *> *InsertedInstructions = nullptr);
 
-using SizeOffsetType = std::pair<APInt, APInt>;
+/// SizeOffsetType - A base template class for the object size visitors. Used
+/// here as a self-documenting way to handle the values rather than using a
+/// \p std::pair.
+template <typename T> struct SizeOffsetType {
+  T Size;
+  T Offset;
+
+  bool knownSize() const;
+  bool knownOffset() const;
+  bool anyKnown() const;
+  bool bothKnown() const;
+};
+
+/// SizeOffsetType<APInt> - Used by \p ObjectSizeOffsetVisitor, which works
+/// with \p APInts.
+template <> struct SizeOffsetType<APInt> {
+  APInt Size;
+  APInt Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(APInt Size, APInt Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.getBitWidth() > 1; }
+  bool knownOffset() const { return Offset.getBitWidth() > 1; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<APInt> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<APInt> &RHS) { return !(*this == RHS); }
+};
+using SizeOffsetAPInt = SizeOffsetType<APInt>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*
 /// statically. Fails if size or offset are not known at compile time.
 class ObjectSizeOffsetVisitor
-  : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {
+    : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetAPInt> {
   const DataLayout &DL;
   const TargetLibraryInfo *TLI;
   ObjectSizeOpts Options;
   unsigned IntTyBits;
   APInt Zero;
-  SmallDenseMap<Instruction *, SizeOffsetType, 8> SeenInsts;
+  SmallDenseMap<Instruction *, SizeOffsetAPInt, 8> SeenInsts;
   unsigned InstructionsVisited;
 
   APInt align(APInt Size, MaybeAlign Align);
 
-  SizeOffsetType unknown() {
-    return std::make_pair(APInt(), APInt());
-  }
+  static SizeOffsetAPInt unknown;
 
 public:
   ObjectSizeOffsetVisitor(const DataLayout &DL, const TargetLibraryInfo *TLI,
                           LLVMContext &Context, ObjectSizeOpts Options = {});
 
-  SizeOffsetType compute(Value *V);
-
-  static bool knownSize(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.first.getBitWidth() > 1;
-  }
-
-  static bool knownOffset(const SizeOffsetType &SizeOffset) {
-    return SizeOffset.second.getBitWidth() > 1;
-  }
-
-  static bool bothKnown(const SizeOffsetType &SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetAPInt compute(Value *V);
 
   // These are "private", except they can't actually be made private. Only
   // compute() should be used by external users.
-  SizeOffsetType visitAllocaInst(AllocaInst &I);
-  SizeOffsetType visitArgument(Argument &A);
-  SizeOffsetType visitCallBase(CallBase &CB);
-  SizeOffsetType visitConstantPointerNull(ConstantPointerNull&);
-  SizeOffsetType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetType visitGlobalAlias(GlobalAlias &GA);
-  SizeOffsetType visitGlobalVariable(GlobalVariable &GV);
-  SizeOffsetType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetType visitLoadInst(LoadInst &I);
-  SizeOffsetType visitPHINode(PHINode&);
-  SizeOffsetType visitSelectInst(SelectInst &I);
-  SizeOffsetType visitUndefValue(UndefValue&);
-  SizeOffsetType visitInstruction(Instruction &I);
+  SizeOffsetAPInt visitAllocaInst(AllocaInst &I);
+  SizeOffsetAPInt visitArgument(Argument &A);
+  SizeOffsetAPInt visitCallBase(CallBase &CB);
+  SizeOffsetAPInt visitConstantPointerNull(ConstantPointerNull &);
+  SizeOffsetAPInt visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetAPInt visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetAPInt visitGlobalAlias(GlobalAlias &GA);
+  SizeOffsetAPInt visitGlobalVariable(GlobalVariable &GV);
+  SizeOffsetAPInt visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetAPInt visitLoadInst(LoadInst &I);
+  SizeOffsetAPInt visitPHINode(PHINode &);
+  SizeOffsetAPInt visitSelectInst(SelectInst &I);
+  SizeOffsetAPInt visitUndefValue(UndefValue &);
+  SizeOffsetAPInt visitInstruction(Instruction &I);
 
 private:
-  SizeOffsetType findLoadSizeOffset(
+  SizeOffsetAPInt findLoadSizeOffset(
       LoadInst &LoadFrom, BasicBlock &BB, BasicBlock::iterator From,
-      SmallDenseMap<BasicBlock *, SizeOffsetType, 8> &VisitedBlocks,
+      SmallDenseMap<BasicBlock *, SizeOffsetAPInt, 8> &VisitedBlocks,
       unsigned &ScannedInstCount);
-  SizeOffsetType combineSizeOffset(SizeOffsetType LHS, SizeOffsetType RHS);
-  SizeOffsetType computeImpl(Value *V);
-  SizeOffsetType computeValue(Value *V);
+  SizeOffsetAPInt combineSizeOffset(SizeOffsetAPInt LHS, SizeOffsetAPInt RHS);
+  SizeOffsetAPInt computeImpl(Value *V);
+  SizeOffsetAPInt computeValue(Value *V);
   bool CheckedZextOrTrunc(APInt &I);
 };
 
-using SizeOffsetEvalType = std::pair<Value *, Value *>;
+template <> struct SizeOffsetType<WeakTrackingVH>;
+
+/// SizeOffsetType<Value *> - Used by \p ObjectSizeOffsetEvaluator, which works
+/// with \p Values.
+template <> struct SizeOffsetType<Value *> {
+  Value *Size;
+  Value *Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+  SizeOffsetType(SizeOffsetType<WeakTrackingVH> &SOT);
+
+  bool knownSize() const { return Size != nullptr; }
+  bool knownOffset() const { return Offset != nullptr; }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return Size == RHS.Size && Offset == RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetValue = SizeOffsetType<Value *>;
+
+/// SizeOffsetType<WeakTrackingVH> - Used by \p ObjectSizeOffsetEvaluator in a
+/// \p DenseMap.
+template <> struct SizeOffsetType<WeakTrackingVH> {
+  WeakTrackingVH Size;
+  WeakTrackingVH Offset;
+
+  SizeOffsetType() = default;
+  SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
+
+  bool knownSize() const { return Size.pointsToAliveValue(); }
+  bool knownOffset() const { return Offset.pointsToAliveValue(); }
+  bool anyKnown() const { return knownSize() || knownOffset(); }
+  bool bothKnown() const { return knownSize() && knownOffset(); }
+
+  bool operator==(const SizeOffsetType<Value *> &RHS) {
+    return (Value *)Size == (Value *)RHS.Size &&
+           (Value *)Offset == (Value *)RHS.Offset;
+  }
+  bool operator!=(const SizeOffsetType<Value *> &RHS) {
+    return !(*this == RHS);
+  }
+};
+using SizeOffsetWeakTrackingVH = SizeOffsetType<WeakTrackingVH>;
 
 /// Evaluate the size and offset of an object pointed to by a Value*.
 /// May create code to compute the result at run-time.
 class ObjectSizeOffsetEvaluator
-  : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetEvalType> {
+    : public InstVisitor<ObjectSizeOffsetEvaluator, SizeOffsetValue> {
   using BuilderTy = IRBuilder<TargetFolder, IRBuilderCallbackInserter>;
-  using WeakEvalType = std::pair<WeakTrackingVH, WeakTrackingVH>;
+  using WeakEvalType = SizeOffsetType<WeakTrackingVH>;
   using CacheMapTy = DenseMap<const Value *, WeakEvalType>;
   using PtrSetTy = SmallPtrSet<const Value *, 8>;
 
@@ -275,45 +341,27 @@ class ObjectSizeOffsetEvaluator
   ObjectSizeOpts EvalOpts;
   SmallPtrSet<Instruction *, 8> InsertedInstructions;
 
-  SizeOffsetEvalType compute_(Value *V);
+  SizeOffsetValue compute_(Value *V);
 
 public:
-  static SizeOffsetEvalType unknown() {
-    return std::make_pair(nullptr, nullptr);
-  }
-
   ObjectSizeOffsetEvaluator(const DataLayout &DL, const TargetLibraryInfo *TLI,
                             LLVMContext &Context, ObjectSizeOpts EvalOpts = {});
 
-  SizeOffsetEvalType compute(Value *V);
+  static SizeOffsetValue unknown;
 
-  bool knownSize(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.first;
-  }
-
-  bool knownOffset(SizeOffsetEvalType SizeOffset) {
-    return SizeOffset.second;
-  }
-
-  bool anyKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) || knownOffset(SizeOffset);
-  }
-
-  bool bothKnown(SizeOffsetEvalType SizeOffset) {
-    return knownSize(SizeOffset) && knownOffset(SizeOffset);
-  }
+  SizeOffsetValue compute(Value *V);
 
   // The individual instruction visitors should be treated as private.
-  SizeOffsetEvalType visitAllocaInst(AllocaInst &I);
-  SizeOffsetEvalType visitCallBase(CallBase &CB);
-  SizeOffsetEvalType visitExtractElementInst(ExtractElementInst &I);
-  SizeOffsetEvalType visitExtractValueInst(ExtractValueInst &I);
-  SizeOffsetEvalType visitGEPOperator(GEPOperator &GEP);
-  SizeOffsetEvalType visitIntToPtrInst(IntToPtrInst&);
-  SizeOffsetEvalType visitLoadInst(LoadInst &I);
-  SizeOffsetEvalType visitPHINode(PHINode &PHI);
-  SizeOffsetEvalType visitSelectInst(SelectInst &I);
-  SizeOffsetEvalType visitInstruction(Instruction &I);
+  SizeOffsetValue visitAllocaInst(AllocaInst &I);
+  SizeOffsetValue visitCallBase(CallBase &CB);
+  SizeOffsetValue visitExtractElementInst(ExtractElementInst &I);
+  SizeOffsetValue visitExtractValueInst(ExtractValueInst &I);
+  SizeOffsetValue visitGEPOperator(GEPOperator &GEP);
+  SizeOffsetValue visitIntToPtrInst(IntToPtrInst &);
+  SizeOffsetValue visitLoadInst(LoadInst &I);
+  SizeOffsetValue visitPHINode(PHINode &PHI);
+  SizeOffsetValue visitSelectInst(SelectInst &I);
+  SizeOffsetValue visitInstruction(Instruction &I);
 };
 
 } // end namespace llvm
diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 9e6811f3bf8815..8cc2d070d1d8e7 100644
--- a/llvm/lib/Analysis/MemoryBuiltins.cpp
+++ b/llvm/lib/Analysis/MemoryBuiltins.cpp
@@ -577,10 +577,12 @@ Value *llvm::getFreedOperand(const CallBase *CB, const TargetLibraryInfo *TLI) {
 //===----------------------------------------------------------------------===//
 //  Utility functions to compute size of objects.
 //
-static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
-  if (Data.second.isNegative() || Data.first.ult(Data.second))
-    return APInt(Data.first.getBitWidth(), 0);
-  return Data.first - Data.second;
+static APInt getSizeWithOverflow(const SizeOffsetAPInt &Data) {
+  APInt Size = Data.Size;
+  APInt Offset = Data.Offset;
+  if (Offset.isNegative() || Size.ult(Size))
+    return APInt(Size.getBitWidth(), 0);
+  return Size - Offset;
 }
 
 /// Compute the size of the object pointed by Ptr. Returns true and the
@@ -590,8 +592,8 @@ static APInt getSizeWithOverflow(const SizeOffsetType &Data) {
 bool llvm::getObjectSize(const Value *Ptr, uint64_t &Size, const DataLayout &DL,
                          const TargetLibraryInfo *TLI, ObjectSizeOpts Opts) {
   ObjectSizeOffsetVisitor Visitor(DL, TLI, Ptr->getContext(), Opts);
-  SizeOffsetType Data = Visitor.compute(const_cast<Value*>(Ptr));
-  if (!Visitor.bothKnown(Data))
+  SizeOffsetAPInt Data = Visitor.compute(const_cast<Value *>(Ptr));
+  if (!Data.bothKnown())
     return false;
 
   Size = getSizeWithOverflow(Data).getZExtValue();
@@ -640,10 +642,9 @@ Value *llvm::lowerObjectSizeCall(
   } else {
     LLVMContext &Ctx = ObjectSize->getFunction()->getContext();
     ObjectSizeOffsetEvaluator Eval(DL, TLI, Ctx, EvalOptions);
-    SizeOffsetEvalType SizeOffsetPair =
-        Eval.compute(ObjectSize->getArgOperand(0));
+    SizeOffsetValue SizeOffsetPair = Eval.compute(ObjectSize->getArgOperand(0));
 
-    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown()) {
+    if (SizeOffsetPair != ObjectSizeOffsetEvaluator::unknown) {
       IRBuilder<TargetFolder, IRBuilderCallbackInserter> Builder(
           Ctx, TargetFolder(DL), IRBuilderCallbackInserter([&](Instruction *I) {
             if (InsertedInstructions)
@@ -651,19 +652,19 @@ Value *llvm::lowerObjectSizeCall(
           }));
       Builder.SetInsertPoint(ObjectSize);
 
+      Value *Size = SizeOffsetPair.Size;
+      Value *Offset = SizeOffsetPair.Offset;
+
       // If we've outside the end of the object, then we can always access
       // exactly 0 bytes.
-      Value *ResultSize =
-          Builder.CreateSub(SizeOffsetPair.first, SizeOffsetPair.second);
-      Value *UseZero =
-          Builder.CreateICmpULT(SizeOffsetPair.first, SizeOffsetPair.second);
+      Value *ResultSize = Builder.CreateSub(Size, Offset);
+      Value *UseZero = Builder.CreateICmpULT(Size, Offset);
       ResultSize = Builder.CreateZExtOrTrunc(ResultSize, ResultType);
       Value *Ret = Builder.CreateSelect(
           UseZero, ConstantInt::get(ResultType, 0), ResultSize);
 
       // The non-constant size expression cannot evaluate to -1.
-      if (!isa<Constant>(SizeOffsetPair.first) ||
-          !isa<Constant>(SizeOffsetPair.second))
+      if (!isa<Constant>(Size) || !isa<Constant>(Offset))
         Builder.CreateAssumption(
             Builder.CreateICmpNE(Ret, ConstantInt::get(ResultType, -1)));
 
@@ -682,6 +683,8 @@ STATISTIC(ObjectVisitorArgument,
 STATISTIC(ObjectVisitorLoad,
           "Number of load instructions with unsolved size and offset");
 
+SizeOffsetAPInt ObjectSizeOffsetVisitor::unknown;
+
 APInt ObjectSizeOffsetVisitor::align(APInt Size, MaybeAlign Alignment) {
   if (Options.RoundToAlign && Alignment)
     return APInt(IntTyBits, alignTo(Size.getZExtValue(), *Alignment));
@@ -697,12 +700,12 @@ ObjectSizeOffsetVisitor::ObjectSizeOffsetVisitor(const DataLayout &DL,
   // a different address space.
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::compute(Value *V) {
   InstructionsVisited = 0;
   return computeImpl(V);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   unsigned InitialIntTyBits = DL.getIndexTypeSizeInBits(V->getType());
 
   // Stripping pointer casts can strip address space casts which can change the
@@ -719,7 +722,7 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   IntTyBits = DL.getIndexTypeSizeInBits(V->getType());
   Zero = APInt::getZero(IntTyBits);
 
-  SizeOffsetType SOT = computeValue(V);
+  SizeOffsetAPInt SOT = computeValue(V);
 
   bool IndexTypeSizeChanged = InitialIntTyBits != IntTyBits;
   if (!IndexTypeSizeChanged && Offset.isZero())
@@ -729,27 +732,28 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   // accumulated some constant offset (or both). Readjust the bit width to match
   // the argument index type size and apply the offset, as required.
   if (IndexTypeSizeChanged) {
-    if (knownSize(SOT) && !::CheckedZextOrTrunc(SOT.first, InitialIntTyBits))
-      SOT.first = APInt();
-    if (knownOffset(SOT) && !::CheckedZextOrTrunc(SOT.second, InitialIntTyBits))
-      SOT.second = APInt();
+    if (SOT.knownSize() && !::CheckedZextOrTrunc(SOT.Size, InitialIntTyBits))
+      SOT.Size = APInt();
+    if (SOT.knownOffset() &&
+        !::CheckedZextOrTrunc(SOT.Offset, InitialIntTyBits))
+      SOT.Offset = APInt();
   }
   // If the computed offset is "unknown" we cannot add the stripped offset.
-  return {SOT.first,
-          SOT.second.getBitWidth() > 1 ? SOT.second + Offset : SOT.second};
+  return {SOT.Size,
+          SOT.Offset.getBitWidth() > 1 ? SOT.Offset + Offset : SOT.Offset};
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::computeValue(Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V)) {
     // If we have already seen this instruction, bail out. Cycles can happen in
     // unreachable code after constant propagation.
-    auto P = SeenInsts.try_emplace(I, unknown());
+    auto P = SeenInsts.try_emplace(I, ObjectSizeOffsetVisitor::unknown);
     if (!P.second)
       return P.first->second;
     ++InstructionsVisited;
     if (InstructionsVisited > ObjectSizeOffsetVisitorMaxVisitInstructions)
-      return unknown();
-    SizeOffsetType Res = visit(*I);
+      return ObjectSizeOffsetVisitor::unknown;
+    SizeOffsetAPInt Res = visit(*I);
     // Cache the result for later visits. If we happened to visit this during
     // the above recursion, we would consider it unknown until now.
     SeenInsts[I] = Res;
@@ -768,55 +772,55 @@ SizeOffsetType ObjectSizeOffsetVisitor::computeValue(Value *V) {
 
   LLVM_DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: "
                     << *V << '\n');
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
 bool ObjectSizeOffsetVisitor::CheckedZextOrTrunc(APInt &I) {
   return ::CheckedZextOrTrunc(I, IntTyBits);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
   TypeSize ElemSize = DL.getTypeAllocSize(I.getAllocatedType());
   if (ElemSize.isScalable() && Options.EvalMode != ObjectSizeOpts::Mode::Min)
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   APInt Size(IntTyBits, ElemSize.getKnownMinValue());
   if (!I.isArrayAllocation())
-    return std::make_pair(align(Size, I.getAlign()), Zero);
+    return SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
 
   Value *ArraySize = I.getArraySize();
   if (const ConstantInt *C = dyn_cast<ConstantInt>(ArraySize)) {
     APInt NumElems = C->getValue();
     if (!CheckedZextOrTrunc(NumElems))
-      return unknown();
+      return ObjectSizeOffsetVisitor::unknown;
 
     bool Overflow;
     Size = Size.umul_ov(NumElems, Overflow);
-    return Overflow ? unknown()
-                    : std::make_pair(align(Size, I.getAlign()), Zero);
+    return Overflow ? ObjectSizeOffsetVisitor::unknown
+                    : SizeOffsetAPInt(align(Size, I.getAlign()), Zero);
   }
-  return unknown();
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
   Type *MemoryTy = A.getPointeeInMemoryValueType();
   // No interprocedural analysis is done at the moment.
   if (!MemoryTy|| !MemoryTy->isSized()) {
     ++ObjectVisitorArgument;
-    return unknown();
+    return ObjectSizeOffsetVisitor::unknown;
   }
 
   APInt Size(IntTyBits, DL.getTypeAllocSize(MemoryTy));
-  return std::make_pair(align(Size, A.getParamAlign()), Zero);
+  return SizeOffsetAPInt(align(Size, A.getParamAlign()), Zero);
 }
 
-SizeOffsetType ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
+SizeOffsetAPInt ObjectSizeOffsetVisitor::visitCallBase(CallBase &CB) {
   if (std::optional<APInt> Size = getAllocSize(&CB, TLI))
-    return std::make_pair(*Size, Zero);
-  return unknown();
+    return SizeOffsetAPInt(*Size, Zero);
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull &CPN) {
   // If null is unknown, there's nothing we can do. Additionally, non-zero
   // address spaces can make use of null, so we don't presume to know anything
   // about that.
@@ -825,45 +829,46 @@ ObjectSizeOffsetVisitor::visitConstantPointerNull(ConstantPointerNull& CPN) {
   // them on the floor, but it's unclear what we should do when a NULL from
   // addrspace(1) gets casted to addrspace(0) (or vice-versa).
   if (Options.NullIsUnknownSize || CPN.getType()->getAddressSpace())
-    return unknown();
-  return std::make_pair(Zero, Zero);
+    return ObjectSizeOffsetVisitor::unknown;
+  return SizeOffsetAPInt(Zero, Zero);
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst&) {
-  return unknown();
+SizeOffsetAPInt
+ObjectSizeOffsetVisitor::visitExtractElementInst(ExtractElementInst &) {
+  return ObjectSizeOffsetVisitor::unknown;
 }
 
-SizeOffsetType
-ObjectSizeOffsetVisitor::visitExtractValueInst(ExtractValueInst&) {
+Size...
[truncated]

@bwendling bwendling changed the title [builtin_object_size] Use classes instead of std::pair (NFC) [NFC][ObjectSizeOffset] Use classes instead of std::pair Jan 4, 2024
Copy link
Member

@jdoerfert jdoerfert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, getting rid of the pair is great.
I am unsure I understand why we do the base template rather than inheritance. Where is the base template used? If we do inheritance we could avoid duplicating of members and provide default impls that work with many things, e.g., all operator== are the same.


SizeOffsetType() = default;
SizeOffsetType(Value *Size, Value *Offset) : Size(Size), Offset(Offset) {}
SizeOffsetType(SizeOffsetType<WeakTrackingVH> &SOT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, where is this defined?

Copy link
Collaborator Author

@bwendling bwendling Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SizeOffsetType<WeakTrackingVH> is defined just below this struct. I had to do it this way because they both reference the other. (Well, I suppose I could have put the WeakTrackingVH first, but the Value * one was already here. :-)

Edit: Oh! I see what you mean. I defined it in MemoryBuiltin.cpp. I can't define it in the header here (after the WeakTrackingVH struct) because it'll have multiple definitions.

@bwendling
Copy link
Collaborator Author

I am unsure I understand why we do the base template rather than inheritance. Where is the base template used? If we do inheritance we could avoid duplicating of members and provide default impls that work with many things, e.g., all operator== are the same.

I made these templates because they're dealing with different types. I can try to avoid duplicating the comparison functions, but I don't know if we'll get much out of inheritance that we don't get with templates...or really just different structs.

@bwendling
Copy link
Collaborator Author

@jdoerfert I reworked the structs to use inheritance instead of template specialization. It should work better now and not duplicate so much.

Copy link

github-actions bot commented Jan 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@jdoerfert jdoerfert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one more comment. I think the new classes are clearly superior to the pairs. I looked over the changes, nothing jumps out. LG

llvm/include/llvm/Analysis/MemoryBuiltins.h Outdated Show resolved Hide resolved
@bwendling bwendling merged commit fc6b566 into llvm:main Jan 6, 2024
3 of 4 checks passed
@bwendling bwendling deleted the object-size-refab branch January 6, 2024 02:09
@dyung
Copy link
Collaborator

dyung commented Jan 6, 2024

Hi @bwendling, your change to MemoryBuiltins.h is hitting an error in the version of Visual Studio 2019 that we use internally to build:

C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(217): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(279): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(292): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'

From the Visual Studio documentation for the error, it appears that we should be able to work around this error by modifying the friend declaration like this:

-  friend class SizeOffsetType;
+  template <typename T, class C> friend class SizeOffsetType;

When I made this change to the 3 locations that caused the error (lines 217/279/292), I was able to build successfully with Visual Studio. Could we update the friend declaration so that it also works with the version of Visual Studio that we are using internally to build?

@bwendling
Copy link
Collaborator Author

Hi @bwendling, your change to MemoryBuiltins.h is hitting an error in the version of Visual Studio 2019 that we use internally to build:

C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(217): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(279): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(292): error C2990: 'llvm::SizeOffsetType': non-class template has already been declared as a class template
C:\j\w\779ddbee\p\llvm\include\llvm/Analysis/MemoryBuiltins.h(193): note: see declaration of 'llvm::SizeOffsetType'

From the Visual Studio documentation for the error, it appears that we should be able to work around this error by modifying the friend declaration like this:

-  friend class SizeOffsetType;
+  template <typename T, class C> friend class SizeOffsetType;

When I made this change to the 3 locations that caused the error (lines 217/279/292), I was able to build successfully with Visual Studio. Could we update the friend declaration so that it also works with the version of Visual Studio that we are using internally to build?

Sorry about that. I pushed a fix here:

To https://github.com/llvm/llvm-project.git
3eb9fd8..0903d99 main -> main

@bwendling
Copy link
Collaborator Author

Actually, that fix was making all of the other builds fail:

/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include/llvm/Analysis/MemoryBuiltins.h:279:47: error: specialization of ‘llvm::SizeOffsetType<llvm::Value*, llvm::SizeOffsetValue>’ after instantiation
  279 |   template <typename T, class C> friend class SizeOffsetType;
      |                                               ^~~~~~~~~~~~~~
/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include/llvm/Analysis/MemoryBuiltins.h:292:47: error: specialization of ‘llvm::SizeOffsetType<llvm::WeakTrackingVH, llvm::SizeOffsetWeakTrackingVH>’ after instantiation
  292 |   template <typename T, class C> friend class SizeOffsetType;
      |                                               ^~~~~~~~~~~~~~
32.474 [1989/19/1551] Building CXX object lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o
FAILED: lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o 
ccache /usr/bin/c++ -DCPUINFO_SUPPORTED_PLATFORM=1 -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/ml-opt-dev-x86-64-b1/build/lib/Transforms/IPO -I/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/lib/Transforms/IPO -I/b/ml-opt-dev-x86-64-b1/build/include -I/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include -isystem /tmp/tflitebuild/tensorflow/include -isystem /tmp/tflitebuild/eigen/include/eigen3 -isystem /tmp/tflitebuild/abseil-cpp/include -isystem /tmp/tflitebuild/flatbuffers/include -isystem /tmp/tflitebuild/gemmlowp/include/gemmlowp -isystem /tmp/tflitebuild/ml_dtypes/src/ml_dtypes -isystem /tmp/tflitebuild/ml_dtypes/src/ml_dtypes/ml_dtypes -isystem /tmp/tflitebuild/ruy/include -isystem /tmp/tflitebuild/cpuinfo/include -isystem /tmp/tflitebuild/ARM_NEON_2_x86_SSE/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -DEIGEN_NEON_GEBP_NR=4 -DTFL_STATIC_LIBRARY_BUILD -std=c++17 -MD -MT lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o -MF lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o.d -o lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o -c /b/ml-opt-dev-x86-64-b1/llvm-project/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
In file included from /b/ml-opt-dev-x86-64-b1/llvm-project/llvm/lib/Transforms/IPO/AttributorAttributes.cpp:35:
/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include/llvm/Analysis/MemoryBuiltins.h:217:47: error: specialization of ‘llvm::SizeOffsetType<llvm::APInt, llvm::SizeOffsetAPInt>’ after instantiation
  217 |   template <typename T, class C> friend class SizeOffsetType;
      |                                               ^~~~~~~~~~~~~~
/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include/llvm/Analysis/MemoryBuiltins.h:279:47: error: specialization of ‘llvm::SizeOffsetType<llvm::Value*, llvm::SizeOffsetValue>’ after instantiation
  279 |   template <typename T, class C> friend class SizeOffsetType;
      |                                               ^~~~~~~~~~~~~~
/b/ml-opt-dev-x86-64-b1/llvm-project/llvm/include/llvm/Analysis/MemoryBuiltins.h:292:47: error: specialization of ‘llvm::SizeOffsetType<llvm::WeakTrackingVH, llvm::SizeOffsetWeakTrackingVH>’ after instantiation
  292 |   template <typename T, class C> friend class SizeOffsetType;
      |                                               ^~~~~~~~~~~~~~
33.552 [1989/18/1552] Building AMDGPUGenMCPseudoLowering.inc...

So I don't know what can be done here. Is it possible for Visual Studio not to use a broken compiler?

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
The use of std::pair makes the values it holds opaque. Using classes
improves this while keeping the POD aspect of a std::pair. As a nice
addition, the "known" functions held inappropriately in the Visitor
classes can now properly reside in the value classes. :-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants