[llvm] 887efa5 - Fix DenseMap iterator asserts when shouldReverseIterate==true
Craig Topper via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 21 00:41:03 PDT 2020
This was causing some failures of the assert in operator*/operator-> in the
iterator for me. I believe I've fixed the issue
in 2cf3c033f3a1d4c2c36a94a80b7ba6bcba1e8711
~Craig
On Mon, Apr 20, 2020 at 5:31 PM Chris Bieneman via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> Author: Chris Bieneman
> Date: 2020-04-20T19:31:32-05:00
> New Revision: 887efa51c1e0e43ca684ed78b92dbc3a0720881b
>
> URL:
> https://github.com/llvm/llvm-project/commit/887efa51c1e0e43ca684ed78b92dbc3a0720881b
> DIFF:
> https://github.com/llvm/llvm-project/commit/887efa51c1e0e43ca684ed78b92dbc3a0720881b.diff
>
> LOG: Fix DenseMap iterator asserts when shouldReverseIterate==true
>
> This patch gets the asserts working correctly when
> LLVM_REVERSE_ITERATION=On by fixing the iterators returned by the
> DenseMap::find* methods so that they return well-formed iterators that work
> with reverse iteration, and satisfy the assertions.
>
> Added:
>
>
> Modified:
> llvm/include/llvm/ADT/DenseMap.h
>
> Removed:
>
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/ADT/DenseMap.h
> b/llvm/include/llvm/ADT/DenseMap.h
> index f453ce539e2b..ff9dab15291f 100644
> --- a/llvm/include/llvm/ADT/DenseMap.h
> +++ b/llvm/include/llvm/ADT/DenseMap.h
> @@ -149,13 +149,19 @@ class DenseMapBase : public DebugEpochBase {
> iterator find(const_arg_type_t<KeyT> Val) {
> BucketT *TheBucket;
> if (LookupBucketFor(Val, TheBucket))
> - return makeIterator(TheBucket, getBucketsEnd(), *this, true);
> + return makeIterator(TheBucket,
> + shouldReverseIterate<KeyT>() ? getBuckets()
> + : getBucketsEnd(),
> + *this, true);
> return end();
> }
> const_iterator find(const_arg_type_t<KeyT> Val) const {
> const BucketT *TheBucket;
> if (LookupBucketFor(Val, TheBucket))
> - return makeConstIterator(TheBucket, getBucketsEnd(), *this, true);
> + return makeConstIterator(TheBucket,
> + shouldReverseIterate<KeyT>() ? getBuckets()
> + :
> getBucketsEnd(),
> + *this, true);
> return end();
> }
>
> @@ -168,14 +174,20 @@ class DenseMapBase : public DebugEpochBase {
> iterator find_as(const LookupKeyT &Val) {
> BucketT *TheBucket;
> if (LookupBucketFor(Val, TheBucket))
> - return makeIterator(TheBucket, getBucketsEnd(), *this, true);
> + return makeIterator(TheBucket,
> + shouldReverseIterate<KeyT>() ? getBuckets()
> + : getBucketsEnd(),
> + *this, true);
> return end();
> }
> template<class LookupKeyT>
> const_iterator find_as(const LookupKeyT &Val) const {
> const BucketT *TheBucket;
> if (LookupBucketFor(Val, TheBucket))
> - return makeConstIterator(TheBucket, getBucketsEnd(), *this, true);
> + return makeConstIterator(TheBucket,
> + shouldReverseIterate<KeyT>() ? getBuckets()
> + :
> getBucketsEnd(),
> + *this, true);
> return end();
> }
>
> @@ -208,17 +220,17 @@ class DenseMapBase : public DebugEpochBase {
> template <typename... Ts>
> std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&... Args) {
> BucketT *TheBucket;
> + BucketT *EndBucket =
> + shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
> if (LookupBucketFor(Key, TheBucket))
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - false); // Already in map.
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this,
> true),
> + false); // Already in map.
>
> // Otherwise, insert the new element.
> TheBucket =
> InsertIntoBucket(TheBucket, std::move(Key),
> std::forward<Ts>(Args)...);
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - true);
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
> + true);
> }
>
> // Inserts key,value pair into the map if the key isn't already in the
> map.
> @@ -227,16 +239,16 @@ class DenseMapBase : public DebugEpochBase {
> template <typename... Ts>
> std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&... Args) {
> BucketT *TheBucket;
> + BucketT *EndBucket =
> + shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
> if (LookupBucketFor(Key, TheBucket))
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - false); // Already in map.
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this,
> true),
> + false); // Already in map.
>
> // Otherwise, insert the new element.
> TheBucket = InsertIntoBucket(TheBucket, Key,
> std::forward<Ts>(Args)...);
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - true);
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
> + true);
> }
>
> /// Alternate version of insert() which allows a
> diff erent, and possibly
> @@ -248,17 +260,17 @@ class DenseMapBase : public DebugEpochBase {
> std::pair<iterator, bool> insert_as(std::pair<KeyT, ValueT> &&KV,
> const LookupKeyT &Val) {
> BucketT *TheBucket;
> + BucketT *EndBucket =
> + shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
> if (LookupBucketFor(Val, TheBucket))
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - false); // Already in map.
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this,
> true),
> + false); // Already in map.
>
> // Otherwise, insert the new element.
> TheBucket = InsertIntoBucketWithLookup(TheBucket, std::move(KV.first),
> std::move(KV.second), Val);
> - return std::make_pair(
> - makeIterator(TheBucket, getBucketsEnd(), *this, true),
> - true);
> + return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
> + true);
> }
>
> /// insert - Range insertion of pairs.
> @@ -1200,20 +1212,16 @@ class DenseMapIterator :
> DebugEpochBase::HandleBase {
>
> reference operator*() const {
> assert(isHandleInSync() && "invalid iterator access!");
> - if (shouldReverseIterate<KeyT>()) {
> - assert(Ptr != &End[-1] && "dereferencing end() iterator");
> - return Ptr[-1];
> - }
> assert(Ptr != End && "dereferencing end() iterator");
> + if (shouldReverseIterate<KeyT>())
> + return Ptr[-1];
> return *Ptr;
> }
> pointer operator->() const {
> assert(isHandleInSync() && "invalid iterator access!");
> - if (shouldReverseIterate<KeyT>()) {
> - assert(Ptr != &End[-1] && "dereferencing end() iterator");
> - return &(Ptr[-1]);
> - }
> assert(Ptr != End && "dereferencing end() iterator");
> + if (shouldReverseIterate<KeyT>())
> + return &(Ptr[-1]);
> return Ptr;
> }
>
> @@ -1234,13 +1242,12 @@ class DenseMapIterator :
> DebugEpochBase::HandleBase {
>
> inline DenseMapIterator& operator++() { // Preincrement
> assert(isHandleInSync() && "invalid iterator access!");
> + assert(Ptr != End && "incrementing end() iterator");
> if (shouldReverseIterate<KeyT>()) {
> - assert(Ptr != &End[-1] && "dereferencing end() iterator");
> --Ptr;
> RetreatPastEmptyBuckets();
> return *this;
> }
> - assert(Ptr != End && "incrementing end() iterator");
> ++Ptr;
> AdvancePastEmptyBuckets();
> return *this;
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://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/20200421/7da039c8/attachment.html>
More information about the llvm-commits
mailing list