[llvm] r289619 - [llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in codegen
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 7 14:12:54 PST 2017
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 <mailto: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 <mailto: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 <mailto: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 <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 <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 <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 <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 <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 <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 <mailto: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/a1836e6a/attachment.html>
More information about the llvm-commits
mailing list