[llvm] r289619 - [llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in codegen
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 7 18:29:20 PST 2017
Thank you, couldn't find the commit thread till you replied :)
On Tue, Mar 7, 2017 at 2:12 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
> r297234 should fix it.
>
> On 2017-Mar-07, at 11:25, Duncan P. N. Exon Smith <dexonsmith at apple.com>
> wrote:
>
> [DannyB replied to the old phab review, but let's move the discussion to
> the commit.]
>
> My guess is that find() should be updated to return end().
>
> On 2017-Mar-07, at 10:57, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> (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)
>>
>>
>> On Tue, Mar 7, 2017 at 10:43 AM, Daniel Berlin <dberlin at dberlin.org>
>> wrote:
>>
>>> So, this change breaks smallptrset's find function when it is on.
>>>
>>> Trivial test case:
>>> SmallPtrSet<int *, 8> IntSet;
>>> int A;
>>> int *B = &A;
>>> EXPECT_FALSE(IntSet.find(B) == IntSet.end());
>>>
>>> Because you've made the end iterator point somewhere else, this will no
>>> longer work.
>>>
>>> Please fix.
>>>
>>>
>
> On 2016-Dec-13, at 16:15, Mandeep Singh Grang via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: mgrang
> Date: Tue Dec 13 18:15:57 2016
> New Revision: 289619
>
> URL: http://llvm.org/viewvc/llvm-project?rev=289619&view=rev
> Log:
> [llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in
> codegen
>
> Summary:
> Given a flag (-mllvm -reverse-iterate) this patch will enable iteration of
> SmallPtrSet in reverse order.
> The idea is to compile the same source with and without this flag and
> expect the code to not change.
> If there is a difference in codegen then it would mean that the codegen is
> sensitive to the iteration order of SmallPtrSet.
> This is enabled only with LLVM_ENABLE_ABI_BREAKING_CHECKS.
>
> Reviewers: chandlerc, dexonsmith, mehdi_amini
>
> Subscribers: mgorny, emaste, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D26718
>
> Added:
> llvm/trunk/unittests/ADT/ReverseIterationTest.cpp
> Modified:
> llvm/trunk/include/llvm/ADT/SmallPtrSet.h
> llvm/trunk/lib/Support/CommandLine.cpp
> llvm/trunk/unittests/ADT/CMakeLists.txt
>
> Modified: llvm/trunk/include/llvm/ADT/SmallPtrSet.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/ADT/SmallPtrSet.h?rev=289619&r1=289618&r2=289619&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/ADT/SmallPtrSet.h (original)
> +++ llvm/trunk/include/llvm/ADT/SmallPtrSet.h Tue Dec 13 18:15:57 2016
> @@ -15,6 +15,7 @@
> #ifndef LLVM_ADT_SMALLPTRSET_H
> #define LLVM_ADT_SMALLPTRSET_H
>
> +#include "llvm/Config/abi-breaking.h"
> #include "llvm/Support/Compiler.h"
> #include "llvm/Support/PointerLikeTypeTraits.h"
> #include <cassert>
> @@ -25,6 +26,13 @@
> #include <iterator>
> #include <utility>
>
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> +namespace llvm {
> +template <class T = void> struct ReverseIterate { static bool value; };
> +template <class T> bool ReverseIterate<T>::value = true;
> +}
> +#endif
> +
> namespace llvm {
>
> /// SmallPtrSetImplBase - This is the common code shared among all the
> @@ -206,6 +214,12 @@ protected:
> public:
> explicit SmallPtrSetIteratorImpl(const void *const *BP, const void*const
> *E)
> : Bucket(BP), End(E) {
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + if (ReverseIterate<bool>::value) {
> + RetreatIfNotValid();
> + return;
> + }
> +#endif
> AdvanceIfNotValid();
> }
>
> @@ -227,6 +241,17 @@ protected:
> *Bucket == SmallPtrSetImplBase::getTombstoneMarker()))
> ++Bucket;
> }
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + void RetreatIfNotValid() {
> + --Bucket;
> + assert(Bucket <= End);
> + while (Bucket != End &&
> + (*Bucket == SmallPtrSetImplBase::getEmptyMarker() ||
> + *Bucket == SmallPtrSetImplBase::getTombstoneMarker())) {
> + --Bucket;
> + }
> + }
> +#endif
> };
>
> /// SmallPtrSetIterator - This implements a const_iterator for SmallPtrSet.
> @@ -252,13 +277,27 @@ public:
> }
>
> inline SmallPtrSetIterator& operator++() { // Preincrement
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + if (ReverseIterate<bool>::value) {
> + RetreatIfNotValid();
> + return *this;
> + }
> +#endif
> ++Bucket;
> AdvanceIfNotValid();
> return *this;
> }
>
> SmallPtrSetIterator operator++(int) { // Postincrement
> - SmallPtrSetIterator tmp = *this; ++*this; return tmp;
> + SmallPtrSetIterator tmp = *this;
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + if (ReverseIterate<bool>::value) {
> + --*this;
> + return tmp;
> + }
> +#endif
> + ++*this;
> + return tmp;
> }
> };
>
> @@ -343,9 +382,22 @@ public:
> }
>
> inline iterator begin() const {
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + if (ReverseIterate<bool>::value)
> + return endPtr();
> +#endif
> return iterator(CurArray, EndPointer());
> }
> inline iterator end() const {
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> + if (ReverseIterate<bool>::value)
> + return iterator(CurArray, CurArray);
> +#endif
> + return endPtr();
> + }
> +
> +private:
> + inline iterator endPtr() const {
> const void *const *End = EndPointer();
> return iterator(End, End);
> }
>
> Modified: llvm/trunk/lib/Support/CommandLine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Support/CommandLine.cpp?rev=289619&r1=289618&r2=289619&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Support/CommandLine.cpp (original)
> +++ llvm/trunk/lib/Support/CommandLine.cpp Tue Dec 13 18:15:57 2016
> @@ -45,6 +45,17 @@ using namespace cl;
>
> #define DEBUG_TYPE "commandline"
>
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> +namespace llvm {
> +// If LLVM_ENABLE_ABI_BREAKING_CHECKS is set the flag -mllvm
> -reverse-iterate
> +// can be used to toggle forward/reverse iteration of unordered
> containers.
> +// This will help uncover differences in codegen caused due to undefined
> +// iteration order.
> +static cl::opt<bool, true> ReverseIteration("reverse-iterate",
> + cl::location(ReverseIterate<bool>::value), cl::init(true));
> +}
> +#endif
> +
> //===-------------------------------------------------------
> ---------------===//
> // Template instantiations and anchors.
> //
>
> Modified: llvm/trunk/unittests/ADT/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/
> ADT/CMakeLists.txt?rev=289619&r1=289618&r2=289619&view=diff
> ============================================================
> ==================
> --- llvm/trunk/unittests/ADT/CMakeLists.txt (original)
> +++ llvm/trunk/unittests/ADT/CMakeLists.txt Tue Dec 13 18:15:57 2016
> @@ -41,6 +41,7 @@ set(ADTSources
> PostOrderIteratorTest.cpp
> PriorityWorklistTest.cpp
> RangeAdapterTest.cpp
> + ReverseIterationTest.cpp
> SCCIteratorTest.cpp
> STLExtrasTest.cpp
> ScopeExitTest.cpp
>
> Added: llvm/trunk/unittests/ADT/ReverseIterationTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/
> ADT/ReverseIterationTest.cpp?rev=289619&view=auto
> ============================================================
> ==================
> --- llvm/trunk/unittests/ADT/ReverseIterationTest.cpp (added)
> +++ llvm/trunk/unittests/ADT/ReverseIterationTest.cpp Tue Dec 13 18:15:57
> 2016
> @@ -0,0 +1,39 @@
> +//===- llvm/unittest/ADT/ReverseIterationTest.cpp
> ------------------------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===------------------------------------------------------
> ----------------===//
> +//
> +// ReverseIteration unit tests.
> +//
> +//===------------------------------------------------------
> ----------------===//
> +
> +#include "gtest/gtest.h"
> +#include "llvm/ADT/SmallPtrSet.h"
> +
> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
> +using namespace llvm;
> +
> +TEST(ReverseIterationTest, SmallPtrSetTest) {
> +
> + SmallPtrSet<void*, 4> Set;
> + void *Ptrs[] = { (void*)0x1, (void*)0x2, (void*)0x3, (void*)0x4 };
> + void *ReversePtrs[] = { (void*)0x4, (void*)0x3, (void*)0x2, (void*)0x1
> };
> +
> + for (auto *Ptr: Ptrs)
> + Set.insert(Ptr);
> +
> + // Check forward iteration.
> + ReverseIterate<bool>::value = false;
> + for (const auto &Tuple : zip(Set, Ptrs))
> + ASSERT_EQ(std::get<0>(Tuple), std::get<1>(Tuple));
> +
> + // Check reverse iteration.
> + ReverseIterate<bool>::value = true;
> + for (const auto &Tuple : zip(Set, ReversePtrs))
> + ASSERT_EQ(std::get<0>(Tuple), std::get<1>(Tuple));
> +}
> +#endif
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170307/4c89120a/attachment.html>
More information about the llvm-commits
mailing list