Rename NumOperands to make it clear its managed by the User. NFC.

This is to try make it very clear that subclasses shouldn't be changing
the value directly.  Now that OperandList for normal instructions is computed
using the NumOperands, its critical that the NumOperands is accurate or we
could compute the wrong offset to the first operand.

I looked over all places which update NumOperands and they are all safe.
Hung off use User's don't use NumOperands to compute the OperandList so they
are safe to continue to manipulate it.  The only other User which changed it
was GlobalVariable which has an optional init list but always allocated space
for a single Use.  It was correctly setting NumOperands to 1 before setting an
initializer, and setting it to 0 after clearing the init list, so the order was safe.

Added some comments to that code to make sure that this isn't changed in future
without being aware of this constraint.

Reviewed by Duncan Exon Smith.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@239621 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/IR/GlobalVariable.h b/include/llvm/IR/GlobalVariable.h
index 9f57705dae72e5dba7c145ab189823a2bc5802f3..126bebbdcb15743e79ca0d0554999959199ae8b5 100644 (file)
--- a/include/llvm/IR/GlobalVariable.h
+++ b/include/llvm/IR/GlobalVariable.h
@@ -67,7 +67,8 @@ public:
                  bool isExternallyInitialized = false);
 
   ~GlobalVariable() override {
-    NumOperands = 1; // FIXME: needed by operator delete
+    // FIXME: needed by operator delete
+    setGlobalVariableNumOperands(1);
   }
 
   /// Provide fast operand accessors

diff --git a/include/llvm/IR/Instructions.h b/include/llvm/IR/Instructions.h
index 39a62656934c9e188ee33faf0d8618b7a2b3ff71..00283766c783045bb8bd2a1dadc05e8869867a86 100644 (file)
--- a/include/llvm/IR/Instructions.h
+++ b/include/llvm/IR/Instructions.h
@@ -2350,12 +2350,12 @@ public:
     assert(BB && "PHI node got a null basic block!");
     assert(getType() == V->getType() &&
            "All operands to PHI node must be the same type as the PHI node!");
-    if (NumOperands == ReservedSpace)
+    if (getNumOperands() == ReservedSpace)
       growOperands();  // Get more space!
     // Initialize some new operands.
-    ++NumOperands;
-    setIncomingValue(NumOperands - 1, V);
-    setIncomingBlock(NumOperands - 1, BB);
+    setNumHungOffUseOperands(getNumOperands() + 1);
+    setIncomingValue(getNumOperands() - 1, V);
+    setIncomingBlock(getNumOperands() - 1, BB);
   }
 
   /// removeIncomingValue - Remove an incoming value.  This is useful if a

diff --git a/include/llvm/IR/User.h b/include/llvm/IR/User.h
index fffa2cc3d4baa0f56fd7aba0301fa939898fa026..3b72cfe1587b45630f269fe0cc31f791a47a893c 100644 (file)
--- a/include/llvm/IR/User.h
+++ b/include/llvm/IR/User.h
@@ -53,7 +53,8 @@ protected:
   User(Type *ty, unsigned vty, Use *OpList, unsigned NumOps)
       : Value(ty, vty) {
     setOperandList(OpList);
-    NumOperands = NumOps;
+    assert(NumOps < (1u << NumUserOperandsBits) && "Too many operands");
+    NumUserOperands = NumOps;
   }
 
   /// \brief Allocate the array of Uses, followed by a pointer
@@ -69,11 +70,12 @@ protected:
 public:
   ~User() override {
     // drop the hung off uses.
-    Use::zap(getOperandList(), getOperandList() + NumOperands, HasHungOffUses);
+    Use::zap(getOperandList(), getOperandList() + NumUserOperands,
+             HasHungOffUses);
     if (HasHungOffUses) {
       setOperandList(nullptr);
       // Reset NumOperands so User::operator delete() does the right thing.
-      NumOperands = 0;
+      NumUserOperands = 0;
     }
   }
   /// \brief Free memory allocated for User and Use objects.
@@ -107,26 +109,48 @@ public:
     return LegacyOperandList;
   }
   Value *getOperand(unsigned i) const {
-    assert(i < NumOperands && "getOperand() out of range!");
+    assert(i < NumUserOperands && "getOperand() out of range!");
     return getOperandList()[i];
   }
   void setOperand(unsigned i, Value *Val) {
-    assert(i < NumOperands && "setOperand() out of range!");
+    assert(i < NumUserOperands && "setOperand() out of range!");
     assert((!isa<Constant>((const Value*)this) ||
             isa<GlobalValue>((const Value*)this)) &&
            "Cannot mutate a constant with setOperand!");
     getOperandList()[i] = Val;
   }
   const Use &getOperandUse(unsigned i) const {
-    assert(i < NumOperands && "getOperandUse() out of range!");
+    assert(i < NumUserOperands && "getOperandUse() out of range!");
     return getOperandList()[i];
   }
   Use &getOperandUse(unsigned i) {
-    assert(i < NumOperands && "getOperandUse() out of range!");
+    assert(i < NumUserOperands && "getOperandUse() out of range!");
     return getOperandList()[i];
   }
 
-  unsigned getNumOperands() const { return NumOperands; }
+  unsigned getNumOperands() const { return NumUserOperands; }
+
+  /// Set the number of operands on a GlobalVariable.
+  ///
+  /// GlobalVariable always allocates space for a single operands, but
+  /// doesn't always use it.
+  ///
+  /// FIXME: As that the number of operands is used to find the start of
+  /// the allocated memory in operator delete, we need to always think we have
+  /// 1 operand before delete.
+  void setGlobalVariableNumOperands(unsigned NumOps) {
+    assert(NumOps <= 1 && "GlobalVariable can only have 0 or 1 operands");
+    NumUserOperands = NumOps;
+  }
+
+  /// \brief Subclasses with hung off uses need to manage the operand count
+  /// themselves.  In these instances, the operand count isn't used to find the
+  /// OperandList, so there's no issue in having the operand count change.
+  void setNumHungOffUseOperands(unsigned NumOps) {
+    assert(HasHungOffUses && "Must have hung off uses to use this method");
+    assert(NumOps < (1u << NumUserOperandsBits) && "Too many operands");
+    NumUserOperands = NumOps;
+  }
 
   // ---------------------------------------------------------------------------
   // Operand Iterator interface...
@@ -139,10 +163,10 @@ public:
   inline op_iterator       op_begin()       { return getOperandList(); }
   inline const_op_iterator op_begin() const { return getOperandList(); }
   inline op_iterator       op_end()         {
-    return getOperandList() + NumOperands;
+    return getOperandList() + NumUserOperands;
   }
   inline const_op_iterator op_end()   const {
-    return getOperandList() + NumOperands;
+    return getOperandList() + NumUserOperands;
   }
   inline op_range operands() {
     return op_range(op_begin(), op_end());

diff --git a/include/llvm/IR/Value.h b/include/llvm/IR/Value.h
index 83df4cddd7f2e674a823c18ea3c3c6cca6366aa9..b175e0ee96976a6b8d779c69b1c67bec90fa4d7d 100644 (file)
--- a/include/llvm/IR/Value.h
+++ b/include/llvm/IR/Value.h
@@ -100,7 +100,11 @@ protected:
   /// This is stored here to save space in User on 64-bit hosts.  Since most
   /// instances of Value have operands, 32-bit hosts aren't significantly
   /// affected.
-  unsigned NumOperands : 29;
+  ///
+  /// Note, this should *NOT* be used directly by any class other than User.
+  /// User uses this value to find the Use list.
+  static const unsigned NumUserOperandsBits = 29;
+  unsigned NumUserOperands : 29;
 
   bool IsUsedByMD : 1;
   bool HasName : 1;

diff --git a/lib/IR/Globals.cpp b/lib/IR/Globals.cpp
index 1028e33eae6471f167be26bec221da97edbb5446..79a458c26f770ce1287b16748dd339ead88fb049 100644 (file)
--- a/lib/IR/Globals.cpp
+++ b/lib/IR/Globals.cpp
@@ -214,14 +214,20 @@ void GlobalVariable::replaceUsesOfWithOnConstant(Value *From, Value *To,
 void GlobalVariable::setInitializer(Constant *InitVal) {
   if (!InitVal) {
     if (hasInitializer()) {
+      // Note, the num operands is used to compute the offset of the operand, so
+      // the order here matters.  Clearing the operand then clearing the num
+      // operands ensures we have the correct offset to the operand.
       Op<0>().set(nullptr);
-      NumOperands = 0;
+      setGlobalVariableNumOperands(0);
     }
   } else {
     assert(InitVal->getType() == getType()->getElementType() &&
            "Initializer type must match GlobalVariable type");
+    // Note, the num operands is used to compute the offset of the operand, so
+    // the order here matters.  We need to set num operands to 1 first so that
+    // we get the correct offset to the first operand when we set it.
     if (!hasInitializer())
-      NumOperands = 1;
+      setGlobalVariableNumOperands(1);
     Op<0>().set(InitVal);
   }
 }

diff --git a/lib/IR/Instructions.cpp b/lib/IR/Instructions.cpp
index dff31bee093595ffef6a997866333e0feeacfc4d..0e2fe391aa631c2b5406e8a9ca980019933324a8 100644 (file)
--- a/lib/IR/Instructions.cpp
+++ b/lib/IR/Instructions.cpp
@@ -108,7 +108,7 @@ Value *PHINode::removeIncomingValue(unsigned Idx, bool DeletePHIIfEmpty) {
 
   // Nuke the last value.
   Op<-1>().set(nullptr);
-  --NumOperands;
+  setNumHungOffUseOperands(getNumOperands() - 1);
 
   // If the PHI node is dead, because it has zero entries, nuke it now.
   if (getNumOperands() == 0 && DeletePHIIfEmpty) {
@@ -199,7 +199,7 @@ LandingPadInst *LandingPadInst::Create(Type *RetTy, Value *PersonalityFn,
 void LandingPadInst::init(Value *PersFn, unsigned NumReservedValues,
                           const Twine &NameStr) {
   ReservedSpace = NumReservedValues;
-  NumOperands = 1;
+  setNumHungOffUseOperands(1);
   allocHungoffUses(ReservedSpace);
   Op<0>() = PersFn;
   setName(NameStr);
@@ -219,7 +219,7 @@ void LandingPadInst::addClause(Constant *Val) {
   unsigned OpNo = getNumOperands();
   growOperands(1);
   assert(OpNo < ReservedSpace && "Growing didn't work!");
-  ++NumOperands;
+  setNumHungOffUseOperands(getNumOperands() + 1);
   getOperandList()[OpNo] = Val;
 }
 
@@ -233,7 +233,7 @@ CallInst::~CallInst() {
 void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
                     const Twine &NameStr) {
   this->FTy = FTy;
-  assert(NumOperands == Args.size() + 1 && "NumOperands not set up?");
+  assert(getNumOperands() == Args.size() + 1 && "NumOperands not set up?");
   Op<-1>() = Func;
 
 #ifndef NDEBUG
@@ -254,7 +254,7 @@ void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
 void CallInst::init(Value *Func, const Twine &NameStr) {
   FTy =
       cast<FunctionType>(cast<PointerType>(Func->getType())->getElementType());
-  assert(NumOperands == 1 && "NumOperands not set up?");
+  assert(getNumOperands() == 1 && "NumOperands not set up?");
   Op<-1>() = Func;
 
   assert(FTy->getNumParams() == 0 && "Calling a function with bad signature");
@@ -509,7 +509,7 @@ void InvokeInst::init(FunctionType *FTy, Value *Fn, BasicBlock *IfNormal,
                       const Twine &NameStr) {
   this->FTy = FTy;
 
-  assert(NumOperands == 3 + Args.size() && "NumOperands not set up?");
+  assert(getNumOperands() == 3 + Args.size() && "NumOperands not set up?");
   Op<-3>() = Fn;
   Op<-2>() = IfNormal;
   Op<-1>() = IfException;
@@ -1205,7 +1205,8 @@ FenceInst::FenceInst(LLVMContext &C, AtomicOrdering Ordering,
 
 void GetElementPtrInst::init(Value *Ptr, ArrayRef<Value *> IdxList,
                              const Twine &Name) {
-  assert(NumOperands == 1 + IdxList.size() && "NumOperands not initialized?");
+  assert(getNumOperands() == 1 + IdxList.size() &&
+         "NumOperands not initialized?");
   Op<0>() = Ptr;
   std::copy(IdxList.begin(), IdxList.end(), op_begin() + 1);
   setName(Name);
@@ -1518,7 +1519,7 @@ void ShuffleVectorInst::getShuffleMask(Constant *Mask,
 
 void InsertValueInst::init(Value *Agg, Value *Val, ArrayRef<unsigned> Idxs, 
                            const Twine &Name) {
-  assert(NumOperands == 2 && "NumOperands not initialized?");
+  assert(getNumOperands() == 2 && "NumOperands not initialized?");
 
   // There's no fundamental reason why we require at least one index
   // (other than weirdness with &*IdxBegin being invalid; see
@@ -1549,7 +1550,7 @@ InsertValueInst::InsertValueInst(const InsertValueInst &IVI)
 //===----------------------------------------------------------------------===//
 
 void ExtractValueInst::init(ArrayRef<unsigned> Idxs, const Twine &Name) {
-  assert(NumOperands == 1 && "NumOperands not initialized?");
+  assert(getNumOperands() == 1 && "NumOperands not initialized?");
 
   // There's no fundamental reason why we require at least one index.
   // But there's no present need to support it.
@@ -3263,7 +3264,7 @@ bool CmpInst::isFalseWhenEqual(unsigned short predicate) {
 void SwitchInst::init(Value *Value, BasicBlock *Default, unsigned NumReserved) {
   assert(Value && Default && NumReserved);
   ReservedSpace = NumReserved;
-  NumOperands = 2;
+  setNumHungOffUseOperands(2);
   allocHungoffUses(ReservedSpace);
 
   Op<0>() = Value;
@@ -3295,7 +3296,7 @@ SwitchInst::SwitchInst(Value *Value, BasicBlock *Default, unsigned NumCases,
 SwitchInst::SwitchInst(const SwitchInst &SI)
   : TerminatorInst(SI.getType(), Instruction::Switch, nullptr, 0) {
   init(SI.getCondition(), SI.getDefaultDest(), SI.getNumOperands());
-  NumOperands = SI.getNumOperands();
+  setNumHungOffUseOperands(SI.getNumOperands());
   Use *OL = getOperandList();
   const Use *InOL = SI.getOperandList();
   for (unsigned i = 2, E = SI.getNumOperands(); i != E; i += 2) {
@@ -3309,13 +3310,13 @@ SwitchInst::SwitchInst(const SwitchInst &SI)
 /// addCase - Add an entry to the switch instruction...
 ///
 void SwitchInst::addCase(ConstantInt *OnVal, BasicBlock *Dest) {
-  unsigned NewCaseIdx = getNumCases(); 
-  unsigned OpNo = NumOperands;
+  unsigned NewCaseIdx = getNumCases();
+  unsigned OpNo = getNumOperands();
   if (OpNo+2 > ReservedSpace)
     growOperands();  // Get more space!
   // Initialize some new operands.
   assert(OpNo+1 < ReservedSpace && "Growing didn't work!");
-  NumOperands = OpNo+2;
+  setNumHungOffUseOperands(OpNo+2);
   CaseIt Case(this, NewCaseIdx);
   Case.setValue(OnVal);
   Case.setSuccessor(Dest);
@@ -3340,7 +3341,7 @@ void SwitchInst::removeCase(CaseIt i) {
   // Nuke the last value.
   OL[NumOps-2].set(nullptr);
   OL[NumOps-2+1].set(nullptr);
-  NumOperands = NumOps-2;
+  setNumHungOffUseOperands(NumOps-2);
 }
 
 /// growOperands - grow operands - This grows the operand list in response
@@ -3373,7 +3374,7 @@ void IndirectBrInst::init(Value *Address, unsigned NumDests) {
   assert(Address && Address->getType()->isPointerTy() &&
          "Address of indirectbr must be a pointer");
   ReservedSpace = 1+NumDests;
-  NumOperands = 1;
+  setNumHungOffUseOperands(1);
   allocHungoffUses(ReservedSpace);
 
   Op<0>() = Address;
@@ -3419,12 +3420,12 @@ IndirectBrInst::IndirectBrInst(const IndirectBrInst &IBI)
 /// addDestination - Add a destination.
 ///
 void IndirectBrInst::addDestination(BasicBlock *DestBB) {
-  unsigned OpNo = NumOperands;
+  unsigned OpNo = getNumOperands();
   if (OpNo+1 > ReservedSpace)
     growOperands();  // Get more space!
   // Initialize some new operands.
   assert(OpNo < ReservedSpace && "Growing didn't work!");
-  NumOperands = OpNo+1;
+  setNumHungOffUseOperands(OpNo+1);
   getOperandList()[OpNo] = DestBB;
 }
 
@@ -3441,7 +3442,7 @@ void IndirectBrInst::removeDestination(unsigned idx) {
   
   // Nuke the last value.
   OL[NumOps-1].set(nullptr);
-  NumOperands = NumOps-1;
+  setNumHungOffUseOperands(NumOps-1);
 }
 
 BasicBlock *IndirectBrInst::getSuccessorV(unsigned idx) const {

diff --git a/lib/IR/User.cpp b/lib/IR/User.cpp
index 0cba526f3183ae5f196abfdf382830c0643f96f2..1084a505cfd0352cb389bd2dc283bc1404471b28 100644 (file)
--- a/lib/IR/User.cpp
+++ b/lib/IR/User.cpp
@@ -86,13 +86,14 @@ void User::growHungoffUses(unsigned NewNumUses, bool IsPhi) {
 //===----------------------------------------------------------------------===//
 
 void *User::operator new(size_t s, unsigned Us) {
+  assert(Us < (1u << NumUserOperandsBits) && "Too many operands");
   void *Storage = ::operator new(s + sizeof(Use) * Us);
   Use *Start = static_cast<Use*>(Storage);
   Use *End = Start + Us;
   User *Obj = reinterpret_cast<User*>(End);
   Obj->setOperandList(Start);
   Obj->HasHungOffUses = false;
-  Obj->NumOperands = Us;
+  Obj->NumUserOperands = Us;
   Use::initTags(Start, End);
   return Obj;
 }
@@ -103,7 +104,7 @@ void *User::operator new(size_t s, unsigned Us) {
 
 void User::operator delete(void *Usr) {
   User *Start = static_cast<User*>(Usr);
-  Use *Storage = static_cast<Use*>(Usr) - Start->NumOperands;
+  Use *Storage = static_cast<Use*>(Usr) - Start->NumUserOperands;
   // If there were hung-off uses, they will have been freed already and
   // NumOperands reset to 0, so here we just free the User itself.
   ::operator delete(Storage);

diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp
index dcf0ad50190f63c10dde574d7841c48e3be02de4..eb5c2253f4e0c81278310f85c35fca52e638df20 100644 (file)
--- a/lib/IR/Value.cpp
+++ b/lib/IR/Value.cpp
@@ -39,6 +39,7 @@ using namespace llvm;
 //===----------------------------------------------------------------------===//
 //                                Value Class
 //===----------------------------------------------------------------------===//
+const unsigned Value::NumUserOperandsBits;
 
 static inline Type *checkType(Type *Ty) {
   assert(Ty && "Value defined with a null type: Error!");
@@ -48,7 +49,7 @@ static inline Type *checkType(Type *Ty) {
 Value::Value(Type *ty, unsigned scid)
     : VTy(checkType(ty)), UseList(nullptr), SubclassID(scid),
       HasValueHandle(0), SubclassOptionalData(0), SubclassData(0),
-      NumOperands(0), IsUsedByMD(false), HasName(false) {
+      NumUserOperands(0), IsUsedByMD(false), HasName(false) {
   // FIXME: Why isn't this in the subclass gunk??
   // Note, we cannot call isa<CallInst> before the CallInst has been
   // constructed.