<div dir="ltr">Thank you, couldn't find the commit thread till you replied :)<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 7, 2017 at 2:12 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">r297234 should fix it.<div><div class="h5"><div><br><div><div><blockquote type="cite"><div>On 2017-Mar-07, at 11:25, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:</div><br class="m_4338975777332838043Apple-interchange-newline"><div><div style="word-wrap:break-word">[DannyB replied to the old phab review, but let's move the discussion to the commit.]<div><br></div><div>My guess is that find() should be updated to return end().</div><div><br></div><div><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div>On 2017-Mar-07, at 10:57, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:</div><br class="m_4338975777332838043Apple-interchange-newline"><div><div dir="ltr">(and FWIW, the reason it needs fixing is it makes it impossible to use find in unit tests, see the FIXME i'm about to commit to smallptrsettest)<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 7, 2017 at 10:43 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> <wbr>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">So, this change breaks smallptrset's find function when it is on.<div><br></div><div>Trivial test case:<br><div>  SmallPtrSet<int *, 8> IntSet;</div><div>  int A;</div><div>  int *B = &A;</div></div><div><div>  EXPECT_FALSE(IntSet.find(B) == IntSet.end());</div></div><div><br></div><div>Because you've made the end iterator point somewhere else, this will no longer work.</div><div><br></div><div>Please fix.</div><div><br></div></div><div class="m_4338975777332838043HOEnZb"><div class="m_4338975777332838043h5"></div></div></blockquote></div></div></div></blockquote></div></div></blockquote><div><br></div><div><br><div><blockquote type="cite"><div>On 2016-Dec-13, at 16:15, Mandeep Singh Grang via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="m_4338975777332838043Apple-interchange-newline"><div><div>Author: mgrang<br>Date: Tue Dec 13 18:15:57 2016<br>New Revision: 289619<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=289619&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=289619&view=rev</a><br>Log:<br>[llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in codegen<br><br>Summary:<br>Given a flag (-mllvm -reverse-iterate) this patch will enable iteration of SmallPtrSet in reverse order.<br>The idea is to compile the same source with and without this flag and expect the code to not change.<br>If there is a difference in codegen then it would mean that the codegen is sensitive to the iteration order of SmallPtrSet.<br>This is enabled only with LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS.<br><br>Reviewers: chandlerc, dexonsmith, mehdi_amini<br><br>Subscribers: mgorny, emaste, llvm-commits<br><br>Differential Revision: <a href="https://reviews.llvm.org/D26718" target="_blank">https://reviews.llvm.org/<wbr>D26718</a><br><br>Added:<br>    llvm/trunk/unittests/ADT/<wbr>ReverseIterationTest.cpp<br>Modified:<br>    llvm/trunk/include/llvm/<wbr>ADT/SmallPtrSet.h<br>    llvm/trunk/lib/Support/<wbr>CommandLine.cpp<br>    llvm/trunk/unittests/ADT/<wbr>CMakeLists.txt<br><br>Modified: llvm/trunk/include/llvm/ADT/<wbr>SmallPtrSet.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallPtrSet.h?rev=289619&r1=289618&r2=289619&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/ADT/SmallPtrSet.h?rev=<wbr>289619&r1=289618&r2=289619&<wbr>view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/include/llvm/ADT/<wbr>SmallPtrSet.h (original)<br>+++ llvm/trunk/include/llvm/ADT/<wbr>SmallPtrSet.h Tue Dec 13 18:15:57 2016<br>@@ -15,6 +15,7 @@<br> #ifndef LLVM_ADT_SMALLPTRSET_H<br> #define LLVM_ADT_SMALLPTRSET_H<br><br>+#include "llvm/Config/abi-breaking.h"<br> #include "llvm/Support/Compiler.h"<br> #include "llvm/Support/<wbr>PointerLikeTypeTraits.h"<br> #include <cassert><br>@@ -25,6 +26,13 @@<br> #include <iterator><br> #include <utility><br><br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+namespace llvm {<br>+template <class T = void> struct ReverseIterate { static bool value; };<br>+template <class T> bool ReverseIterate<T>::value = true;<br>+}<br>+#endif<br>+<br> namespace llvm {<br><br> /// SmallPtrSetImplBase - This is the common code shared among all the<br>@@ -206,6 +214,12 @@ protected:<br> public:<br>   explicit SmallPtrSetIteratorImpl(const void *const *BP, const void*const *E)<br>     : Bucket(BP), End(E) {<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+    if (ReverseIterate<bool>::value) {<br>+      RetreatIfNotValid();<br>+      return;<br>+    }<br>+#endif<br>     AdvanceIfNotValid();<br>   }<br><br>@@ -227,6 +241,17 @@ protected:<br>             *Bucket == SmallPtrSetImplBase::<wbr>getTombstoneMarker()))<br>       ++Bucket;<br>   }<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+  void RetreatIfNotValid() {<br>+    --Bucket;<br>+    assert(Bucket <= End);<br>+    while (Bucket != End &&<br>+           (*Bucket == SmallPtrSetImplBase::<wbr>getEmptyMarker() ||<br>+            *Bucket == SmallPtrSetImplBase::<wbr>getTombstoneMarker())) {<br>+      --Bucket;<br>+    }<br>+  }<br>+#endif<br> };<br><br> /// SmallPtrSetIterator - This implements a const_iterator for SmallPtrSet.<br>@@ -252,13 +277,27 @@ public:<br>   }<br><br>   inline SmallPtrSetIterator& operator++() {          // Preincrement<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+    if (ReverseIterate<bool>::value) {<br>+      RetreatIfNotValid();<br>+      return *this;<br>+    }<br>+#endif<br>     ++Bucket;<br>     AdvanceIfNotValid();<br>     return *this;<br>   }<br><br>   SmallPtrSetIterator operator++(int) {        // Postincrement<br>-    SmallPtrSetIterator tmp = *this; ++*this; return tmp;<br>+    SmallPtrSetIterator tmp = *this;<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+    if (ReverseIterate<bool>::value) {<br>+      --*this;<br>+      return tmp;<br>+    }<br>+#endif<br>+    ++*this;<br>+    return tmp;<br>   }<br> };<br><br>@@ -343,9 +382,22 @@ public:<br>   }<br><br>   inline iterator begin() const {<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+    if (ReverseIterate<bool>::value)<br>+      return endPtr();<br>+#endif<br>     return iterator(CurArray, EndPointer());<br>   }<br>   inline iterator end() const {<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+    if (ReverseIterate<bool>::value)<br>+      return iterator(CurArray, CurArray);<br>+#endif<br>+    return endPtr();<br>+  }<br>+<br>+private:<br>+  inline iterator endPtr() const {<br>     const void *const *End = EndPointer();<br>     return iterator(End, End);<br>   }<br><br>Modified: llvm/trunk/lib/Support/<wbr>CommandLine.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=289619&r1=289618&r2=289619&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Support/CommandLine.cpp?rev=<wbr>289619&r1=289618&r2=289619&<wbr>view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/lib/Support/<wbr>CommandLine.cpp (original)<br>+++ llvm/trunk/lib/Support/<wbr>CommandLine.cpp Tue Dec 13 18:15:57 2016<br>@@ -45,6 +45,17 @@ using namespace cl;<br><br> #define DEBUG_TYPE "commandline"<br><br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+namespace llvm {<br>+// If LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS is set the flag -mllvm -reverse-iterate<br>+// can be used to toggle forward/reverse iteration of unordered containers.<br>+// This will help uncover differences in codegen caused due to undefined<br>+// iteration order.<br>+static cl::opt<bool, true> ReverseIteration("reverse-<wbr>iterate",<br>+  cl::location(ReverseIterate<<wbr>bool>::value), cl::init(true));<br>+}<br>+#endif<br>+<br> //===-------------------------<wbr>------------------------------<wbr>---------------===//<br> // Template instantiations and anchors.<br> //<br><br>Modified: llvm/trunk/unittests/ADT/<wbr>CMakeLists.txt<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/CMakeLists.txt?rev=289619&r1=289618&r2=289619&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>ADT/CMakeLists.txt?rev=289619&<wbr>r1=289618&r2=289619&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/unittests/ADT/<wbr>CMakeLists.txt (original)<br>+++ llvm/trunk/unittests/ADT/<wbr>CMakeLists.txt Tue Dec 13 18:15:57 2016<br>@@ -41,6 +41,7 @@ set(ADTSources<br>   PostOrderIteratorTest.cpp<br>   PriorityWorklistTest.cpp<br>   RangeAdapterTest.cpp<br>+  ReverseIterationTest.cpp<br>   SCCIteratorTest.cpp<br>   STLExtrasTest.cpp<br>   ScopeExitTest.cpp<br><br>Added: llvm/trunk/unittests/ADT/<wbr>ReverseIterationTest.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ReverseIterationTest.cpp?rev=289619&view=auto" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>ADT/ReverseIterationTest.cpp?<wbr>rev=289619&view=auto</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/unittests/ADT/<wbr>ReverseIterationTest.cpp (added)<br>+++ llvm/trunk/unittests/ADT/<wbr>ReverseIterationTest.cpp Tue Dec 13 18:15:57 2016<br>@@ -0,0 +1,39 @@<br>+//===- llvm/unittest/ADT/<wbr>ReverseIterationTest.cpp ------------------------------<wbr>===//<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>+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>+//<br>+// ReverseIteration unit tests.<br>+//<br>+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>+<br>+#include "gtest/gtest.h"<br>+#include "llvm/ADT/SmallPtrSet.h"<br>+<br>+#if LLVM_ENABLE_ABI_BREAKING_<wbr>CHECKS<br>+using namespace llvm;<br>+<br>+TEST(ReverseIterationTest, SmallPtrSetTest) {<br>+<br>+  SmallPtrSet<void*, 4> Set;<br>+  void *Ptrs[] = { (void*)0x1, (void*)0x2, (void*)0x3, (void*)0x4 };<br>+  void *ReversePtrs[] = { (void*)0x4, (void*)0x3, (void*)0x2, (void*)0x1 };<br>+<br>+  for (auto *Ptr: Ptrs)<br>+    Set.insert(Ptr);<br>+<br>+  // Check forward iteration.<br>+  ReverseIterate<bool>::value = false;<br>+  for (const auto &Tuple : zip(Set, Ptrs))<br>+    ASSERT_EQ(std::get<0>(<wbr>Tuple), std::get<1>(Tuple));<br>+<br>+  // Check reverse iteration.<br>+  ReverseIterate<bool>::value = true;<br>+  for (const auto &Tuple : zip(Set, ReversePtrs))<br>+    ASSERT_EQ(std::get<0>(<wbr>Tuple), std::get<1>(Tuple));<br>+}<br>+#endif<br><br><br>______________________________<wbr>_________________<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" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br></div></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div></div></div></div></div></blockquote></div><br></div>