[llvm] 2c84b92 - Fix assertion in SmallDenseMap constructor with reserve from non-power-of-2 buckets count

[David Blaikie via llvm-commits]

Sorry, got the wrong author on this. Should've been @ivafanas as per
the Phab review.

On Mon, Jul 25, 2022 at 10:09 AM David Blaikie via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Vladislav Dzhidzhoev
> Date: 2022-07-25T17:09:44Z
> New Revision: 2c84b92346bc45a88f92d781e30daa8490049c4d
>
> URL: https://github.com/llvm/llvm-project/commit/2c84b92346bc45a88f92d781e30daa8490049c4d
> DIFF: https://github.com/llvm/llvm-project/commit/2c84b92346bc45a88f92d781e30daa8490049c4d.diff
>
> LOG: Fix assertion in SmallDenseMap constructor with reserve from non-power-of-2 buckets count
>
> `SmallDenseMap` constructor with reserve gets an arbitrary `NumInitBuckets` value and passes it below to `init` method.
>
> If `NumInitBuckets` is greater then `InlineBuckets`, then `SmallDenseMap` initializes to large representation passing `NumInitBuckets` below to `DenseMap` initialization. `DenseMap::initEmpty` method asserts that initial buckets count must be a power of 2.
>
> Proposed solution is to update `NumInitBuckets` value in `SmallDenseMap` constructor till the next power of 2. It should satisfy both `DenseMap` preconditions and required minimum buckets count for reservation.
>
> Reviewed By: atrick
>
> Differential Revision: https://reviews.llvm.org/D129825
>
> Added:
>
>
> Modified:
>     llvm/include/llvm/ADT/DenseMap.h
>     llvm/unittests/ADT/DenseMapTest.cpp
>
> Removed:
>
>
>
> ################################################################################
> diff  --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
> index c14414c46419c..a6d19d342822f 100644
> --- a/llvm/include/llvm/ADT/DenseMap.h
> +++ b/llvm/include/llvm/ADT/DenseMap.h
> @@ -907,6 +907,8 @@ class SmallDenseMap
>
>  public:
>    explicit SmallDenseMap(unsigned NumInitBuckets = 0) {
> +    if (NumInitBuckets > InlineBuckets)
> +      NumInitBuckets = NextPowerOf2(NumInitBuckets - 1);
>      init(NumInitBuckets);
>    }
>
>
> diff  --git a/llvm/unittests/ADT/DenseMapTest.cpp b/llvm/unittests/ADT/DenseMapTest.cpp
> index 4dd314c5c9019..619ddb05f269f 100644
> --- a/llvm/unittests/ADT/DenseMapTest.cpp
> +++ b/llvm/unittests/ADT/DenseMapTest.cpp
> @@ -607,6 +607,15 @@ TEST(DenseMapCustomTest, LargeSmallDenseMapCompaction) {
>    EXPECT_TRUE(map.find(0) == map.end());
>  }
>
> +TEST(DenseMapCustomTest, SmallDenseMapWithNumBucketsNonPowerOf2) {
> +  // Is not power of 2.
> +  const unsigned NumInitBuckets = 33;
> +  // Power of 2 less then NumInitBuckets.
> +  constexpr unsigned InlineBuckets = 4;
> +  // Constructor should not trigger assert.
> +  SmallDenseMap<int, int, InlineBuckets> map(NumInitBuckets);
> +}
> +
>  TEST(DenseMapCustomTest, TryEmplaceTest) {
>    DenseMap<int, std::unique_ptr<int>> Map;
>    std::unique_ptr<int> P(new int(2));
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits