[llvm-commits] [llvm] r159527 - in /llvm/trunk: include/llvm/Support/IntegersSubset.h include/llvm/Support/IntegersSubsetMapping.h lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp lib/Transforms/Utils/LowerSwitch.cpp lib/Transforms/Utils/Simplify
Chandler Carruth
chandlerc at google.com
Mon Jul 2 06:22:18 PDT 2012
To make matters worse, this is also causing a *slew* of new warnings:
[28/446] Building CXX object lib/VMCore/CMakeFiles/LLVMCore.dir/Metadata.cpp.o
In file included from ../lib/VMCore/Metadata.cpp:15:
In file included from ../lib/VMCore/LLVMContextImpl.h:19:
In file included from ../lib/VMCore/ConstantsContext.h:21:
In file included from ../include/llvm/Instructions.h:23:
../include/llvm/Support/IntegersSubset.h:214:7: warning: default label
in switch which covers all enumeration values
[-Wcovered-switch-default] default:
^../include/llvm/Support/IntegersSubsetMapping.h:379:12: note:
in instantiation of member function
'llvm::IntRange<llvm::IntItem>::isSingleNumber' requested here if
(!R.isSingleNumber())
^../include/llvm/Support/IntegersSubsetMapping.h:374:5: note: in
instantiation of member function
'llvm::IntegersSubsetMapping<llvm::BasicBlock, llvm::IntegersSubset,
llvm::IntItem>::add' requested here add(REx, S);
^../include/llvm/Support/IntegersSubsetMapping.h:364:5: note: in
instantiation of member function
'llvm::IntegersSubsetMapping<llvm::BasicBlock, llvm::IntegersSubset,
llvm::IntItem>::add' requested here add(R, S);
^../include/llvm/Instructions.h:2812:15: note: in instantiation of
member function 'llvm::IntegersSubsetMapping<llvm::BasicBlock,
llvm::IntegersSubset, llvm::IntItem>::add' requested here
Mapping.add(IntItem::fromConstantInt(V)); ^1 warning
generated.
On Mon, Jul 2, 2012 at 6:19 AM, Chandler Carruth <chandlerc at google.com>wrote:
> I haven't had time to review this in detail, however, I have a strong
> meta-level comment: please break these patches up before you submit them!
>
> Submitting lumped together patches like this undermines the development
> and review process of LLVM:
> - It makes it hard for reviewers to understand the motivations for
> different parts of a patch.
> - It obscures whether all aspects of the patch are well tested.
> - It makes build bot failures harder to understand
> - It forces more than is strictly necessary to come out in the event that
> a revert is needed.
> - It impedes discussion on any one point because of overlap with
> discussion on other points.
>
> Also, I'm not really sure these are "obvious" and thus valid for
> post-commit review.
>
> On Mon, Jul 2, 2012 at 6:02 AM, Stepan Dyatkovskiy <stpworld at narod.ru>wrote:
>
>> Author: dyatkovskiy
>> Date: Mon Jul 2 08:02:18 2012
>> New Revision: 159527
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=159527&view=rev
>> Log:
>> IntRange:
>> - Changed isSingleNumber method behaviour. Now this flag is calculated
>> on demand.
>>
>
> Why?
>
> IntegersSubsetMapping
>> - Optimized diff operation.
>>
>
> Under what cases? How much? Did you check in a test case to catch
> regressions?
>
>
>> - Replaced type of Items field from std::list with std::map.
>>
>
> Why? This type of datastructure change is fairly surprising. We usually
> move *away* from std::map, not toward it.
>
>
>> - Added new methods:
>> bool isOverlapped(self &RHS)
>> void add(self& RHS, SuccessorClass *S)
>> void detachCase(self& NewMapping, SuccessorClass *Succ)
>> void removeCase(SuccessorClass *Succ)
>> SuccessorClass *findSuccessor(const IntTy& Val)
>> const IntTy* getCaseSingleNumber(SuccessorClass *Succ)
>>
>
> But you didn't add any new tests for them. That's unacceptable.
>
>
>> IntegersSubsetTest
>> - DiffTest: Added checks for successors.
>> SimplifyCFG
>> Updated SwitchInst usage (now it is case-ragnes compatible) for
>> - SimplifyEqualityComparisonWithOnlyPredecessor
>> - FoldValueComparisonIntoPredecessors
>>
>>
>> Modified:
>> llvm/trunk/include/llvm/Support/IntegersSubset.h
>> llvm/trunk/include/llvm/Support/IntegersSubsetMapping.h
>> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> llvm/trunk/lib/Transforms/Utils/LowerSwitch.cpp
>> llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>> llvm/trunk/test/Transforms/SimplifyCFG/switch_create.ll
>> llvm/trunk/unittests/Support/IntegersSubsetTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/Support/IntegersSubset.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/IntegersSubset.h?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/IntegersSubset.h (original)
>> +++ llvm/trunk/include/llvm/Support/IntegersSubset.h Mon Jul 2 08:02:18
>> 2012
>> @@ -182,7 +182,12 @@
>> IntType Low;
>> IntType High;
>> bool IsEmpty : 1;
>> - bool IsSingleNumber : 1;
>> + enum Type {
>> + SINGLE_NUMBER,
>> + RANGE,
>> + UNKNOWN
>> + };
>> + Type RangeType;
>>
>> public:
>> typedef IntRange<IntType> self;
>> @@ -191,15 +196,30 @@
>> IntRange() : IsEmpty(true) {}
>> IntRange(const self &RHS) :
>> Low(RHS.Low), High(RHS.High),
>> - IsEmpty(RHS.IsEmpty), IsSingleNumber(RHS.IsSingleNumber) {}
>> + IsEmpty(RHS.IsEmpty), RangeType(RHS.RangeType) {}
>> IntRange(const IntType &C) :
>> - Low(C), High(C), IsEmpty(false), IsSingleNumber(true) {}
>> + Low(C), High(C), IsEmpty(false), RangeType(SINGLE_NUMBER) {}
>>
>> IntRange(const IntType &L, const IntType &H) : Low(L), High(H),
>> - IsEmpty(false), IsSingleNumber(Low == High) {}
>> + IsEmpty(false), RangeType(UNKNOWN) {}
>>
>> bool isEmpty() const { return IsEmpty; }
>> - bool isSingleNumber() const { return IsSingleNumber; }
>> + bool isSingleNumber() const {
>> + switch (RangeType) {
>> + case SINGLE_NUMBER:
>> + return true;
>> + case RANGE:
>> + return false;
>> + case UNKNOWN:
>> + default:
>> + if (Low == High) {
>> + const_cast<Type&>(RangeType) = SINGLE_NUMBER;
>> + return true;
>> + }
>> + const_cast<Type&>(RangeType) = RANGE;
>> + return false;
>> + }
>> + }
>>
>> const IntType& getLow() const {
>> assert(!IsEmpty && "Range is empty.");
>> @@ -213,13 +233,13 @@
>> bool operator<(const self &RHS) const {
>> assert(!IsEmpty && "Left range is empty.");
>> assert(!RHS.IsEmpty && "Right range is empty.");
>> + if (Low < RHS.Low)
>> + return true;
>> if (Low == RHS.Low) {
>> if (High > RHS.High)
>> return true;
>> return false;
>> }
>> - if (Low < RHS.Low)
>> - return true;
>> return false;
>> }
>>
>> @@ -512,7 +532,7 @@
>> e = Src.end(); i != e; ++i) {
>> const Range &R = *i;
>> std::vector<Constant*> r;
>> - if (R.isSingleNumber()) {
>> + if (!R.isSingleNumber()) {
>> r.reserve(2);
>> // FIXME: Since currently we have ConstantInt based numbers
>> // use hack-conversion of IntItem to ConstantInt
>>
>> Modified: llvm/trunk/include/llvm/Support/IntegersSubsetMapping.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/IntegersSubsetMapping.h?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/IntegersSubsetMapping.h (original)
>> +++ llvm/trunk/include/llvm/Support/IntegersSubsetMapping.h Mon Jul 2
>> 08:02:18 2012
>> @@ -58,22 +58,16 @@
>>
>> protected:
>>
>> - typedef std::list<Cluster> CaseItems;
>> + typedef std::map<RangeEx, SuccessorClass*> CaseItems;
>> typedef typename CaseItems::iterator CaseItemIt;
>> typedef typename CaseItems::const_iterator CaseItemConstIt;
>>
>> // TODO: Change unclean CRS prefixes to SubsetMap for example.
>> typedef std::map<SuccessorClass*, RangesCollection > CRSMap;
>> typedef typename CRSMap::iterator CRSMapIt;
>> -
>> - struct ClustersCmp {
>> - bool operator()(const Cluster &C1, const Cluster &C2) {
>> - return C1.first < C2.first;
>> - }
>> - };
>>
>> CaseItems Items;
>> - bool Sorted;
>> + bool SingleNumbersOnly;
>>
>> bool isIntersected(CaseItemIt& LItem, CaseItemIt& RItem) {
>> return LItem->first.getHigh() >= RItem->first.getLow();
>> @@ -91,18 +85,6 @@
>> return LItem->first.getHigh() >= RLow;
>> }
>>
>> - void sort() {
>> - if (!Sorted) {
>> - std::vector<Cluster> clustersVector;
>> - clustersVector.reserve(Items.size());
>> - clustersVector.insert(clustersVector.begin(), Items.begin(),
>> Items.end());
>> - std::sort(clustersVector.begin(), clustersVector.end(),
>> ClustersCmp());
>> - Items.clear();
>> - Items.insert(Items.begin(), clustersVector.begin(),
>> clustersVector.end());
>> - Sorted = true;
>> - }
>> - }
>> -
>> enum DiffProcessState {
>> L_OPENED,
>> INTERSECT_OPENED,
>> @@ -213,7 +195,7 @@
>> }
>> }
>>
>> - void onRLOpen(const IntTy &Pt,
>> + void onLROpen(const IntTy &Pt,
>> SuccessorClass *LS,
>> SuccessorClass *RS) {
>> switch (State) {
>> @@ -229,7 +211,7 @@
>> OpenPt = Pt;
>> }
>>
>> - void onRLClose(const IntTy &Pt) {
>> + void onLRClose(const IntTy &Pt) {
>> switch (State) {
>> case INTERSECT_OPENED:
>> if (IntersectionMapping)
>> @@ -245,6 +227,48 @@
>> bool isLOpened() { return State == L_OPENED; }
>> bool isROpened() { return State == R_OPENED; }
>> };
>> +
>> + void diff_single_numbers(self *LExclude, self *Intersection, self
>> *RExclude,
>> + const self& RHS) {
>> +
>> + CaseItemConstIt L = Items.begin(), R = RHS.Items.begin();
>> + CaseItemConstIt el = Items.end(), er = RHS.Items.end();
>> + while (L != el && R != er) {
>> + const Cluster &LCluster = *L;
>> + const RangeEx &LRange = LCluster.first;
>> + const Cluster &RCluster = *R;
>> + const RangeEx &RRange = RCluster.first;
>> +
>> + if (LRange.getLow() < RRange.getLow()) {
>> + if (LExclude)
>> + LExclude->add(LRange.getLow(), LCluster.second);
>> + ++L;
>> + } else if (LRange.getLow() > RRange.getLow()) {
>> + if (RExclude)
>> + RExclude->add(RRange.getLow(), RCluster.second);
>> + ++R;
>> + } else {
>> + if (Intersection)
>> + Intersection->add(LRange.getLow(), LCluster.second);
>> + ++L;
>> + ++R;
>> + }
>> + }
>> +
>> + if (L != Items.end()) {
>> + if (LExclude)
>> + do {
>> + LExclude->add(L->first, L->second);
>> + ++L;
>> + } while (L != Items.end());
>> + } else if (R != RHS.Items.end()) {
>> + if (RExclude)
>> + do {
>> + RExclude->add(R->first, R->second);
>> + ++R;
>> + } while (R != RHS.Items.end());
>> + }
>> + }
>>
>> public:
>>
>> @@ -256,15 +280,18 @@
>>
>> typedef std::pair<SuccessorClass*, IntegersSubsetTy> Case;
>> typedef std::list<Case> Cases;
>> + typedef typename Cases::iterator CasesIt;
>> +
>> + IntegersSubsetMapping() : SingleNumbersOnly(true) {}
>>
>> - IntegersSubsetMapping() {
>> - Sorted = false;
>> + bool verify() {
>> + RangeIterator DummyErrItem;
>> + return verify(DummyErrItem);
>> }
>>
>> bool verify(RangeIterator& errItem) {
>> if (Items.empty())
>> return true;
>> - sort();
>> for (CaseItemIt j = Items.begin(), i = j++, e = Items.end();
>> j != e; i = j++) {
>> if (isIntersected(i, j) && i->second != j->second) {
>> @@ -275,10 +302,36 @@
>> return true;
>> }
>>
>> + bool isOverlapped(self &RHS) {
>> + if (Items.empty() || RHS.empty())
>> + return true;
>> +
>> + for (CaseItemIt L = Items.begin(), R = RHS.Items.begin(),
>> + el = Items.end(), er = RHS.Items.end(); L != el && R != er;) {
>> +
>> + const RangeTy &LRange = L->first;
>> + const RangeTy &RRange = R->first;
>> +
>> + if (LRange.getLow() > RRange.getLow()) {
>> + if (RRange.isSingleNumber() || LRange.getLow() >
>> RRange.getHigh())
>> + ++R;
>> + else
>> + return true;
>> + } else if (LRange.getLow() < RRange.getLow()) {
>> + if (LRange.isSingleNumber() || LRange.getHigh() <
>> RRange.getLow())
>> + ++L;
>> + else
>> + return true;
>> + } else // iRange.getLow() == jRange.getLow()
>> + return true;
>> + }
>> + return false;
>> + }
>> +
>> +
>> void optimize() {
>> if (Items.size() < 2)
>> return;
>> - sort();
>> CaseItems OldItems = Items;
>> Items.clear();
>> const IntTy *Low = &OldItems.begin()->first.getLow();
>> @@ -303,8 +356,6 @@
>> }
>> RangeEx R(*Low, *High, Weight);
>> add(R, Successor);
>> - // We recollected the Items, but we kept it sorted.
>> - Sorted = true;
>> }
>>
>> /// Adds a constant value.
>> @@ -323,8 +374,10 @@
>> add(REx, S);
>> }
>> void add(const RangeEx &R, SuccessorClass *S = 0) {
>> - Items.push_back(std::make_pair(R, S));
>> - Sorted = false;
>> + //Items.push_back(std::make_pair(R, S));
>> + Items.insert(std::make_pair(R, S));
>> + if (!R.isSingleNumber())
>> + SingleNumbersOnly = false;
>> }
>>
>> /// Adds all ranges and values from given ranges set to the current
>> @@ -337,9 +390,17 @@
>> }
>>
>> void add(self& RHS) {
>> - Items.insert(Items.end(), RHS.Items.begin(), RHS.Items.end());
>> + //Items.insert(Items.begin(), RHS.Items.begin(), RHS.Items.end());
>> + Items.insert(RHS.Items.begin(), RHS.Items.end());
>> + if (!RHS.SingleNumbersOnly)
>> + SingleNumbersOnly = false;
>> }
>>
>> + void add(self& RHS, SuccessorClass *S) {
>> + for (CaseItemIt i = RHS.Items.begin(), e = RHS.Items.end(); i != e;
>> ++i)
>> + add(i->first, S);
>> + }
>> +
>> void add(const RangesCollection& RHS, SuccessorClass *S = 0) {
>> for (RangesCollectionConstIt i = RHS.begin(), e = RHS.end(); i != e;
>> ++i)
>> add(*i, S);
>> @@ -348,6 +409,34 @@
>> /// Removes items from set.
>> void removeItem(RangeIterator i) { Items.erase(i); }
>>
>> + /// Moves whole case from current mapping to the NewMapping object.
>> + void detachCase(self& NewMapping, SuccessorClass *Succ) {
>> + for (CaseItemIt i = Items.begin(); i != Items.end();)
>> + if (i->second == Succ) {
>> + NewMapping.add(i->first, i->second);
>> + Items.erase(i++);
>> + } else
>> + ++i;
>> + }
>> +
>> + /// Removes all clusters for given successor.
>> + void removeCase(SuccessorClass *Succ) {
>> + for (CaseItemIt i = Items.begin(); i != Items.end();)
>> + if (i->second == Succ) {
>> + Items.erase(i++);
>> + } else
>> + ++i;
>> + }
>> +
>> + /// Find successor that satisfies given value.
>> + SuccessorClass *findSuccessor(const IntTy& Val) {
>> + for (CaseItemIt i = Items.begin(); i != Items.end(); ++i) {
>> + if (i->first.isInRange(Val))
>> + return i->second;
>> + }
>> + return 0;
>> + }
>> +
>> /// Calculates the difference between this mapping and RHS.
>> /// THIS without RHS is placed into LExclude,
>> /// RHS without THIS is placed into RExclude,
>> @@ -355,10 +444,16 @@
>> void diff(self *LExclude, self *Intersection, self *RExclude,
>> const self& RHS) {
>>
>> + if (SingleNumbersOnly && RHS.SingleNumbersOnly) {
>> + diff_single_numbers(LExclude, Intersection, RExclude, RHS);
>> + return;
>> + }
>> +
>> DiffStateMachine Machine(LExclude, Intersection, RExclude);
>>
>> CaseItemConstIt L = Items.begin(), R = RHS.Items.begin();
>> - while (L != Items.end() && R != RHS.Items.end()) {
>> + CaseItemConstIt el = Items.end(), er = RHS.Items.end();
>> + while (L != el && R != er) {
>> const Cluster &LCluster = *L;
>> const RangeEx &LRange = LCluster.first;
>> const Cluster &RCluster = *R;
>> @@ -377,7 +472,36 @@
>> ++R;
>> continue;
>> }
>> +
>> + if (LRange.isSingleNumber() && RRange.isSingleNumber()) {
>> + Machine.onLROpen(LRange.getLow(), LCluster.second,
>> RCluster.second);
>> + Machine.onLRClose(LRange.getLow());
>> + ++L;
>> + ++R;
>> + continue;
>> + }
>>
>> + if (LRange.isSingleNumber()) {
>> + Machine.onLOpen(LRange.getLow(), LCluster.second);
>> + Machine.onLClose(LRange.getLow());
>> + ++L;
>> + while(L != Items.end() && L->first.getHigh() < RRange.getHigh())
>> {
>> + Machine.onLOpen(LRange.getLow(), LCluster.second);
>> + Machine.onLClose(LRange.getLow());
>> + ++L;
>> + }
>> + continue;
>> + } else if (RRange.isSingleNumber()) {
>> + Machine.onROpen(R->first.getLow(), R->second);
>> + Machine.onRClose(R->first.getHigh());
>> + ++R;
>> + while(R != RHS.Items.end() && R->first.getHigh() <
>> LRange.getHigh()) {
>> + Machine.onROpen(R->first.getLow(), R->second);
>> + Machine.onRClose(R->first.getHigh());
>> + ++R;
>> + }
>> + continue;
>> + } else
>> if (LRange.getLow() < RRange.getLow()) {
>> // May be opened in previous iteration.
>> if (!Machine.isLOpened())
>> @@ -390,7 +514,7 @@
>> Machine.onLOpen(LRange.getLow(), LCluster.second);
>> }
>> else
>> - Machine.onRLOpen(LRange.getLow(), LCluster.second,
>> RCluster.second);
>> + Machine.onLROpen(LRange.getLow(), LCluster.second,
>> RCluster.second);
>>
>> if (LRange.getHigh() < RRange.getHigh()) {
>> Machine.onLClose(LRange.getHigh());
>> @@ -411,7 +535,7 @@
>> }
>> }
>> else {
>> - Machine.onRLClose(LRange.getHigh());
>> + Machine.onLRClose(LRange.getHigh());
>> ++L;
>> ++R;
>> }
>> @@ -441,7 +565,20 @@
>> }
>>
>> /// Builds the finalized case objects.
>> - void getCases(Cases& TheCases) {
>> + void getCases(Cases& TheCases, bool PreventMerging = false) {
>> + //FIXME: PreventMerging is a temporary parameter.
>> + //Currently a set of passes is still knows nothing about
>> + //switches with case ranges, and if these passes meet switch
>> + //with complex case that crashs the application.
>> + if (PreventMerging) {
>> + for (RangeIterator i = this->begin(); i != this->end(); ++i) {
>> + RangesCollection SingleRange;
>> + SingleRange.push_back(i->first);
>> + TheCases.push_back(std::make_pair(i->second,
>> +
>> IntegersSubsetTy(SingleRange)));
>> + }
>> + return;
>> + }
>> CRSMap TheCRSMap;
>> for (RangeIterator i = this->begin(); i != this->end(); ++i)
>> TheCRSMap[i->second].push_back(i->first);
>> @@ -458,6 +595,22 @@
>> return IntegersSubsetTy(Ranges);
>> }
>>
>> + /// Returns pointer to value of case if it is single-numbered or 0
>> + /// in another case.
>> + const IntTy* getCaseSingleNumber(SuccessorClass *Succ) {
>> + const IntTy* Res = 0;
>> + for (CaseItemIt i = Items.begin(); i != Items.end(); ++i)
>> + if (i->second == Succ) {
>> + if (!i->first.isSingleNumber())
>> + return 0;
>> + if (Res)
>> + return 0;
>> + else
>> + Res = &(i->first.getLow());
>> + }
>> + return Res;
>> + }
>> +
>> /// Returns true if there is no ranges and values inside.
>> bool empty() const { return Items.empty(); }
>>
>>
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Mon Jul
>> 2 08:02:18 2012
>> @@ -2450,22 +2450,23 @@
>> size_t numCmps = 0;
>> for (Clusterifier::RangeIterator i = TheClusterifier.begin(),
>> e = TheClusterifier.end(); i != e; ++i, ++numCmps) {
>> - Clusterifier::Cluster &C = *i;
>> + const Clusterifier::RangeEx &R = i->first;
>> + MachineBasicBlock *MBB = i->second;
>> unsigned W = 0;
>> if (BPI) {
>> - W = BPI->getEdgeWeight(SI.getParent(), C.second->getBasicBlock());
>> + W = BPI->getEdgeWeight(SI.getParent(), MBB->getBasicBlock());
>> if (!W)
>> W = 16;
>> - W *= C.first.Weight;
>> - BPI->setEdgeWeight(SI.getParent(), C.second->getBasicBlock(), W);
>> + W *= R.Weight;
>> + BPI->setEdgeWeight(SI.getParent(), MBB->getBasicBlock(), W);
>> }
>>
>> // FIXME: Currently work with ConstantInt based numbers.
>> // Changing it to APInt based is a pretty heavy for this commit.
>> - Cases.push_back(Case(C.first.getLow().toConstantInt(),
>> - C.first.getHigh().toConstantInt(), C.second,
>> W));
>> + Cases.push_back(Case(R.getLow().toConstantInt(),
>> + R.getHigh().toConstantInt(), MBB, W));
>>
>> - if (C.first.getLow() != C.first.getHigh())
>> + if (R.getLow() != R.getHigh())
>> // A range counts double, since it requires two compares.
>> ++numCmps;
>> }
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/LowerSwitch.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/LowerSwitch.cpp?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/LowerSwitch.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/LowerSwitch.cpp Mon Jul 2 08:02:18
>> 2012
>> @@ -238,13 +238,14 @@
>> size_t numCmps = 0;
>> for (IntegersSubsetToBB::RangeIterator i = TheClusterifier.begin(),
>> e = TheClusterifier.end(); i != e; ++i, ++numCmps) {
>> - IntegersSubsetToBB::Cluster &C = *i;
>> + const IntegersSubsetToBB::RangeTy &R = i->first;
>> + BasicBlock *S = i->second;
>>
>> // FIXME: Currently work with ConstantInt based numbers.
>> // Changing it to APInt based is a pretty heavy for this commit.
>> - Cases.push_back(CaseRange(C.first.getLow().toConstantInt(),
>> - C.first.getHigh().toConstantInt(),
>> C.second));
>> - if (C.first.isSingleNumber())
>> + Cases.push_back(CaseRange(R.getLow().toConstantInt(),
>> + R.getHigh().toConstantInt(), S));
>> + if (R.isSingleNumber())
>> // A range counts double, since it requires two compares.
>> ++numCmps;
>> }
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Mon Jul 2 08:02:18
>> 2012
>> @@ -56,26 +56,12 @@
>> STATISTIC(NumSpeculations, "Number of speculative executed
>> instructions");
>>
>> namespace {
>> - /// ValueEqualityComparisonCase - Represents a case of a switch.
>> - struct ValueEqualityComparisonCase {
>> - ConstantInt *Value;
>> - BasicBlock *Dest;
>> -
>> - ValueEqualityComparisonCase(ConstantInt *Value, BasicBlock *Dest)
>> - : Value(Value), Dest(Dest) {}
>> -
>> - bool operator<(ValueEqualityComparisonCase RHS) const {
>> - // Comparing pointers is ok as we only rely on the order for
>> uniquing.
>> - return Value < RHS.Value;
>> - }
>> - };
>> -
>> class SimplifyCFGOpt {
>> const TargetData *const TD;
>>
>> Value *isValueEqualityComparison(TerminatorInst *TI);
>> BasicBlock *GetValueEqualityComparisonCases(TerminatorInst *TI,
>> - std::vector<ValueEqualityComparisonCase>
>> &Cases);
>> + IntegersSubsetToBB &Mapping);
>> bool SimplifyEqualityComparisonWithOnlyPredecessor(TerminatorInst *TI,
>> BasicBlock *Pred,
>> IRBuilder<>
>> &Builder);
>> @@ -532,72 +518,25 @@
>> /// decode all of the 'cases' that it represents and return the
>> 'default' block.
>> BasicBlock *SimplifyCFGOpt::
>> GetValueEqualityComparisonCases(TerminatorInst *TI,
>> - std::vector<ValueEqualityComparisonCase>
>> -
>> &Cases) {
>> + IntegersSubsetToBB &Mapping) {
>> if (SwitchInst *SI = dyn_cast<SwitchInst>(TI)) {
>> - Cases.reserve(SI->getNumCases());
>> - for (SwitchInst::CaseIt i = SI->case_begin(), e = SI->case_end(); i
>> != e; ++i)
>> - Cases.push_back(ValueEqualityComparisonCase(i.getCaseValue(),
>> - i.getCaseSuccessor()));
>> + for (SwitchInst::CaseIt i = SI->case_begin(), e = SI->case_end();
>> + i != e; ++i)
>> + Mapping.add(i.getCaseValueEx(), i.getCaseSuccessor());
>> return SI->getDefaultDest();
>> }
>> -
>> +
>> BranchInst *BI = cast<BranchInst>(TI);
>> ICmpInst *ICI = cast<ICmpInst>(BI->getCondition());
>> - BasicBlock *Succ = BI->getSuccessor(ICI->getPredicate() ==
>> ICmpInst::ICMP_NE);
>> -
>> Cases.push_back(ValueEqualityComparisonCase(GetConstantInt(ICI->getOperand(1),
>> - TD),
>> - Succ));
>> + IntegersSubsetToBB Builder;
>> +
>> + Mapping.add(
>> + IntItem::fromConstantInt(GetConstantInt(ICI->getOperand(1), TD)),
>> + BI->getSuccessor(ICI->getPredicate() == ICmpInst::ICMP_NE));
>> +
>> return BI->getSuccessor(ICI->getPredicate() == ICmpInst::ICMP_EQ);
>> }
>>
>> -
>> -/// EliminateBlockCases - Given a vector of bb/value pairs, remove any
>> entries
>> -/// in the list that match the specified block.
>> -static void EliminateBlockCases(BasicBlock *BB,
>> - std::vector<ValueEqualityComparisonCase>
>> &Cases) {
>> - for (unsigned i = 0, e = Cases.size(); i != e; ++i)
>> - if (Cases[i].Dest == BB) {
>> - Cases.erase(Cases.begin()+i);
>> - --i; --e;
>> - }
>> -}
>> -
>> -/// ValuesOverlap - Return true if there are any keys in C1 that exist
>> in C2 as
>> -/// well.
>> -static bool
>> -ValuesOverlap(std::vector<ValueEqualityComparisonCase> &C1,
>> - std::vector<ValueEqualityComparisonCase > &C2) {
>> - std::vector<ValueEqualityComparisonCase> *V1 = &C1, *V2 = &C2;
>> -
>> - // Make V1 be smaller than V2.
>> - if (V1->size() > V2->size())
>> - std::swap(V1, V2);
>> -
>> - if (V1->size() == 0) return false;
>> - if (V1->size() == 1) {
>> - // Just scan V2.
>> - ConstantInt *TheVal = (*V1)[0].Value;
>> - for (unsigned i = 0, e = V2->size(); i != e; ++i)
>> - if (TheVal == (*V2)[i].Value)
>> - return true;
>> - }
>> -
>> - // Otherwise, just sort both lists and compare element by element.
>> - array_pod_sort(V1->begin(), V1->end());
>> - array_pod_sort(V2->begin(), V2->end());
>> - unsigned i1 = 0, i2 = 0, e1 = V1->size(), e2 = V2->size();
>> - while (i1 != e1 && i2 != e2) {
>> - if ((*V1)[i1].Value == (*V2)[i2].Value)
>> - return true;
>> - if ((*V1)[i1].Value < (*V2)[i2].Value)
>> - ++i1;
>> - else
>> - ++i2;
>> - }
>> - return false;
>> -}
>> -
>> /// SimplifyEqualityComparisonWithOnlyPredecessor - If TI is known to be
>> a
>> /// terminator instruction and its block is known to only have a single
>> /// predecessor block, check to see if that predecessor is also a value
>> @@ -616,23 +555,27 @@
>> if (ThisVal != PredVal) return false; // Different predicates.
>>
>> // Find out information about when control will move from Pred to TI's
>> block.
>> - std::vector<ValueEqualityComparisonCase> PredCases;
>> + IntegersSubsetToBB PredCases;
>> BasicBlock *PredDef =
>> GetValueEqualityComparisonCases(Pred->getTerminator(),
>> PredCases);
>> - EliminateBlockCases(PredDef, PredCases); // Remove default from cases.
>>
>> + // Remove default from cases.
>> + PredCases.removeCase(PredDef);
>> +
>> // Find information about how control leaves this block.
>> - std::vector<ValueEqualityComparisonCase> ThisCases;
>> + IntegersSubsetToBB ThisCases;
>> BasicBlock *ThisDef = GetValueEqualityComparisonCases(TI, ThisCases);
>> - EliminateBlockCases(ThisDef, ThisCases); // Remove default from cases.
>>
>> + // Remove default from cases.
>> + ThisCases.removeCase(ThisDef);
>> +
>> // If TI's block is the default block from Pred's comparison,
>> potentially
>> // simplify TI based on this knowledge.
>> if (PredDef == TI->getParent()) {
>> // If we are here, we know that the value is none of those cases
>> listed in
>> // PredCases. If there are any cases in ThisCases that are in
>> PredCases, we
>> // can simplify TI.
>> - if (!ValuesOverlap(PredCases, ThisCases))
>> + if (!PredCases.isOverlapped(ThisCases))
>> return false;
>>
>> if (isa<BranchInst>(TI)) {
>> @@ -644,7 +587,7 @@
>> (void) NI;
>>
>> // Remove PHI node entries for the dead edge.
>> - ThisCases[0].Dest->removePredecessor(TI->getParent());
>> + ThisCases.begin()->second->removePredecessor(TI->getParent());
>>
>> DEBUG(dbgs() << "Threading pred instr: " << *Pred->getTerminator()
>> << "Through successor TI: " << *TI << "Leaving: " << *NI <<
>> "\n");
>> @@ -655,45 +598,45 @@
>>
>> SwitchInst *SI = cast<SwitchInst>(TI);
>> // Okay, TI has cases that are statically dead, prune them away.
>> - SmallPtrSet<Constant*, 16> DeadCases;
>> - for (unsigned i = 0, e = PredCases.size(); i != e; ++i)
>> - DeadCases.insert(PredCases[i].Value);
>> + IRBuilder<> CodeBuilder(SI);
>> + CodeBuilder.SetCurrentDebugLocation(SI->getDebugLoc());
>>
>> DEBUG(dbgs() << "Threading pred instr: " << *Pred->getTerminator()
>> << "Through successor TI: " << *TI);
>>
>> - for (SwitchInst::CaseIt i = SI->case_end(), e = SI->case_begin(); i
>> != e;) {
>> - --i;
>> - if (DeadCases.count(i.getCaseValue())) {
>> - i.getCaseSuccessor()->removePredecessor(TI->getParent());
>> - SI->removeCase(i);
>> - }
>> - }
>> -
>> - DEBUG(dbgs() << "Leaving: " << *TI << "\n");
>> + // Okay, TI has cases that are statically dead, prune them away.
>> + IntegersSubsetToBB NewThisCases;
>> + IntegersSubsetToBB WastedCases;
>> + ThisCases.diff(&NewThisCases, &WastedCases, 0, PredCases);
>> +
>> + IntegersSubsetToBB::Cases Cases;
>> + NewThisCases.getCases(Cases, true/*temporary prevent complex
>> cases*/);
>> +
>> + SwitchInst *NewSI =
>> + CodeBuilder.CreateSwitch(ThisVal, SI->getDefaultDest(),
>> Cases.size());
>> +
>> + for (IntegersSubsetToBB::RangeIterator i = WastedCases.begin(),
>> + e = WastedCases.end(); i !=
>> e; ++i)
>> + i->second->removePredecessor(TI->getParent());
>> +
>> + for (IntegersSubsetToBB::CasesIt i = Cases.begin(), e = Cases.end();
>> + i != e; ++i)
>> + NewSI->addCase(i->second, i->first);
>> +
>> + EraseTerminatorInstAndDCECond(SI);
>> + DEBUG(dbgs() << "Leaving: " << *NewSI << "\n");
>> return true;
>> }
>>
>> // Otherwise, TI's block must correspond to some matched value. Find
>> out
>> // which value (or set of values) this is.
>> - ConstantInt *TIV = 0;
>> BasicBlock *TIBB = TI->getParent();
>> - for (unsigned i = 0, e = PredCases.size(); i != e; ++i)
>> - if (PredCases[i].Dest == TIBB) {
>> - if (TIV != 0)
>> - return false; // Cannot handle multiple values coming to this
>> block.
>> - TIV = PredCases[i].Value;
>> - }
>> - assert(TIV && "No edge from pred to succ?");
>> + const IntItem* TIVIntITem = PredCases.getCaseSingleNumber(TIBB);
>> + assert(TIVIntITem && "No edge from pred to succ?");
>>
>> // Okay, we found the one constant that our value can be if we get
>> into TI's
>> // BB. Find out which successor will unconditionally be branched to.
>> - BasicBlock *TheRealDest = 0;
>> - for (unsigned i = 0, e = ThisCases.size(); i != e; ++i)
>> - if (ThisCases[i].Value == TIV) {
>> - TheRealDest = ThisCases[i].Dest;
>> - break;
>> - }
>> + BasicBlock *TheRealDest = ThisCases.findSuccessor(*TIVIntITem);
>>
>> // If not handled by any explicit cases, it is handled by the default
>> case.
>> if (TheRealDest == 0) TheRealDest = ThisDef;
>> @@ -759,10 +702,10 @@
>>
>> if (PCV == CV && SafeToMergeTerminators(TI, PTI)) {
>> // Figure out which 'cases' to copy from SI to PSI.
>> - std::vector<ValueEqualityComparisonCase> BBCases;
>> + IntegersSubsetToBB BBCases;
>> BasicBlock *BBDefault = GetValueEqualityComparisonCases(TI,
>> BBCases);
>>
>> - std::vector<ValueEqualityComparisonCase> PredCases;
>> + IntegersSubsetToBB PredCases;
>> BasicBlock *PredDefault = GetValueEqualityComparisonCases(PTI,
>> PredCases);
>>
>> // Based on whether the default edge from PTI goes to BB or not,
>> fill in
>> @@ -773,61 +716,51 @@
>> if (PredDefault == BB) {
>> // If this is the default destination from PTI, only the edges
>> in TI
>> // that don't occur in PTI, or that branch to BB will be
>> activated.
>> - std::set<ConstantInt*, ConstantIntOrdering> PTIHandled;
>> - for (unsigned i = 0, e = PredCases.size(); i != e; ++i)
>> - if (PredCases[i].Dest != BB)
>> - PTIHandled.insert(PredCases[i].Value);
>> - else {
>> - // The default destination is BB, we don't need explicit
>> targets.
>> - std::swap(PredCases[i], PredCases.back());
>> - PredCases.pop_back();
>> - --i; --e;
>> - }
>> +
>> + PredCases.removeCase(BB);
>> + IntegersSubsetToBB NewBBCases;
>> + BBCases.diff(&NewBBCases, 0, 0, PredCases);
>> + PredCases.add(NewBBCases);
>> + for (IntegersSubsetToBB::RangeIterator i = NewBBCases.begin(),
>> + e = NewBBCases.end(); i
>> != e; ++i)
>> + NewSuccessors.push_back(i->second);
>>
>> - // Reconstruct the new switch statement we will be building.
>> + // Replace the default if needed.
>> if (PredDefault != BBDefault) {
>> PredDefault->removePredecessor(Pred);
>> PredDefault = BBDefault;
>> NewSuccessors.push_back(BBDefault);
>> }
>> - for (unsigned i = 0, e = BBCases.size(); i != e; ++i)
>> - if (!PTIHandled.count(BBCases[i].Value) &&
>> - BBCases[i].Dest != BBDefault) {
>> - PredCases.push_back(BBCases[i]);
>> - NewSuccessors.push_back(BBCases[i].Dest);
>> - }
>>
>> } else {
>> // If this is not the default destination from PSI, only the
>> edges
>> // in SI that occur in PSI with a destination of BB will be
>> // activated.
>> - std::set<ConstantInt*, ConstantIntOrdering> PTIHandled;
>> - for (unsigned i = 0, e = PredCases.size(); i != e; ++i)
>> - if (PredCases[i].Dest == BB) {
>> - PTIHandled.insert(PredCases[i].Value);
>> - std::swap(PredCases[i], PredCases.back());
>> - PredCases.pop_back();
>> - --i; --e;
>> - }
>> + IntegersSubsetToBB BBPredCase;
>> + PredCases.detachCase(BBPredCase, BB);
>> + IntegersSubsetToBB CasesWithBBDef;
>> + IntegersSubsetToBB InharitedCases;
>>
>> // Okay, now we know which constants were sent to BB from the
>> // predecessor. Figure out where they will all go now.
>> - for (unsigned i = 0, e = BBCases.size(); i != e; ++i)
>> - if (PTIHandled.count(BBCases[i].Value)) {
>> - // If this is one we are capable of getting...
>> - PredCases.push_back(BBCases[i]);
>> - NewSuccessors.push_back(BBCases[i].Dest);
>> - PTIHandled.erase(BBCases[i].Value);// This constant is taken
>> care of
>> - }
>> -
>> + if (!BBPredCase.empty()) {
>> + BBCases.diff(
>> + 0, // LHS excl RHS
>> + &InharitedCases, // intersection
>> + &CasesWithBBDef, // RHS excl LHS
>> + BBPredCase); // RHS.
>> + }
>> + PredCases.add(InharitedCases);
>> + for (IntegersSubsetToBB::RangeIterator i =
>> InharitedCases.begin(),
>> + e = InharitedCases.end();
>> + i != e; ++i)
>> + NewSuccessors.push_back(i->second);
>> +
>> // If there are any constants vectored to BB that TI doesn't
>> handle,
>> // they must go to the default destination of TI.
>> - for (std::set<ConstantInt*, ConstantIntOrdering>::iterator I =
>> - PTIHandled.begin(),
>> - E = PTIHandled.end(); I != E; ++I) {
>> - PredCases.push_back(ValueEqualityComparisonCase(*I,
>> BBDefault));
>> + PredCases.add(CasesWithBBDef, BBDefault);
>> + for (unsigned i = 0, e = CasesWithBBDef.size(); i != e; ++i)
>> NewSuccessors.push_back(BBDefault);
>> - }
>> }
>>
>> // Okay, at this point, we know which new successor Pred will get.
>> Make
>> @@ -848,8 +781,12 @@
>> SwitchInst *NewSI = Builder.CreateSwitch(CV, PredDefault,
>> PredCases.size());
>> NewSI->setDebugLoc(PTI->getDebugLoc());
>> - for (unsigned i = 0, e = PredCases.size(); i != e; ++i)
>> - NewSI->addCase(PredCases[i].Value, PredCases[i].Dest);
>> + IntegersSubsetToBB::Cases Cases;
>> + PredCases.getCases(Cases, true/*temporary prevent complex case*/);
>> +
>> + for (IntegersSubsetToBB::CasesIt i = Cases.begin(), e =
>> Cases.end();
>> + i != e; ++i)
>> + NewSI->addCase(i->second, i->first);
>>
>> EraseTerminatorInstAndDCECond(PTI);
>>
>>
>> Modified: llvm/trunk/test/Transforms/SimplifyCFG/switch_create.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/switch_create.ll?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/SimplifyCFG/switch_create.ll (original)
>> +++ llvm/trunk/test/Transforms/SimplifyCFG/switch_create.ll Mon Jul 2
>> 08:02:18 2012
>> @@ -82,8 +82,8 @@
>>
>> ; CHECK: @test4
>> ; CHECK: switch i8 %c, label %lor.rhs [
>> -; CHECK: i8 62, label %lor.end
>> ; CHECK: i8 34, label %lor.end
>> +; CHECK: i8 62, label %lor.end
>> ; CHECK: i8 92, label %lor.end
>> ; CHECK: ]
>> }
>>
>> Modified: llvm/trunk/unittests/Support/IntegersSubsetTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/IntegersSubsetTest.cpp?rev=159527&r1=159526&r2=159527&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/Support/IntegersSubsetTest.cpp (original)
>> +++ llvm/trunk/unittests/Support/IntegersSubsetTest.cpp Mon Jul 2
>> 08:02:18 2012
>> @@ -193,20 +193,20 @@
>> const unsigned_ranges IntersectRes,
>> unsigned IntersectResSize
>> ) {
>> -
>> + unsigned successors[2] = {0, 1};
>> Mapping::RangesCollection Ranges;
>>
>> Mapping LHSMapping;
>> for (unsigned i = 0; i < LSize; ++i)
>> Ranges.push_back(Range(Int(LHS[i][0]), Int(LHS[i][1])));
>> - LHSMapping.add(Ranges);
>> + LHSMapping.add(Ranges, &successors[0]);
>>
>> Ranges.clear();
>>
>> Mapping RHSMapping;
>> for (unsigned i = 0; i < RSize; ++i)
>> Ranges.push_back(Range(Int(RHS[i][0]), Int(RHS[i][1])));
>> - RHSMapping.add(Ranges);
>> + RHSMapping.add(Ranges, &successors[1]);
>>
>> Mapping LExclude, Intersection;
>>
>> @@ -217,8 +217,10 @@
>>
>> unsigned i = 0;
>> for (Mapping::RangeIterator rei = LExclude.begin(),
>> - e = LExclude.end(); rei != e; ++rei, ++i)
>> + e = LExclude.end(); rei != e; ++rei, ++i) {
>> EXPECT_EQ(rei->first, Range(ExcludeRes[i][0], ExcludeRes[i][1]));
>> + EXPECT_EQ(rei->second, &successors[0]);
>> + }
>> } else
>> EXPECT_TRUE(LExclude.empty());
>>
>> @@ -227,8 +229,10 @@
>>
>> unsigned i = 0;
>> for (Mapping::RangeIterator ii = Intersection.begin(),
>> - e = Intersection.end(); ii != e; ++ii, ++i)
>> + e = Intersection.end(); ii != e; ++ii, ++i) {
>> EXPECT_EQ(ii->first, Range(IntersectRes[i][0],
>> IntersectRes[i][1]));
>> + EXPECT_EQ(ii->second, &successors[0]);
>> + }
>> } else
>> EXPECT_TRUE(Intersection.empty());
>>
>> @@ -241,9 +245,11 @@
>> EXPECT_EQ(LExclude.size(), ExcludeResSize);
>>
>> unsigned i = 0;
>> - for (Mapping::RangeIterator rei = LExclude.begin(),
>> - e = LExclude.end(); rei != e; ++rei, ++i)
>> - EXPECT_EQ(rei->first, Range(ExcludeRes[i][0], ExcludeRes[i][1]));
>> + for (Mapping::RangeIterator lei = LExclude.begin(),
>> + e = LExclude.end(); lei != e; ++lei, ++i) {
>> + EXPECT_EQ(lei->first, Range(ExcludeRes[i][0], ExcludeRes[i][1]));
>> + EXPECT_EQ(lei->second, &successors[0]);
>> + }
>> } else
>> EXPECT_TRUE(LExclude.empty());
>> }
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120702/e1c0697b/attachment.html>
More information about the llvm-commits
mailing list