[llvm] r239621 - Rename NumOperands to make it clear its managed by the User. NFC.

Pete Cooper peter_cooper at apple.com
Fri Jun 12 10:48:10 PDT 2015


Author: pete
Date: Fri Jun 12 12:48:10 2015
New Revision: 239621

URL: http://llvm.org/viewvc/llvm-project?rev=239621&view=rev
Log:
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.

Modified:
    llvm/trunk/include/llvm/IR/GlobalVariable.h
    llvm/trunk/include/llvm/IR/Instructions.h
    llvm/trunk/include/llvm/IR/User.h
    llvm/trunk/include/llvm/IR/Value.h
    llvm/trunk/lib/IR/Globals.cpp
    llvm/trunk/lib/IR/Instructions.cpp
    llvm/trunk/lib/IR/User.cpp
    llvm/trunk/lib/IR/Value.cpp

Modified: llvm/trunk/include/llvm/IR/GlobalVariable.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/GlobalVariable.h?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/GlobalVariable.h (original)
+++ llvm/trunk/include/llvm/IR/GlobalVariable.h Fri Jun 12 12:48:10 2015
@@ -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

Modified: llvm/trunk/include/llvm/IR/Instructions.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Instructions.h (original)
+++ llvm/trunk/include/llvm/IR/Instructions.h Fri Jun 12 12:48:10 2015
@@ -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

Modified: llvm/trunk/include/llvm/IR/User.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/User.h?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/User.h (original)
+++ llvm/trunk/include/llvm/IR/User.h Fri Jun 12 12:48:10 2015
@@ -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());

Modified: llvm/trunk/include/llvm/IR/Value.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Value.h?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Value.h (original)
+++ llvm/trunk/include/llvm/IR/Value.h Fri Jun 12 12:48:10 2015
@@ -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;

Modified: llvm/trunk/lib/IR/Globals.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Globals.cpp?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Globals.cpp (original)
+++ llvm/trunk/lib/IR/Globals.cpp Fri Jun 12 12:48:10 2015
@@ -214,14 +214,20 @@ void GlobalVariable::replaceUsesOfWithOn
 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);
   }
 }

Modified: llvm/trunk/lib/IR/Instructions.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instructions.cpp?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Instructions.cpp (original)
+++ llvm/trunk/lib/IR/Instructions.cpp Fri Jun 12 12:48:10 2015
@@ -108,7 +108,7 @@ Value *PHINode::removeIncomingValue(unsi
 
   // 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(T
 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
   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, V
 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,
                       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, Ato
 
 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(C
 
 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 I
 //===----------------------------------------------------------------------===//
 
 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
 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, Bas
 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
 /// 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
   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 Ind
 /// 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(u
   
   // Nuke the last value.
   OL[NumOps-1].set(nullptr);
-  NumOperands = NumOps-1;
+  setNumHungOffUseOperands(NumOps-1);
 }
 
 BasicBlock *IndirectBrInst::getSuccessorV(unsigned idx) const {

Modified: llvm/trunk/lib/IR/User.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/User.cpp?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/lib/IR/User.cpp (original)
+++ llvm/trunk/lib/IR/User.cpp Fri Jun 12 12:48:10 2015
@@ -86,13 +86,14 @@ void User::growHungoffUses(unsigned NewN
 //===----------------------------------------------------------------------===//
 
 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, unsig
 
 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);

Modified: llvm/trunk/lib/IR/Value.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Value.cpp?rev=239621&r1=239620&r2=239621&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Value.cpp (original)
+++ llvm/trunk/lib/IR/Value.cpp Fri Jun 12 12:48:10 2015
@@ -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.





More information about the llvm-commits mailing list