From: Pete Cooper Date: Fri, 12 Jun 2015 17:48:10 +0000 (+0000) Subject: Rename NumOperands to make it clear its managed by the User. NFC. X-Git-Tag: llvmorg-3.7.0-rc1~2476 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=b4eede2c079ef0475c023f883d48e10e15401d16;p=platform%2Fupstream%2Fllvm.git 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. llvm-svn: 239621 --- diff --git a/llvm/include/llvm/IR/GlobalVariable.h b/llvm/include/llvm/IR/GlobalVariable.h index 9f57705..126bebb 100644 --- a/llvm/include/llvm/IR/GlobalVariable.h +++ b/llvm/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/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h index 39a6265..0028376 100644 --- a/llvm/include/llvm/IR/Instructions.h +++ b/llvm/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/llvm/include/llvm/IR/User.h b/llvm/include/llvm/IR/User.h index fffa2cc..3b72cfe 100644 --- a/llvm/include/llvm/IR/User.h +++ b/llvm/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((const Value*)this) || isa((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/llvm/include/llvm/IR/Value.h b/llvm/include/llvm/IR/Value.h index 83df4cd..b175e0e 100644 --- a/llvm/include/llvm/IR/Value.h +++ b/llvm/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/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp index 1028e33..79a458c 100644 --- a/llvm/lib/IR/Globals.cpp +++ b/llvm/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/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp index dff31be..0e2fe39 100644 --- a/llvm/lib/IR/Instructions.cpp +++ b/llvm/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 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 Args, void CallInst::init(Value *Func, const Twine &NameStr) { FTy = cast(cast(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 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 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 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/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp index 0cba526..1084a50 100644 --- a/llvm/lib/IR/User.cpp +++ b/llvm/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(Storage); Use *End = Start + Us; User *Obj = reinterpret_cast(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(Usr); - Use *Storage = static_cast(Usr) - Start->NumOperands; + Use *Storage = static_cast(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/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp index dcf0ad5..eb5c225 100644 --- a/llvm/lib/IR/Value.cpp +++ b/llvm/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 before the CallInst has been // constructed.