From 43c3a4a7e76920c5646e473b72620acc7eb4ca5a Mon Sep 17 00:00:00 2001 From: Stepan Dyatkovskiy Date: Fri, 22 Jun 2012 14:53:30 +0000 Subject: [PATCH] Fixed r158979. Original message: Performance optimizations: - SwitchInst: case values stored separately from Operands List. It allows to make faster access to individual case value numbers or ranges. - Optimized IntItem, added APInt value caching. - Optimized IntegersSubsetGeneric: added optimizations for cases when subset is single number or when subset consists from single numbers only. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@158997 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Instructions.h | 134 +++++++++++++++++++------ include/llvm/Support/IntegersSubset.h | 79 +++++++++++---- lib/Transforms/Utils/CodeExtractor.cpp | 3 +- lib/VMCore/Instructions.cpp | 20 +++- 4 files changed, 180 insertions(+), 56 deletions(-) diff --git a/include/llvm/Instructions.h b/include/llvm/Instructions.h index 955522539e0..5904043cc68 100644 --- a/include/llvm/Instructions.h +++ b/include/llvm/Instructions.h @@ -2442,10 +2442,31 @@ DEFINE_TRANSPARENT_OPERAND_ACCESSORS(BranchInst, Value) class SwitchInst : public TerminatorInst { void *operator new(size_t, unsigned); // DO NOT IMPLEMENT unsigned ReservedSpace; + // Operands format: // Operand[0] = Value to switch on // Operand[1] = Default basic block destination // Operand[2n ] = Value to match // Operand[2n+1] = BasicBlock to go to on match + + // Store case values separately from operands list. We needn't User-Use + // concept here, since it is just a case value, it will always constant, + // and case value couldn't reused with another instructions/values. + // Additionally: + // It allows us to use custom type for case values that is not inherited + // from Value. Since case value is a complex type that implements + // the subset of integers, we needn't extract sub-constants within + // slow getAggregateElement method. + // For case values we will use std::list to by two reasons: + // 1. It allows to add/remove cases without whole collection reallocation. + // 2. In most of cases we needn't random access. + // Currently case values are also stored in Operands List, but it will moved + // out in future commits. + typedef std::list Subsets; + typedef Subsets::iterator SubsetsIt; + typedef Subsets::const_iterator SubsetsConstIt; + + Subsets TheSubsets; + SwitchInst(const SwitchInst &SI); void init(Value *Value, BasicBlock *Default, unsigned NumReserved); void growOperands(); @@ -2470,12 +2491,20 @@ protected: virtual SwitchInst *clone_impl() const; public: - template + // FIXME: Currently there are a lot of unclean template parameters, + // we need to make refactoring in future. + // All these parameters are used to implement both iterator and const_iterator + // without code duplication. + // SwitchInstTy may be "const SwitchInst" or "SwitchInst" + // ConstantIntTy may be "const ConstantInt" or "ConstantInt" + // SubsetsItTy may be SubsetsConstIt or SubsetsIt + // BasicBlockTy may be "const BasicBlock" or "BasicBlock" + template class CaseIteratorT; - typedef CaseIteratorT - ConstCaseIt; - + typedef CaseIteratorT ConstCaseIt; class CaseIt; // -2 @@ -2516,23 +2545,23 @@ public: /// Returns a read/write iterator that points to the first /// case in SwitchInst. CaseIt case_begin() { - return CaseIt(this, 0); + return CaseIt(this, 0, TheSubsets.begin()); } /// Returns a read-only iterator that points to the first /// case in the SwitchInst. ConstCaseIt case_begin() const { - return ConstCaseIt(this, 0); + return ConstCaseIt(this, 0, TheSubsets.begin()); } /// Returns a read/write iterator that points one past the last /// in the SwitchInst. CaseIt case_end() { - return CaseIt(this, getNumCases()); + return CaseIt(this, getNumCases(), TheSubsets.end()); } /// Returns a read-only iterator that points one past the last /// in the SwitchInst. ConstCaseIt case_end() const { - return ConstCaseIt(this, getNumCases()); + return ConstCaseIt(this, getNumCases(), TheSubsets.end()); } /// Returns an iterator that points to the default case. /// Note: this iterator allows to resolve successor only. Attempt @@ -2540,10 +2569,10 @@ public: /// Also note, that increment and decrement also causes an assertion and /// makes iterator invalid. CaseIt case_default() { - return CaseIt(this, DefaultPseudoIndex); + return CaseIt(this, DefaultPseudoIndex, TheSubsets.end()); } ConstCaseIt case_default() const { - return ConstCaseIt(this, DefaultPseudoIndex); + return ConstCaseIt(this, DefaultPseudoIndex, TheSubsets.end()); } /// findCaseValue - Search all of the case values for the specified constant. @@ -2597,7 +2626,7 @@ public: /// Note: /// This action invalidates iterators for all cases following the one removed, /// including the case_end() iterator. - void removeCase(CaseIt i); + void removeCase(CaseIt& i); unsigned getNumSuccessors() const { return getNumOperands()/2; } BasicBlock *getSuccessor(unsigned idx) const { @@ -2622,24 +2651,38 @@ public: // Case iterators definition. - template + template class CaseIteratorT { protected: SwitchInstTy *SI; - unsigned Index; - - public: - - typedef CaseIteratorT Self; + unsigned long Index; + SubsetsItTy SubsetIt; /// Initializes case iterator for given SwitchInst and for given /// case number. - CaseIteratorT(SwitchInstTy *SI, unsigned CaseNum) { + friend class SwitchInst; + CaseIteratorT(SwitchInstTy *SI, unsigned SuccessorIndex, + SubsetsItTy CaseValueIt) { this->SI = SI; - Index = CaseNum; + Index = SuccessorIndex; + this->SubsetIt = CaseValueIt; } + public: + typedef typename SubsetsItTy::reference IntegersSubsetRef; + typedef CaseIteratorT Self; + + CaseIteratorT(SwitchInstTy *SI, unsigned CaseNum) { + this->SI = SI; + Index = CaseNum; + SubsetIt = SI->TheSubsets.begin(); + std::advance(SubsetIt, CaseNum); + } + + /// Initializes case iterator for given SwitchInst and for given /// TerminatorInst's successor index. static Self fromSuccessorIndex(SwitchInstTy *SI, unsigned SuccessorIndex) { @@ -2654,19 +2697,18 @@ public: /// @Deprecated ConstantIntTy *getCaseValue() { assert(Index < SI->getNumCases() && "Index out the number of cases."); - IntegersSubset CaseRanges = - reinterpret_cast(SI->getOperand(2 + Index*2)); - IntegersSubset::Range R = CaseRanges.getItem(0); + IntegersSubsetRef CaseRanges = *SubsetIt; // FIXME: Currently we work with ConstantInt based cases. // So return CaseValue as ConstantInt. - return R.getLow().toConstantInt(); + return CaseRanges.getSingleNumber(0).toConstantInt(); } /// Resolves case value for current case. +// IntegersSubsetRef getCaseValueEx() { IntegersSubset getCaseValueEx() { assert(Index < SI->getNumCases() && "Index out the number of cases."); - return reinterpret_cast(SI->getOperand(2 + Index*2)); + return *SubsetIt; } /// Resolves successor for current case. @@ -2689,9 +2731,14 @@ public: Self operator++() { // Check index correctness after increment. - // Note: Index == getNumCases() means end(). - assert(Index+1 <= SI->getNumCases() && "Index out the number of cases."); + // Note: Index == getNumCases() means end(). + unsigned NumCases = SI->getNumCases(); + assert(Index+1 <= NumCases && "Index out the number of cases."); ++Index; + if (Index == 0) + SubsetIt = SI->TheSubsets.begin(); + else + ++SubsetIt; return *this; } Self operator++(int) { @@ -2703,9 +2750,18 @@ public: // Check index correctness after decrement. // Note: Index == getNumCases() means end(). // Also allow "-1" iterator here. That will became valid after ++. - assert((Index == 0 || Index-1 <= SI->getNumCases()) && + unsigned NumCases = SI->getNumCases(); + assert((Index == 0 || Index-1 <= NumCases) && "Index out the number of cases."); --Index; + if (Index == NumCases) { + SubsetIt = SI->TheSubsets.end(); + return *this; + } + + if (Index != -1UL) + --SubsetIt; + return *this; } Self operator--(int) { @@ -2723,14 +2779,25 @@ public: } }; - class CaseIt : public CaseIteratorT { + class CaseIt : public CaseIteratorT { + typedef CaseIteratorT + ParentTy; - typedef CaseIteratorT ParentTy; + protected: + friend class SwitchInst; + CaseIt(SwitchInst *SI, unsigned CaseNum, SubsetsIt SubsetIt) : + ParentTy(SI, CaseNum, SubsetIt) {} + + void updateCaseValueOperand(IntegersSubset& V) { + SI->setOperand(2 + Index*2, reinterpret_cast((Constant*)V)); + } public: + + CaseIt(SwitchInst *SI, unsigned CaseNum) : ParentTy(SI, CaseNum) {} CaseIt(const ParentTy& Src) : ParentTy(Src) {} - CaseIt(SwitchInst *SI, unsigned CaseNum) : ParentTy(SI, CaseNum) {} /// Sets the new value for current case. /// @Deprecated. @@ -2740,14 +2807,15 @@ public: // FIXME: Currently we work with ConstantInt based cases. // So inititalize IntItem container directly from ConstantInt. Mapping.add(IntItem::fromConstantInt(V)); - SI->setOperand(2 + Index*2, - reinterpret_cast((Constant*)Mapping.getCase())); + *SubsetIt = Mapping.getCase(); + updateCaseValueOperand(*SubsetIt); } /// Sets the new value for current case. void setValueEx(IntegersSubset& V) { assert(Index < SI->getNumCases() && "Index out the number of cases."); - SI->setOperand(2 + Index*2, reinterpret_cast((Constant*)V)); + *SubsetIt = V; + updateCaseValueOperand(*SubsetIt); } /// Sets the new successor for current case. diff --git a/include/llvm/Support/IntegersSubset.h b/include/llvm/Support/IntegersSubset.h index 2ceeea5b665..69c94a39811 100644 --- a/include/llvm/Support/IntegersSubset.h +++ b/include/llvm/Support/IntegersSubset.h @@ -40,26 +40,26 @@ namespace llvm { #define INT_ITEM_DEFINE_COMPARISON(op,func) \ bool operator op (const APInt& RHS) const { \ - return ConstantIntVal->getValue().func(RHS); \ + return getAPIntValue().func(RHS); \ } #define INT_ITEM_DEFINE_UNARY_OP(op) \ IntItem operator op () const { \ - APInt res = op(ConstantIntVal->getValue()); \ + APInt res = op(getAPIntValue()); \ Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); \ return IntItem(cast(NewVal)); \ } #define INT_ITEM_DEFINE_BINARY_OP(op) \ IntItem operator op (const APInt& RHS) const { \ - APInt res = ConstantIntVal->getValue() op RHS; \ + APInt res = getAPIntValue() op RHS; \ Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); \ return IntItem(cast(NewVal)); \ } #define INT_ITEM_DEFINE_ASSIGNMENT_BY_OP(op) \ IntItem& operator op (const APInt& RHS) {\ - APInt res = ConstantIntVal->getValue();\ + APInt res = getAPIntValue();\ res op RHS; \ Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); \ ConstantIntVal = cast(NewVal); \ @@ -68,7 +68,7 @@ namespace llvm { #define INT_ITEM_DEFINE_PREINCDEC(op) \ IntItem& operator op () { \ - APInt res = ConstantIntVal->getValue(); \ + APInt res = getAPIntValue(); \ op(res); \ Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); \ ConstantIntVal = cast(NewVal); \ @@ -77,7 +77,7 @@ namespace llvm { #define INT_ITEM_DEFINE_POSTINCDEC(op) \ IntItem& operator op (int) { \ - APInt res = ConstantIntVal->getValue();\ + APInt res = getAPIntValue();\ op(res); \ Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); \ OldConstantIntVal = ConstantIntVal; \ @@ -87,18 +87,24 @@ namespace llvm { #define INT_ITEM_DEFINE_OP_STANDARD_INT(RetTy, op, IntTy) \ RetTy operator op (IntTy RHS) const { \ - return (*this) op APInt(ConstantIntVal->getValue().getBitWidth(), RHS); \ + return (*this) op APInt(getAPIntValue().getBitWidth(), RHS); \ } class IntItem { ConstantInt *ConstantIntVal; - IntItem(const ConstantInt *V) : ConstantIntVal(const_cast(V)) {} + const APInt* APIntVal; + IntItem(const ConstantInt *V) : + ConstantIntVal(const_cast(V)), + APIntVal(&ConstantIntVal->getValue()){} + const APInt& getAPIntValue() const { + return *APIntVal; + } public: IntItem() {} operator const APInt&() const { - return (const APInt&)ConstantIntVal->getValue(); + return getAPIntValue(); } // Propagate APInt operators. @@ -137,7 +143,7 @@ public: // Special case for <<= IntItem& operator <<= (unsigned RHS) { - APInt res = ConstantIntVal->getValue(); + APInt res = getAPIntValue(); res <<= RHS; Constant *NewVal = ConstantInt::get(ConstantIntVal->getContext(), res); ConstantIntVal = cast(NewVal); @@ -278,9 +284,9 @@ public: // In short, for more compact memory consumption we can store flat // numbers collection, and define range as pair of indices. // In that case we can safe some memory on 32 bit machines. - typedef std::list FlatCollectionTy; + typedef std::vector FlatCollectionTy; typedef std::pair RangeLinkTy; - typedef SmallVector RangeLinksTy; + typedef std::vector RangeLinksTy; typedef typename RangeLinksTy::const_iterator RangeLinksConstIt; typedef IntegersSubsetGeneric self; @@ -290,21 +296,33 @@ protected: FlatCollectionTy FlatCollection; RangeLinksTy RangeLinks; + bool IsSingleNumber; + bool IsSingleNumbersOnly; + public: template explicit IntegersSubsetGeneric(const RangesCollectionTy& Links) { assert(Links.size() && "Empty ranges are not allowed."); + + // In case of big set of single numbers consumes additional RAM space, + // but allows to avoid additional reallocation. + FlatCollection.reserve(Links.size() * 2); + RangeLinks.reserve(Links.size()); + IsSingleNumbersOnly = true; for (typename RangesCollectionTy::const_iterator i = Links.begin(), e = Links.end(); i != e; ++i) { RangeLinkTy RangeLink; FlatCollection.push_back(i->getLow()); RangeLink.first = &FlatCollection.back(); - if (i->getLow() != i->getHigh()) + if (i->getLow() != i->getHigh()) { FlatCollection.push_back(i->getHigh()); + IsSingleNumbersOnly = false; + } RangeLink.second = &FlatCollection.back(); RangeLinks.push_back(RangeLink); } + IsSingleNumber = IsSingleNumbersOnly && RangeLinks.size() == 1; } IntegersSubsetGeneric(const self& RHS) { @@ -314,6 +332,8 @@ public: self& operator=(const self& RHS) { FlatCollection.clear(); RangeLinks.clear(); + FlatCollection.reserve(RHS.RangeLinks.size() * 2); + RangeLinks.reserve(RHS.RangeLinks.size()); for (RangeLinksConstIt i = RHS.RangeLinks.begin(), e = RHS.RangeLinks.end(); i != e; ++i) { RangeLinkTy RangeLink; @@ -324,6 +344,8 @@ public: RangeLink.second = &FlatCollection.back(); RangeLinks.push_back(RangeLink); } + IsSingleNumber = RHS.IsSingleNumber; + IsSingleNumbersOnly = RHS.IsSingleNumbersOnly; return *this; } @@ -333,6 +355,13 @@ public: /// true if it equals to one of contained values or belongs to the one of /// contained ranges. bool isSatisfies(const IntTy &CheckingVal) const { + if (IsSingleNumber) + return FlatCollection.front() == CheckingVal; + if (IsSingleNumbersOnly) + return std::find(FlatCollection.begin(), + FlatCollection.end(), + CheckingVal) != FlatCollection.end(); + for (unsigned i = 0, e = getNumItems(); i < e; ++i) { if (RangeLinks[i].first == RangeLinks[i].second) { if (*RangeLinks[i].first == CheckingVal) @@ -360,8 +389,12 @@ public: /// Returns true if whole subset contains single element. bool isSingleNumber() const { - return RangeLinks.size() == 1 && - RangeLinks[0].first == RangeLinks[0].second; + return IsSingleNumber; + } + + /// Returns true if whole subset contains only single numbers, no ranges. + bool isSingleNumbersOnly() const { + return IsSingleNumbersOnly; } /// Does the same like getItem(idx).isSingleNumber(), but @@ -385,7 +418,7 @@ public: } return sz.getZExtValue(); } - + /// Allows to access single value even if it belongs to some range. /// Ranges set is considered as flat numbers collection. /// [<1>, <4,8>] is considered as [1,4,5,6,7,8] @@ -409,6 +442,14 @@ public: assert(0 && "Index exceeds high border."); return sz; } + + /// Does the same as getSingleValue, but works only if subset contains + /// single numbers only. + const IntTy& getSingleNumber(unsigned idx) const { + assert(IsSingleNumbersOnly && "This method works properly if subset " + "contains single numbers only."); + return FlatCollection[idx]; + } }; //===----------------------------------------------------------------------===// @@ -455,9 +496,9 @@ class IntegersSubset : public IntegersSubsetGeneric { } public: - - IntegersSubset(Constant *C) : ParentTy(rangesFromConstant(C)), - Holder(C) {} + + explicit IntegersSubset(Constant *C) : ParentTy(rangesFromConstant(C)), + Holder(C) {} template explicit IntegersSubset(const RangesCollectionTy& Src) : ParentTy(Src) { diff --git a/lib/Transforms/Utils/CodeExtractor.cpp b/lib/Transforms/Utils/CodeExtractor.cpp index f10dbbeef2d..c545cd68c98 100644 --- a/lib/Transforms/Utils/CodeExtractor.cpp +++ b/lib/Transforms/Utils/CodeExtractor.cpp @@ -664,7 +664,8 @@ emitCallAndSwitchStatement(Function *newFunction, BasicBlock *codeReplacer, TheSwitch->setCondition(call); TheSwitch->setDefaultDest(TheSwitch->getSuccessor(NumExitBlocks)); // Remove redundant case - TheSwitch->removeCase(SwitchInst::CaseIt(TheSwitch, NumExitBlocks-1)); + SwitchInst::CaseIt ToBeRemoved(TheSwitch, NumExitBlocks-1); + TheSwitch->removeCase(ToBeRemoved); break; } } diff --git a/lib/VMCore/Instructions.cpp b/lib/VMCore/Instructions.cpp index 26cc6322da7..383994554db 100644 --- a/lib/VMCore/Instructions.cpp +++ b/lib/VMCore/Instructions.cpp @@ -3158,6 +3158,7 @@ SwitchInst::SwitchInst(const SwitchInst &SI) OL[i] = InOL[i]; OL[i+1] = InOL[i+1]; } + TheSubsets = SI.TheSubsets; SubclassOptionalData = SI.SubclassOptionalData; } @@ -3186,14 +3187,17 @@ void SwitchInst::addCase(IntegersSubset& OnVal, BasicBlock *Dest) { // Initialize some new operands. assert(OpNo+1 < ReservedSpace && "Growing didn't work!"); NumOperands = OpNo+2; - CaseIt Case(this, NewCaseIdx); - Case.setValueEx(OnVal); + + SubsetsIt TheSubsetsIt = TheSubsets.insert(TheSubsets.end(), OnVal); + + CaseIt Case(this, NewCaseIdx, TheSubsetsIt); + Case.updateCaseValueOperand(OnVal); Case.setSuccessor(Dest); } /// removeCase - This method removes the specified case and its successor /// from the switch instruction. -void SwitchInst::removeCase(CaseIt i) { +void SwitchInst::removeCase(CaseIt& i) { unsigned idx = i.getCaseIndex(); assert(2 + idx*2 < getNumOperands() && "Case index out of range!!!"); @@ -3210,6 +3214,16 @@ void SwitchInst::removeCase(CaseIt i) { // Nuke the last value. OL[NumOps-2].set(0); OL[NumOps-2+1].set(0); + + // Do the same with TheCases collection: + if (i.SubsetIt != --TheSubsets.end()) { + *i.SubsetIt = TheSubsets.back(); + TheSubsets.pop_back(); + } else { + TheSubsets.pop_back(); + i.SubsetIt = TheSubsets.end(); + } + NumOperands = NumOps-2; }