From 0d256c0f5d246e884ec0320b703bb3a0606aefc5 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sun, 26 Mar 2017 02:49:23 +0000 Subject: [PATCH] [IR] Make SwitchInst::CaseIt almost a normal iterator. This moves it to the iterator facade utilities giving it full random access semantics, etc. It can also now be used with standard algorithms like std::all_of and std::any_of and range adaptors like llvm::reverse. Also make the semantics of iterating match what every other iterator uses and forbid decrementing past the begin iterator. This was used as a hacky way to work around iterator invalidation. However, every instance trying to do this failed to actually avoid touching invalid iterators despite the clear documentation that the removed and all subsequent iterators become invalid including the end iterator. So I've added a return of the next iterator to removeCase and rewritten the loops that were doing this to correctly follow the iterator pattern of either incremneting or removing and assigning fresh values to the iterator and the end. In one case we were trying to go backwards to make this cleaner but it doesn't actually work. I've made that code match the code we use everywhere else to remove cases as we iterate. This changes the order of cases in one test output and I moved that test to CHECK-DAG so it wouldn't care -- the order isn't semantically meaningful anyways. llvm-svn: 298791 --- llvm/include/llvm/IR/Instructions.h | 64 +++++++++---------- llvm/lib/IR/Instructions.cpp | 4 +- .../Scalar/CorrelatedValuePropagation.cpp | 13 ++-- llvm/lib/Transforms/Utils/Local.cpp | 10 +-- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 17 ++--- .../CorrelatedValuePropagation/basic.ll | 4 +- 6 files changed, 60 insertions(+), 52 deletions(-) diff --git a/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h index 6ab9b51cb8ff..3e94ee443b88 100644 --- a/llvm/include/llvm/IR/Instructions.h +++ b/llvm/include/llvm/IR/Instructions.h @@ -3094,14 +3094,22 @@ public: static const unsigned DefaultPseudoIndex = static_cast(~0L-1); template - class CaseIteratorT { + class CaseIteratorT + : public iterator_facade_base< + CaseIteratorT, + std::random_access_iterator_tag, + CaseIteratorT> { protected: SwitchInstTy *SI; - unsigned Index; + ptrdiff_t Index; public: typedef CaseIteratorT Self; + /// Default constructed iterator is in an invalid state until assigned to + /// a case for a particular switch. + CaseIteratorT() : SI(nullptr) {} + /// Initializes case iterator for given SwitchInst and for given /// case number. CaseIteratorT(SwitchInstTy *SI, unsigned CaseNum) { @@ -3143,43 +3151,32 @@ public: return Index != DefaultPseudoIndex ? Index + 1 : 0; } - Self operator++() { - // Check index correctness after increment. + Self &operator+=(ptrdiff_t N) { + // Check index correctness after addition. // Note: Index == getNumCases() means end(). - assert(Index+1 <= SI->getNumCases() && "Index out the number of cases."); - ++Index; - return *this; - } - Self operator++(int) { - Self tmp = *this; - ++(*this); - return tmp; - } - Self operator--() { - // 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()) && + assert(Index + N >= 0 && Index + N <= SI->getNumCases() && "Index out the number of cases."); - --Index; + Index += N; return *this; } - Self operator--(int) { - Self tmp = *this; - --(*this); - return tmp; + Self &operator-=(ptrdiff_t N) { + // Check index correctness after subtraction. + // Note: Index == getNumCases() means end(). + assert(Index - N >= 0 && Index - N <= SI->getNumCases() && + "Index out the number of cases."); + Index -= N; + return *this; } bool operator==(const Self& RHS) const { - assert(RHS.SI == SI && "Incompatible operators."); - return RHS.Index == Index; + assert(SI == RHS.SI && "Incompatible operators."); + return Index == RHS.Index; } - bool operator!=(const Self& RHS) const { - assert(RHS.SI == SI && "Incompatible operators."); - return RHS.Index != Index; - } - Self &operator*() { - return *this; + bool operator<(const Self& RHS) const { + assert(SI == RHS.SI && "Incompatible operators."); + return Index < RHS.Index; } + Self &operator*() { return *this; } + const Self &operator*() const { return *this; } }; typedef CaseIteratorT @@ -3325,8 +3322,9 @@ public: /// index idx and above. /// Note: /// This action invalidates iterators for all cases following the one removed, - /// including the case_end() iterator. - void removeCase(CaseIt i); + /// including the case_end() iterator. It returns an iterator for the next + /// case. + CaseIt removeCase(CaseIt i); unsigned getNumSuccessors() const { return getNumOperands()/2; } BasicBlock *getSuccessor(unsigned idx) const { diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp index f3a1022a2a66..02645511efc9 100644 --- a/llvm/lib/IR/Instructions.cpp +++ b/llvm/lib/IR/Instructions.cpp @@ -3662,7 +3662,7 @@ void SwitchInst::addCase(ConstantInt *OnVal, BasicBlock *Dest) { /// removeCase - This method removes the specified case and its successor /// from the switch instruction. -void SwitchInst::removeCase(CaseIt i) { +SwitchInst::CaseIt SwitchInst::removeCase(CaseIt i) { unsigned idx = i.getCaseIndex(); assert(2 + idx*2 < getNumOperands() && "Case index out of range!!!"); @@ -3680,6 +3680,8 @@ void SwitchInst::removeCase(CaseIt i) { OL[NumOps-2].set(nullptr); OL[NumOps-2+1].set(nullptr); setNumHungOffUseOperands(NumOps-2); + + return CaseIt(this, idx); } /// growOperands - grow operands - This grows the operand list in response diff --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp index 43a78cdde618..42706cc80c76 100644 --- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp +++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp @@ -235,8 +235,7 @@ static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) { // Analyse each switch case in turn. This is done in reverse order so that // removing a case doesn't cause trouble for the iteration. bool Changed = false; - for (SwitchInst::CaseIt CI = SI->case_end(), CE = SI->case_begin(); CI-- != CE; - ) { + for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) { ConstantInt *Case = CI.getCaseValue(); // Check to see if the switch condition is equal to/not equal to the case @@ -271,7 +270,8 @@ static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) { if (State == LazyValueInfo::False) { // This case never fires - remove it. CI.getCaseSuccessor()->removePredecessor(BB); - SI->removeCase(CI); // Does not invalidate the iterator. + CI = SI->removeCase(CI); + CE = SI->case_end(); // The condition can be modified by removePredecessor's PHI simplification // logic. @@ -279,7 +279,9 @@ static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) { ++NumDeadCases; Changed = true; - } else if (State == LazyValueInfo::True) { + continue; + } + if (State == LazyValueInfo::True) { // This case always fires. Arrange for the switch to be turned into an // unconditional branch by replacing the switch condition with the case // value. @@ -288,6 +290,9 @@ static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) { Changed = true; break; } + + // Increment the case iterator sense we didn't delete it. + ++CI; } if (Changed) diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp index b1f3e43f7722..0448913f9b3e 100644 --- a/llvm/lib/Transforms/Utils/Local.cpp +++ b/llvm/lib/Transforms/Utils/Local.cpp @@ -130,8 +130,7 @@ bool llvm::ConstantFoldTerminator(BasicBlock *BB, bool DeleteDeadConditions, } // Figure out which case it goes to. - for (SwitchInst::CaseIt i = SI->case_begin(), e = SI->case_end(); - i != e; ++i) { + for (auto i = SI->case_begin(), e = SI->case_end(); i != e;) { // Found case matching a constant operand? if (i.getCaseValue() == CI) { TheOnlyDest = i.getCaseSuccessor(); @@ -165,8 +164,8 @@ bool llvm::ConstantFoldTerminator(BasicBlock *BB, bool DeleteDeadConditions, } // Remove this entry. DefaultDest->removePredecessor(SI->getParent()); - SI->removeCase(i); - --i; --e; + i = SI->removeCase(i); + e = SI->case_end(); continue; } @@ -174,6 +173,9 @@ bool llvm::ConstantFoldTerminator(BasicBlock *BB, bool DeleteDeadConditions, // We do this by reseting "TheOnlyDest" to null when we find two non-equal // destinations. if (i.getCaseSuccessor() != TheOnlyDest) TheOnlyDest = nullptr; + + // Increment this iterator as we haven't removed the case. + ++i; } if (CI && !TheOnlyDest) { diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index 7661b98a7169..766b85957aae 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -4156,15 +4156,16 @@ bool SimplifyCFGOpt::SimplifyUnreachable(UnreachableInst *UI) { } } } else if (auto *SI = dyn_cast(TI)) { - for (SwitchInst::CaseIt i = SI->case_begin(), e = SI->case_end(); i != e; - ++i) - if (i.getCaseSuccessor() == BB) { - BB->removePredecessor(SI->getParent()); - SI->removeCase(i); - --i; - --e; - Changed = true; + for (auto i = SI->case_begin(), e = SI->case_end(); i != e;) { + if (i.getCaseSuccessor() != BB) { + ++i; + continue; } + BB->removePredecessor(SI->getParent()); + i = SI->removeCase(i); + e = SI->case_end(); + Changed = true; + } } else if (auto *II = dyn_cast(TI)) { if (II->getUnwindDest() == BB) { removeUnwindEdge(TI->getParent()); diff --git a/llvm/test/Transforms/CorrelatedValuePropagation/basic.ll b/llvm/test/Transforms/CorrelatedValuePropagation/basic.ll index 9836c7f80778..14b9a1999cc3 100644 --- a/llvm/test/Transforms/CorrelatedValuePropagation/basic.ll +++ b/llvm/test/Transforms/CorrelatedValuePropagation/basic.ll @@ -115,9 +115,9 @@ negative: i32 1, label %out ; CHECK-NOT: i32 1 i32 -1, label %next -; CHECK: i32 -1, label %next +; CHECK-DAG: i32 -1, label %next i32 -2, label %next -; CHECK: i32 -2, label %next +; CHECK-DAG: i32 -2, label %next i32 2, label %out ; CHECK-NOT: i32 2 i32 3, label %out