[PATCH] D84717: [ADT][BitVector][NFC] Merge find_first_in() / find_first_unset_in()

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM

In D84717#2178016 <https://reviews.llvm.org/D84717#2178016>, @baziotis wrote:

> One proposal by a friend of mine, GeorgeLS <https://github.com/GeorgeLS>, is that we can make a wrapper in which instead of passing a `bool`, we pass a `BitWord`.
> And then we XOR with this `BitWord` every `BitWord` we read from `Bits`. So, the passed `BitWord` acts conditionally. If we want the "unset" version, we pass `~0`.
> If we want the "set" version, we pass `0`.

An optimizing compiler might already do this if it thinks it is profitable. That is, InstCombine

  if (!Set)
    Copy = ~Copy;

to

  Mask = Set ? 0 : -1;
  Copy ^= Mask;

and hoist Mask out of the loop.

However, unless you show that there is a relevant performance difference, I don't have a preference for either version.



================
Comment at: llvm/include/llvm/ADT/BitVector.h:272
   int find_first_unset_in(unsigned Begin, unsigned End) const {
-    assert(Begin <= End && End <= Size);
-    if (Begin == End)
-      return -1;
-
-    unsigned FirstWord = Begin / BITWORD_SIZE;
-    unsigned LastWord = (End - 1) / BITWORD_SIZE;
-
-    // Check subsequent words.
-    for (unsigned i = FirstWord; i <= LastWord; ++i) {
-      BitWord Copy = Bits[i];
-
-      if (i == FirstWord) {
-        unsigned FirstBit = Begin % BITWORD_SIZE;
-        Copy |= maskTrailingOnes<BitWord>(FirstBit);
-      }
-
-      if (i == LastWord) {
-        unsigned LastBit = (End - 1) % BITWORD_SIZE;
-        Copy |= maskTrailingZeros<BitWord>(LastBit + 1);
-      }
-      if (Copy != ~BitWord(0)) {
-        unsigned Result = i * BITWORD_SIZE + countTrailingOnes(Copy);
-        return Result < size() ? Result : -1;
-      }
-    }
-    return -1;
+    return find_first_in(Begin, End, /* set = */ false);
   }
----------------
`set` -> `Set`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84717/new/

https://reviews.llvm.org/D84717