<div dir="ltr">FWIW, it was intentional in order to make sure the return of operator= is a reference to *this and to ensure self assignment works, etc.<div><br></div><div>I'll try to find a way to test this w/o triggering warnings though. =D<br><div><br><div class="gmail_quote"><div dir="ltr">On Fri, May 26, 2017 at 5:07 AM Daniel Jasper via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Removed in r303974.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 26, 2017 at 1:37 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="m_3355342364554572297h5">On Fri, May 26, 2017 at 5:10 AM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Thu May 25 22:10:00 2017<br>
New Revision: 303964<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=303964&view=rev" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://llvm.org/viewvc/llvm-project?rev=303964&view=rev</a><br>
Log:<br>
[IR] Add an iterator and range accessor for the PHI nodes of a basic<br>
block.<br>
<br>
This allows writing much more natural and readable range based for loops<br>
directly over the PHI nodes. It also takes advantage of the same tricks<br>
for terminating the sequence as the hand coded versions.<br>
<br>
I've replaced one example of this mostly to showcase the difference and<br>
I've added a unit test to make sure the facilities really work the way<br>
they're intended. I want to use this inside of SimpleLoopUnswitch but it<br>
seems generally nice.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D33533" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">https://reviews.llvm.org/D33533</a><br>
<br>
Added:<br>
llvm/trunk/unittests/IR/BasicBlockTest.cpp<br>
Modified:<br>
llvm/trunk/include/llvm/IR/BasicBlock.h<br>
llvm/trunk/lib/IR/BasicBlock.cpp<br>
llvm/trunk/unittests/IR/CMakeLists.txt<br>
<br>
Modified: llvm/trunk/include/llvm/IR/BasicBlock.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/BasicBlock.h?rev=303964&r1=303963&r2=303964&view=diff" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/BasicBlock.h?rev=303964&r1=303963&r2=303964&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/BasicBlock.h (original)<br>
+++ llvm/trunk/include/llvm/IR/BasicBlock.h Thu May 25 22:10:00 2017<br>
@@ -33,6 +33,7 @@ class Function;<br>
class LandingPadInst;<br>
class LLVMContext;<br>
class Module;<br>
+class PHINode;<br>
class TerminatorInst;<br>
class ValueSymbolTable;<br>
<br>
@@ -261,6 +262,50 @@ public:<br>
inline const Instruction &back() const { return InstList.back(); }<br>
inline Instruction &back() { return InstList.back(); }<br>
<br>
+ /// Iterator to walk just the phi nodes in the basic block.<br>
+ template <typename PHINodeT = PHINode, typename BBIteratorT = iterator><br>
+ class phi_iterator_impl<br>
+ : public iterator_facade_base<phi_iterator_impl<PHINodeT, BBIteratorT>,<br>
+ std::forward_iterator_tag, PHINodeT> {<br>
+ friend BasicBlock;<br>
+<br>
+ PHINodeT *PN;<br>
+<br>
+ phi_iterator_impl(PHINodeT *PN) : PN(PN) {}<br>
+<br>
+ public:<br>
+ // Allow default construction to build variables, but this doesn't build<br>
+ // a useful iterator.<br>
+ phi_iterator_impl() = default;<br>
+<br>
+ // Allow conversion between instantiations where valid.<br>
+ template <typename PHINodeU, typename BBIteratorU><br>
+ phi_iterator_impl(const phi_iterator_impl<PHINodeU, BBIteratorU> &Arg)<br>
+ : PN(Arg.PN) {}<br>
+<br>
+ bool operator==(const phi_iterator_impl &Arg) const { return PN == Arg.PN; }<br>
+<br>
+ PHINodeT &operator*() const { return *PN; }<br>
+<br>
+ using phi_iterator_impl::iterator_facade_base::operator++;<br>
+ phi_iterator_impl &operator++() {<br>
+ assert(PN && "Cannot increment the end iterator!");<br>
+ PN = dyn_cast<PHINodeT>(std::next(BBIteratorT(PN)));<br>
+ return *this;<br>
+ }<br>
+ };<br>
+ typedef phi_iterator_impl<> phi_iterator;<br>
+ typedef phi_iterator_impl<const PHINode, BasicBlock::const_iterator><br>
+ const_phi_iterator;<br>
+<br>
+ /// Returns a range that iterates over the phis in the basic block.<br>
+ ///<br>
+ /// Note that this cannot be used with basic blocks that have no terminator.<br>
+ iterator_range<const_phi_iterator> phis() const {<br>
+ return const_cast<BasicBlock *>(this)->phis();<br>
+ }<br>
+ iterator_range<phi_iterator> phis();<br>
+<br>
/// \brief Return the underlying instruction list container.<br>
///<br>
/// Currently you need to access the underlying instruction list container<br>
<br>
Modified: llvm/trunk/lib/IR/BasicBlock.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/BasicBlock.cpp?rev=303964&r1=303963&r2=303964&view=diff" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/BasicBlock.cpp?rev=303964&r1=303963&r2=303964&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/BasicBlock.cpp (original)<br>
+++ llvm/trunk/lib/IR/BasicBlock.cpp Thu May 25 22:10:00 2017<br>
@@ -263,6 +263,10 @@ const BasicBlock *BasicBlock::getUniqueS<br>
return SuccBB;<br>
}<br>
<br>
+iterator_range<BasicBlock::phi_iterator> BasicBlock::phis() {<br>
+ return make_range<phi_iterator>(dyn_cast<PHINode>(&front()), nullptr);<br>
+}<br>
+<br>
/// This method is used to notify a BasicBlock that the<br>
/// specified Predecessor of the block is no longer able to reach it. This is<br>
/// actually not used to update the Predecessor list, but is actually used to<br>
@@ -389,13 +393,11 @@ BasicBlock *BasicBlock::splitBasicBlock(<br>
// Loop over any phi nodes in the basic block, updating the BB field of<br>
// incoming values...<br>
BasicBlock *Successor = *I;<br>
- PHINode *PN;<br>
- for (BasicBlock::iterator II = Successor->begin();<br>
- (PN = dyn_cast<PHINode>(II)); ++II) {<br>
- int IDX = PN->getBasicBlockIndex(this);<br>
- while (IDX != -1) {<br>
- PN->setIncomingBlock((unsigned)IDX, New);<br>
- IDX = PN->getBasicBlockIndex(this);<br>
+ for (auto &PN : Successor->phis()) {<br>
+ int Idx = PN.getBasicBlockIndex(this);<br>
+ while (Idx != -1) {<br>
+ PN.setIncomingBlock((unsigned)Idx, New);<br>
+ Idx = PN.getBasicBlockIndex(this);<br>
}<br>
}<br>
}<br>
<br>
Added: llvm/trunk/unittests/IR/BasicBlockTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/BasicBlockTest.cpp?rev=303964&view=auto" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/BasicBlockTest.cpp?rev=303964&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/IR/BasicBlockTest.cpp (added)<br>
+++ llvm/trunk/unittests/IR/BasicBlockTest.cpp Thu May 25 22:10:00 2017<br>
@@ -0,0 +1,75 @@<br>
+//===- llvm/unittest/IR/BasicBlockTest.cpp - BasicBlock unit tests --------===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "llvm/IR/BasicBlock.h"<br>
+#include "llvm/ADT/STLExtras.h"<br>
+#include "llvm/IR/Function.h"<br>
+#include "llvm/IR/IRBuilder.h"<br>
+#include "llvm/IR/LLVMContext.h"<br>
+#include "llvm/IR/Module.h"<br>
+#include "llvm/IR/NoFolder.h"<br>
+#include "gmock/gmock-matchers.h"<br>
+#include "gtest/gtest.h"<br>
+#include <memory><br>
+<br>
+namespace llvm {<br>
+namespace {<br>
+<br>
+TEST(BasicBlockTest, PhiRange) {<br>
+ LLVMContext Context;<br>
+<br>
+ // Create the main block.<br>
+ std::unique_ptr<BasicBlock> BB(BasicBlock::Create(Context));<br>
+<br>
+ // Create some predecessors of it.<br>
+ std::unique_ptr<BasicBlock> BB1(BasicBlock::Create(Context));<br>
+ BranchInst::Create(BB.get(), BB1.get());<br>
+ std::unique_ptr<BasicBlock> BB2(BasicBlock::Create(Context));<br>
+ BranchInst::Create(BB.get(), BB2.get());<br>
+<br>
+ // Make it a cycle.<br>
+ auto *BI = BranchInst::Create(BB.get(), BB.get());<br>
+<br>
+ // Now insert some PHI nodes.<br>
+ auto *Int32Ty = Type::getInt32Ty(Context);<br>
+ auto *P1 = PHINode::Create(Int32Ty, /*NumReservedValues*/ 3, "phi.1", BI);<br>
+ auto *P2 = PHINode::Create(Int32Ty, /*NumReservedValues*/ 3, "phi.2", BI);<br>
+ auto *P3 = PHINode::Create(Int32Ty, /*NumReservedValues*/ 3, "phi.3", BI);<br>
+<br>
+ // Some non-PHI nodes.<br>
+ auto *Sum = BinaryOperator::CreateAdd(P1, P2, "sum", BI);<br>
+<br>
+ // Now wire up the incoming values that are interesting.<br>
+ P1->addIncoming(P2, BB.get());<br>
+ P2->addIncoming(P1, BB.get());<br>
+ P3->addIncoming(Sum, BB.get());<br>
+<br>
+ // Finally, let's iterate them, which is the thing we're trying to test.<br>
+ // We'll use this to wire up the rest of the incoming values.<br>
+ for (auto &PN : BB->phis()) {<br>
+ PN.addIncoming(UndefValue::get(Int32Ty), BB1.get());<br>
+ PN.addIncoming(UndefValue::get(Int32Ty), BB2.get());<br>
+ }<br>
+<br>
+ // Test that we can use const iterators and generally that the iterators<br>
+ // behave like iterators.<br>
+ BasicBlock::const_phi_iterator CI;<br>
+ CI = CI = BB->phis().begin();<br></blockquote><div><br></div></div></div><div>Is this intentional? It seems to trigger GCC's -Wsequence-point.</div><div><div class="m_3355342364554572297h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ EXPECT_NE(CI, BB->phis().end());<br>
+<br>
+ // And iterate a const range.<br>
+ for (const auto &PN : const_cast<const BasicBlock *>(BB.get())->phis()) {<br>
+ EXPECT_EQ(BB.get(), PN.getIncomingBlock(0));<br>
+ EXPECT_EQ(BB1.get(), PN.getIncomingBlock(1));<br>
+ EXPECT_EQ(BB2.get(), PN.getIncomingBlock(2));<br>
+ }<br>
+}<br>
+<br>
+} // End anonymous namespace.<br>
+} // End llvm namespace.<br>
<br>
Modified: llvm/trunk/unittests/IR/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/CMakeLists.txt?rev=303964&r1=303963&r2=303964&view=diff" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/CMakeLists.txt?rev=303964&r1=303963&r2=303964&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/IR/CMakeLists.txt (original)<br>
+++ llvm/trunk/unittests/IR/CMakeLists.txt Thu May 25 22:10:00 2017<br>
@@ -8,6 +8,7 @@ set(LLVM_LINK_COMPONENTS<br>
set(IRSources<br>
AsmWriterTest.cpp<br>
AttributesTest.cpp<br>
+ BasicBlockTest.cpp<br>
ConstantRangeTest.cpp<br>
ConstantsTest.cpp<br>
DebugInfoTest.cpp<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="m_3355342364554572297m_5037282843377960885cremed" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div></div>