llvm.org GIT mirror llvm / 1d79fff
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@280032 91177308-0d34-0410-b5e6-96231b3b80d8 Duncan P. N. Exon Smith 3 years ago
13 changed file(s) with 230 addition(s) and 54 deletion(s). Raw diff Collapse all Expand all
3434 namespace llvm {
3535
3636 template class iplist;
37 template> class ilist_iterator;
37 template > class ilist_iterator;
3838
3939 /// An access class for ilist_node private API.
4040 ///
145145 template struct ConstCorrectNodeType {
146146 typedef const ilist_node type;
147147 };
148
149 template struct IteratorHelper {
150 template static void increment(T *&I) {
151 I = ilist_node_access::getNext(*I);
152 }
153 template static void decrement(T *&I) {
154 I = ilist_node_access::getPrev(*I);
155 }
156 };
157 template <> struct IteratorHelper {
158 template static void increment(T *&I) {
159 IteratorHelper::decrement(I);
160 }
161 template static void decrement(T *&I) {
162 IteratorHelper::increment(I);
163 }
164 };
165
148166 } // end namespace ilist_detail
149167
150168 //===----------------------------------------------------------------------===//
151169 // Iterator for intrusive list.
152170 //
153 template >
171 template , bool IsReverse>
154172 class ilist_iterator
155173 : public std::iterator {
156174 public:
184202 // a nonconst iterator...
185203 template
186204 ilist_iterator(
187 const ilist_iterator> &RHS,
205 const ilist_iterator, IsReverse> &RHS,
188206 typename std::enable_if::value,
189207 void *>::type = nullptr)
190208 : NodePtr(RHS.getNodePtr()) {}
192210 // This is templated so that we can allow assigning to a const iterator from
193211 // a nonconst iterator...
194212 template
195 const ilist_iterator &operator=(const ilist_iterator &RHS) {
213 const ilist_iterator &
214 operator=(const ilist_iterator &RHS) {
196215 NodePtr = RHS.getNodePtr();
197216 return *this;
217 }
218
219 /// Convert from an iterator to its reverse.
220 ///
221 /// TODO: Roll this into the implicit constructor once we're sure that no one
222 /// is relying on the std::reverse_iterator off-by-one semantics.
223 ilist_iterator getReverse() const {
224 if (NodePtr)
225 return ilist_iterator(*NodePtr);
226 return ilist_iterator();
198227 }
199228
200229 void reset(pointer NP) { NodePtr = NP; }
216245
217246 // Increment and decrement operators...
218247 ilist_iterator &operator--() {
219 NodePtr = ilist_node_access::getPrev(*NodePtr);
220 assert(NodePtr && "--'d off the beginning of an ilist!");
248 ilist_detail::IteratorHelper::decrement(NodePtr);
221249 return *this;
222250 }
223251 ilist_iterator &operator++() {
224 NodePtr = ilist_node_access::getNext(*NodePtr);
252 ilist_detail::IteratorHelper::increment(NodePtr);
225253 return *this;
226254 }
227255 ilist_iterator operator--(int) {
355383 typedef ilist_iterator const_iterator;
356384 typedef size_t size_type;
357385 typedef ptrdiff_t difference_type;
358 typedef std::reverse_iterator const_reverse_iterator;
359 typedef std::reverse_iterator reverse_iterator;
386 typedef ilist_iterator const_reverse_iterator;
387 typedef ilist_iterator reverse_iterator;
360388
361389 iplist() = default;
362390 ~iplist() { clear(); }
368396 const_iterator end() const { return const_iterator(Sentinel); }
369397
370398 // reverse iterator creation methods.
371 reverse_iterator rbegin() { return reverse_iterator(end()); }
372 const_reverse_iterator rbegin() const{ return const_reverse_iterator(end()); }
373 reverse_iterator rend() { return reverse_iterator(begin()); }
374 const_reverse_iterator rend() const { return const_reverse_iterator(begin());}
375
399 reverse_iterator rbegin() { return ++reverse_iterator(Sentinel); }
400 const_reverse_iterator rbegin() const{ return ++const_reverse_iterator(Sentinel); }
401 reverse_iterator rend() { return reverse_iterator(Sentinel); }
402 const_reverse_iterator rend() const { return const_reverse_iterator(Sentinel); }
376403
377404 // Miscellaneous inspection routines.
378405 size_type max_size() const { return size_type(-1); }
5050 };
5151
5252 struct ilist_node_access;
53 template > class ilist_iterator;
53 template , bool IsReverse = false> class ilist_iterator;
5454 template class ilist_sentinel;
5555
5656 /// Templated wrapper class.
5858 friend class ilist_base;
5959 friend struct ilist_node_access;
6060 friend struct ilist_traits;
61 friend class ilist_iterator>;
61 friend class ilist_iterator, false>;
62 friend class ilist_iterator;
6263 friend class ilist_sentinel;
6364
6465 protected:
149149
150150 typedef Instructions::iterator instr_iterator;
151151 typedef Instructions::const_iterator const_instr_iterator;
152 typedef std::reverse_iterator reverse_instr_iterator;
153 typedef
154 std::reverse_iterator const_reverse_instr_iterator;
152 typedef Instructions::reverse_iterator reverse_instr_iterator;
153 typedef Instructions::const_reverse_iterator const_reverse_instr_iterator;
155154
156155 typedef MachineInstrBundleIterator iterator;
157156 typedef MachineInstrBundleIterator const_iterator;
192191 const_iterator begin() const { return instr_begin(); }
193192 iterator end () { return instr_end(); }
194193 const_iterator end () const { return instr_end(); }
195 reverse_iterator rbegin() { return instr_rbegin(); }
196 const_reverse_iterator rbegin() const { return instr_rbegin(); }
197 reverse_iterator rend () { return instr_rend(); }
198 const_reverse_iterator rend () const { return instr_rend(); }
194 reverse_iterator rbegin() { return reverse_iterator(end()); }
195 const_reverse_iterator rbegin() const {
196 return const_reverse_iterator(end());
197 }
198 reverse_iterator rend() { return reverse_iterator(begin()); }
199 const_reverse_iterator rend() const {
200 return const_reverse_iterator(begin());
201 }
199202
200203 /// Support for MachineInstr::getNextNode().
201204 static Instructions MachineBasicBlock::*getSublistAccess(MachineInstr *) {
444444 // Provide accessors for the MachineBasicBlock list...
445445 typedef BasicBlockListType::iterator iterator;
446446 typedef BasicBlockListType::const_iterator const_iterator;
447 typedef std::reverse_iterator const_reverse_iterator;
448 typedef std::reverse_iterator reverse_iterator;
447 typedef BasicBlockListType::const_reverse_iterator const_reverse_iterator;
448 typedef BasicBlockListType::reverse_iterator reverse_iterator;
449449
450450 /// Support for MachineBasicBlock::getNextNode().
451451 static BasicBlockListType MachineFunction::*
2828
2929 namespace llvm {
3030 class ValueSymbolTable;
31
32 template class ilist_iterator;
33 template class iplist;
34 template struct ilist_traits;
3531
3632 /// Template metafunction to get the parent type for a symbol table list.
3733 ///
6561 template
6662 class SymbolTableListTraits : public ilist_node_traits {
6763 typedef SymbolTableList ListTy;
64 typedef ilist_iterator iterator;
6865 typedef
6966 typename SymbolTableListParentType::type ItemParentClass;
7067
9390 public:
9491 void addNodeToList(ValueSubClass *V);
9592 void removeNodeFromList(ValueSubClass *V);
96 void transferNodesFromList(SymbolTableListTraits &L2,
97 ilist_iterator first,
98 ilist_iterator last);
99 //private:
93 void transferNodesFromList(SymbolTableListTraits &L2, iterator first,
94 iterator last);
95 // private:
10096 template
10197 void setSymTabObject(TPtr *, TPtr);
10298 static ValueSymbolTable *toPtr(ValueSymbolTable *P) { return P; }
28792879 used = false;
28802880 }
28812881 if (!used) {
2882 MI->eraseFromParent();
2883 ME = (*MBB)->instr_rend();
2882 MI++->eraseFromParent();
28842883 continue;
28852884 }
28862885 ++MI;
104104 // RET is generated as part of epilogue generation and hence we know
105105 // what the two instructions preceding it are and that it is safe to
106106 // insert RET above them.
107 MachineBasicBlock::reverse_instr_iterator RI(I);
107 MachineBasicBlock::reverse_instr_iterator RI = ++I.getReverse();
108108 assert(RI->getOpcode() == Lanai::LDW_RI && RI->getOperand(0).isReg() &&
109109 RI->getOperand(0).getReg() == Lanai::FP &&
110110 RI->getOperand(1).isReg() &&
116116 RI->getOperand(0).getReg() == Lanai::SP &&
117117 RI->getOperand(1).isReg() &&
118118 RI->getOperand(1).getReg() == Lanai::FP);
119 ++RI;
120 MachineBasicBlock::instr_iterator FI(RI.base());
119 MachineBasicBlock::instr_iterator FI = RI.getReverse();
121120 MBB.splice(std::next(I), &MBB, FI, I);
122121 FilledSlots += 2;
123122 } else {
153152 bool SawLoad = false;
154153 bool SawStore = false;
155154
156 for (MachineBasicBlock::reverse_instr_iterator I(Slot); I != MBB.instr_rend();
157 ++I) {
155 for (MachineBasicBlock::reverse_instr_iterator I = ++Slot.getReverse();
156 I != MBB.instr_rend(); ++I) {
158157 // skip debug value
159158 if (I->isDebugValue())
160159 continue;
161160
162161 // Convert to forward iterator.
163 MachineBasicBlock::instr_iterator FI(std::next(I).base());
162 MachineBasicBlock::instr_iterator FI = I.getReverse();
164163
165164 if (I->hasUnmodeledSideEffects() || I->isInlineAsm() || I->isLabel() ||
166165 FI == LastFiller || I->isPseudo())
9898 MachineInstr *
9999 X86FixupSetCCPass::findFlagsImpDef(MachineBasicBlock *MBB,
100100 MachineBasicBlock::reverse_iterator MI) {
101 auto MBBStart = MBB->instr_rend();
101 // FIXME: Should this be instr_rend(), and MI be reverse_instr_iterator?
102 auto MBBStart = MBB->rend();
102103 for (int i = 0; (i < SearchBound) && (MI != MBBStart); ++i, ++MI)
103104 for (auto &Op : MI->implicit_operands())
104105 if ((Op.getReg() == X86::EFLAGS) && (Op.isDef()))
14111411 void LoopReroll::DAGRootTracker::replace(const SCEV *IterCount) {
14121412 BasicBlock *Header = L->getHeader();
14131413 // Remove instructions associated with non-base iterations.
1414 for (BasicBlock::reverse_iterator J = Header->rbegin();
1415 J != Header->rend();) {
1414 for (BasicBlock::reverse_iterator J = Header->rbegin(), JE = Header->rend();
1415 J != JE;) {
14161416 unsigned I = Uses[&*J].find_first();
14171417 if (I > 0 && I < IL_All) {
1418 Instruction *D = &*J;
1419 DEBUG(dbgs() << "LRR: removing: " << *D << "\n");
1420 D->eraseFromParent();
1418 DEBUG(dbgs() << "LRR: removing: " << *J << "\n");
1419 J++->eraseFromParent();
14211420 continue;
14221421 }
14231422
25772577 // call result is not live (normal), nor are it's arguments
25782578 // (unless they're used again later). This adjustment is
25792579 // specifically what we need to relocate
2580 BasicBlock::reverse_iterator rend(Inst->getIterator());
2581 computeLiveInValues(BB->rbegin(), rend, LiveOut);
2580 computeLiveInValues(BB->rbegin(), ++Inst->getIterator().getReverse(),
2581 LiveOut);
25822582 LiveOut.remove(Inst);
25832583 Out.insert(LiveOut.begin(), LiveOut.end());
25842584 }
18671867 );
18681868
18691869 // Now find the sequence of instructions between PrevInst and Inst.
1870 BasicBlock::reverse_iterator InstIt(Inst->getIterator()),
1871 PrevInstIt(PrevInst->getIterator());
1872 --PrevInstIt;
1870 BasicBlock::reverse_iterator InstIt = ++Inst->getIterator().getReverse(),
1871 PrevInstIt =
1872 PrevInst->getIterator().getReverse();
18731873 while (InstIt != PrevInstIt) {
18741874 if (PrevInstIt == PrevInst->getParent()->rend()) {
18751875 PrevInstIt = Inst->getParent()->rbegin();
30353035 }
30363036 // Search up and down at the same time, because we don't know if the new
30373037 // instruction is above or below the existing scheduling region.
3038 BasicBlock::reverse_iterator UpIter(ScheduleStart->getIterator());
3038 BasicBlock::reverse_iterator UpIter =
3039 ++ScheduleStart->getIterator().getReverse();
30393040 BasicBlock::reverse_iterator UpperEnd = BB->rend();
3040 BasicBlock::iterator DownIter(ScheduleEnd);
3041 BasicBlock::iterator DownIter = ScheduleEnd->getIterator();
30413042 BasicBlock::iterator LowerEnd = BB->end();
30423043 for (;;) {
30433044 if (++ScheduleRegionSize > ScheduleRegionSizeLimit) {
1818 HashingTest.cpp
1919 ilistTestTemp.cpp
2020 IListBaseTest.cpp
21 IListIteratorTest.cpp
2122 IListNodeBaseTest.cpp
2223 IListSentinelTest.cpp
2324 ImmutableMapTest.cpp
0 //===- unittests/ADT/IListIteratorTest.cpp - ilist_iterator unit tests ----===//
1 //
2 // The LLVM Compiler Infrastructure
3 //
4 // This file is distributed under the University of Illinois Open Source
5 // License. See LICENSE.TXT for details.
6 //
7 //===----------------------------------------------------------------------===//
8
9 #include "llvm/ADT/ilist.h"
10 #include "gtest/gtest.h"
11
12 using namespace llvm;
13
14 namespace {
15
16 struct Node : ilist_node {};
17
18 TEST(IListIteratorTest, DefaultConstructor) {
19 iplist::iterator I;
20 iplist::reverse_iterator RI;
21 iplist::const_iterator CI;
22 iplist::const_reverse_iterator CRI;
23 EXPECT_EQ(nullptr, I.getNodePtr());
24 EXPECT_EQ(nullptr, CI.getNodePtr());
25 EXPECT_EQ(nullptr, RI.getNodePtr());
26 EXPECT_EQ(nullptr, CRI.getNodePtr());
27 EXPECT_EQ(I, I);
28 EXPECT_EQ(I, CI);
29 EXPECT_EQ(CI, I);
30 EXPECT_EQ(CI, CI);
31 EXPECT_EQ(RI, RI);
32 EXPECT_EQ(RI, CRI);
33 EXPECT_EQ(CRI, RI);
34 EXPECT_EQ(CRI, CRI);
35 EXPECT_EQ(I, RI.getReverse());
36 EXPECT_EQ(RI, I.getReverse());
37 }
38
39 TEST(IListIteratorTest, Empty) {
40 iplist L;
41
42 // Check iterators of L.
43 EXPECT_EQ(L.begin(), L.end());
44 EXPECT_EQ(L.rbegin(), L.rend());
45
46 // Reverse of end should be rend (since the sentinel sits on both sides).
47 EXPECT_EQ(L.end(), L.rend().getReverse());
48 EXPECT_EQ(L.rend(), L.end().getReverse());
49
50 // Iterators shouldn't match default constructors.
51 iplist::iterator I;
52 iplist::reverse_iterator RI;
53 EXPECT_NE(I, L.begin());
54 EXPECT_NE(I, L.end());
55 EXPECT_NE(RI, L.rbegin());
56 EXPECT_NE(RI, L.rend());
57
58 // Don't delete nodes.
59 L.clearAndLeakNodesUnsafely();
60 }
61
62 TEST(IListIteratorTest, OneNodeList) {
63 iplist L;
64 Node A;
65 L.insert(L.end(), &A);
66
67 // Check address of reference.
68 EXPECT_EQ(&A, &*L.begin());
69 EXPECT_EQ(&A, &*L.rbegin());
70
71 // Check that the handle matches.
72 EXPECT_EQ(L.rbegin().getNodePtr(), L.begin().getNodePtr());
73
74 // Check iteration.
75 EXPECT_EQ(L.end(), ++L.begin());
76 EXPECT_EQ(L.begin(), --L.end());
77 EXPECT_EQ(L.rend(), ++L.rbegin());
78 EXPECT_EQ(L.rbegin(), --L.rend());
79
80 // Check conversions.
81 EXPECT_EQ(L.rbegin(), L.begin().getReverse());
82 EXPECT_EQ(L.begin(), L.rbegin().getReverse());
83
84 // Don't delete nodes.
85 L.clearAndLeakNodesUnsafely();
86 }
87
88 TEST(IListIteratorTest, TwoNodeList) {
89 iplist L;
90 Node A, B;
91 L.insert(L.end(), &A);
92 L.insert(L.end(), &B);
93
94 // Check order.
95 EXPECT_EQ(&A, &*L.begin());
96 EXPECT_EQ(&B, &*++L.begin());
97 EXPECT_EQ(L.end(), ++++L.begin());
98 EXPECT_EQ(&B, &*L.rbegin());
99 EXPECT_EQ(&A, &*++L.rbegin());
100 EXPECT_EQ(L.rend(), ++++L.rbegin());
101
102 // Check conversions.
103 EXPECT_EQ(++L.rbegin(), L.begin().getReverse());
104 EXPECT_EQ(L.rbegin(), (++L.begin()).getReverse());
105 EXPECT_EQ(++L.begin(), L.rbegin().getReverse());
106 EXPECT_EQ(L.begin(), (++L.rbegin()).getReverse());
107
108 // Don't delete nodes.
109 L.clearAndLeakNodesUnsafely();
110 }
111
112 TEST(IListIteratorTest, CheckEraseForward) {
113 iplist L;
114 Node A, B;
115 L.insert(L.end(), &A);
116 L.insert(L.end(), &B);
117
118 // Erase nodes.
119 auto I = L.begin();
120 EXPECT_EQ(&A, &*I);
121 EXPECT_EQ(&A, L.remove(I++));
122 EXPECT_EQ(&B, &*I);
123 EXPECT_EQ(&B, L.remove(I++));
124 EXPECT_EQ(L.end(), I);
125
126 // Don't delete nodes.
127 L.clearAndLeakNodesUnsafely();
128 }
129
130 TEST(IListIteratorTest, CheckEraseReverse) {
131 iplist L;
132 Node A, B;
133 L.insert(L.end(), &A);
134 L.insert(L.end(), &B);
135
136 // Erase nodes.
137 auto RI = L.rbegin();
138 EXPECT_EQ(&B, &*RI);
139 EXPECT_EQ(&B, L.remove(&*RI++));
140 EXPECT_EQ(&A, &*RI);
141 EXPECT_EQ(&A, L.remove(&*RI++));
142 EXPECT_EQ(L.rend(), RI);
143
144 // Don't delete nodes.
145 L.clearAndLeakNodesUnsafely();
146 }
147
148 } // end namespace