Use enums for indexing bitset

[sethrj]

This refactors the Bitset class to index on enumerations rather than using integers. All the bitsets we care about have a fixed meaning for each one of the bits, and we will want to convert existing enums into bitsets: for example,

enum class MscModelSelection
{
    none,
    urban,  //!< Urban for all energies
    wentzelvi,  //!< Wentzel VI for all energies
    urban_wentzelvi,  //!< Urban below 100 MeV, Wentzel VI above
    size_
};

will become

enum class MscModelSelection
{
    none,
    urban,
    wentzelvi,
    size_
};

and

        if (options_.msc == MMS::urban || options_.msc == MMS::urban_wentzelvi)

will become

        if (options_.msc[MMS::urban])

This PR also flips the EnumArray template parameters to mirror the Array parameters (value type, then size).

[github-actions]

Test summary

 4 215 files   6 607 suites   15m 10s ⏱️
 1 691 tests  1 685 ✅  6 💤 0 ❌
23 240 runs  23 160 ✅ 80 💤 0 ❌

Results for commit 45eeb90.

♻️ This comment has been updated with latest results.

[pcanal]

Can we extend the intro (and then commit) with some additional information on the advantages of the change?

[esseivaju]

LGTM! Just a few minor comments

[pcanal]

Performance-wise, this is the crux of this functionality (setting might be rare but testing might not be).
Thus there is a shift operation plus a binary or operation for each test.
An alternative implementation is to use (not as easily enforceable from a C++ stand point )

enum class MscModelSelection
{
    none,
    urban = 1, 
    wentzelvi = 2, /* really 1  << 1 */
    somethingelse = 4, /* really 1 << 2 */
    size_
};

The Bitset associated with that now wouldn't have to do the shift operation at run-time (as long as there is less than 64 enum values).

[sethrj]

@pcanal Being constexpr, this should be optimized away. Since we define CELER_CONSTEXPR_FUNCTION as constexpr inline __attribute__((always_inline)) it even gets replaced by a scalar value in completely "unoptimized" code:
https://godbolt.org/z/eYehqebqa

So there's no penalty to indexing with i versus 2^i.

[pcanal]

constexpr is not a guarantee, it is a possibility that depends on all inputs to also be constexpr. Unlike the example you used, bit_pos_ is a data member and thus not guaranteed to be constexpr. bit_pos_ is in turn set from the parameter to operator[] and so options_.msc[MMS::urban] would indeed have the possibility of executing the shift at compile time but MscModelSelection m = ...; .... then somewhere else/indirect .... options_.msc[m] would not be able to. If only the former is intended to be used, we could/should/might add code to make the later fail to compile.

[sethrj]

Oh I see what you mean. Yes we do have some manual bit flags already (ORANGE VolumeRecord::Flags for instance). Unlike your enum, the Bitset can be extended to an arbitrary number of bits (though now that I think about it, perhaps we've overengineered it since I can't think of any case where we want more than 32 separate flags...)

Lol when compiling a test code that gets a bit from the Bitset, what's 1 line with -O1 becomes 58 lines with -O0 (on ARM with Apple Clang anyway), compared to 8 instructions with the manual mask approach.

enum class Foo
{
    a,
    b,
    c,
    size_
};

using Bitset = celeritas::EnumBitset<Foo>;

bool get_b(Bitset bs)
{
    return bs[Foo::b];
}

bool get_manual(short int const s)
{
    return s & 0x2;
}

So, the question is which do we value more: clarity and ease of use, or performance in debug mode?

[sethrj]

Actually I have a potentially easy fix for at least part of your problem... just store mask_ instead of bit_pos_.

[pcanal]

since I can't think of any case where we want more than 32 separate flags...

I agree that a solution that support 32 flags (or 64 with a uint64_t) will be sufficient.

[pcanal]

what's 1 line with -O1

For constexpr inputs or run-time inputs?

[sethrj]

Storing mask_ saved like one instruction in -O0, looking closer at the assembly the main overhead is stack pointer manipulation 🤷

This is for a constexpr enum access but run-time value being accessed: get_b above compiles down to get_manual.

Maybe we've approached this wrong: perhaps we just need an "enum bitset" with a single word... @esseivaju what circumstance would require a bitset with compile-time length > 32?

[esseivaju]

I can't foresee many situations where a length larger than 32bits would be needed. We briefly touched on that in the PR implementing the bitset, since N is a constexpr, N=1 reduces to a single word. So, I'm not sure what would be the benefit of supporting a single word only as it doesn't add that much complexity.

[pcanal]

This is for a constexpr enum access but run-time value being accessed:

The part that was surprising was what's 1 line with -O1. What is the one instructions that does both shift and test? (More likely I miss assumed what that was covering :) ).