unorderedmap: adding missing constexpr

[romintomasetti]

This PR adds missing constexpr in Kokkos::UnorderedMap.

[masterleinad]

Why does it matter here to use if constexpr?

[romintomasetti]

Why does it matter here to use if constexpr?

@masterleinad What do you mean by here ?

[masterleinad]

@masterleinad What do you mean by here ?

It doesn't look like replacing if with if constexpr doesn't make any difference for the places you change in this pull request, does it?

[romintomasetti]

Good question!

I quickly drafted this code to check that with gcc-12:

#include <cstdio>
#include <cstdlib>
#include <type_traits>
#include <utility>

#ifndef CONSTEXPR_OR_NOT
    #error "CONSTEXPR_OR_NOT must be defined."
#endif

template <typename value_t>
class UnorderedMap
{
public:
    static constexpr bool is_set = !std::is_same_v<value_t, double>;

public:
    UnorderedMap(const size_t size)
    {
        array = new value_t[size];

        printf("This is a %s.\n", is_set ? "set" : "map");
    }

    ~UnorderedMap() { delete[] array; }

    template <typename T>
    void __attribute__((noinline)) insert(const size_t index, T&& value)
    {
        if CONSTEXPR_OR_NOT (!is_set)
        {
            printf("Inserting a new value at index %zu\n", index);
            array[index] = std::forward<value_t>(value);
        }
        else
        {
            printf("Not inserting a new value at index %zu\n", index);
        }
    }

    auto get(const size_t index) const { return array[index]; }

private:
    value_t* array;
};

int main()
{
    UnorderedMap<double> umap(5);
    UnorderedMap<int   > uset(5);

    umap.insert(0, double(4.2));
    uset.insert(0, int(42));

    if( ! umap.is_set) printf("Value is %.5f.\n", umap.get(0));
    if(   uset.is_set) printf("Value is %d.\n"  , uset.get(0));

    return EXIT_SUCCESS;
};

and compile with:

set -ex

g++-12 -Werror -std=c++17 -O0 -fverbose-asm -DCONSTEXPR_OR_NOT=constexpr test.cpp -S -o test_wi_constexpr.s
g++-12 -Werror -std=c++17 -O0 -fverbose-asm -DCONSTEXPR_OR_NOT=          test.cpp -S -o test_wo_constexpr.s

g++-12 -Werror -std=c++17 -O0 -fverbose-asm -DCONSTEXPR_OR_NOT=constexpr test.cpp -o test_wi_constexpr
g++-12 -Werror -std=c++17 -O0 -fverbose-asm -DCONSTEXPR_OR_NOT=          test.cpp -o test_wo_constexpr

./test_wi_constexpr
./test_wo_constexpr

Then, compare the assembly output.

With gcc-12 you're right that the compiler will always evaluate the if as if it was a constexpr if because it knows that is_set is constexpr, and so it makes this optimization for you under-the-hood.

However, I couldn't find anything in the standard that forces a compiler to do so. So my guess is that not all compilers would do that. Am I right ? (if so I suggest we go forward with this PR - though it's rather a style fix rather than a performance fix - at least for gcc-12)

Interesting read: https://www.learncpp.com/cpp-tutorial/constexpr-if-statements/.

EDIT: It seems that nvcc 12.2 does the same for device code (i.e. assume constexpr if when the statement is a constant expression).

[masterleinad]

If there is no measurable benefit, I'm not in favor of this pull request to reduce churn.

[romintomasetti]

@masterleinad I understand your point of view. It's true that there is no performance gain.

However, in light of the (IMO very good) SO answer https://stackoverflow.com/a/54545744, I still think it's best practice to write constexpr in this case.

[romintomasetti]

For the record - this PR extracts changes from #6584.

[dalg24]

@masterleinad I understand your point of view. It's true that there is no performance gain.

However, in light of the (IMO very good) SO answer https://stackoverflow.com/a/54545744, I still think it's best practice to write constexpr in this case.

I would agree if we were writing new code. But these changes are mostly unnecessary and you will find plenty of other guidelines that tell you "if it ain't broke, don't fix it"

[dalg24]

Avoid unrelated changes

[romintomasetti]

I thought it would be a related change... I reverted and simply added constexpr in the declaration of invalid_index.

[dalg24]

The PR description is what should defend what is in scope and what is not.
It says "add[ing] missing constexpr in UnorderedMap" and early versions of this PR mixed in more changes which does not help building a good rational for changing code.

The constexpr is "missing" as in we could/should add it? Is it clearly not a bug but how does the change improve things?
You need to pick your battles. If what is most important to you is that maps with const value type should not be insertable, then focus on that.

[dalg24]

The [[maybe_unused]] attribute on arguments of a function of a function whose implementation does not fit on one page is an example of "code smell", that is to say I am not convinced since is an improvement whatsoever.

[romintomasetti]

I was "forced" by the compiler to add [[maybe_unused]] because this PR guards some parts of the insert code with if constexpr (instead of if). Therefore, compiling with a void value type (i.e. making a set instead of a map) makes these code blocks guarded by if constexpr not instantiated, and so the compiler complains:

uho you have v argument that is unused

[romintomasetti]

(So yes, the addition of constexpr is not just a useless change, it actually changes how compilers behave - for good in this case IMO since it's now clearly written that v might be unused, and that's the case for a set)

[dalg24]

I understand what made you add the attribute. I am just saying that it is not clear to me that the code is better (as in more readable, more maintainable) now than before.

[nliber]

If we are going to do anything at all, it seems to me that the fundamental problem is that when the container is a set, the signature of insert should be

insert(key_type const&);

I know it is technically a breaking change, but can we just do that? That seems like a positive change for the public facing API. That being said, I don't know if that is even worth it, as it seems very unlikely that folks are calling this on a set with more than one specified parameter.

But that doesn't really address the disagreement here. The code with [[maybe_unused]] looks worse to me, because as a reader I have to reason out the circumstances that it is unused (and it doesn't help that is_set is a terrible name because as a reader I have to know that it refers to the type of the container and not referring to something like the state of the container being valid). And this seems like a lot of churn for a theoretical micro-optimization (one branch in a very complicated function that in practice gets optimized away). If that is really the concern, should we also be concerned about creating unused parameters (which also get optimized away in practice)?