[PATCH] D27454: Extract LaneBitmask into a separate type

[Matthias Braun via Phabricator via llvm-commits]

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

LGTM with the declaration moved to an MC header. If you want to pursue the direction of having a MC/LaneBitmask.h with a header only implementation and including that in TableGen go for it (I'm fine with that as well with the solution currently here).

Some more nitpicks/suggestions below but nothing that should stop the patch.



================
Comment at: include/llvm/MC/MCRegisterInfo.h:29
+
+// A lane mask is a bitmask representing the covering of a register with
+// sub-registers.
----------------
Should use `///`


================
Comment at: include/llvm/MC/MCRegisterInfo.h:52-55
+  void print(raw_ostream &OS) const {
+    // Make sure this format is correct for the underlying type.
+    OS << format("%08X", Mask);
+  }
----------------
One slightly annoying thing here is that we need to add `#include "raw_ostream.h"` and `#include "Format.h"` to the header. Would it make sense to maybe not add the print() function to LaneBitmask at all, just declaring Printable PrintLaneMask() here and implementing it in MCRegisterInfo.cpp and using something like `static_assert(sizeof(LaneBitmask::Type) == 4)` (or alternatively leaving the format string but not the actual call to format() in the LaneBitmask struct).


================
Comment at: include/llvm/MC/MCRegisterInfo.h:57
+
+  constexpr LaneBitmask() : Mask(0) {}
+  explicit constexpr LaneBitmask(Type V) : Mask(V) {}
----------------
The `: Mask(0)` is unnecessary.


================
Comment at: include/llvm/MC/MCRegisterInfo.h:63-64
+  constexpr bool operator< (LaneBitmask M)  const { return Mask < M.Mask; }
+  constexpr bool none() const { return Mask == 0; }
+  constexpr bool all()  const { return ~Mask == 0; }
+
----------------
Looking at the users `!X.none()` seems to be common enough that it warrants a `bool any() const { return Mask != 0; }` maybe even a `operator bool() const { return Mask != 0; }`.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:616
+  OS << "LaneBitmask(0x";
+  // Keep this format in sync with that in LaneBitmask::print.
+  OS << format("%08X", Val);
----------------
You could add a `static_assert(sizeof(LaneBitmaskType) == 4)` to be safe.


Repository:
  rL LLVM

https://reviews.llvm.org/D27454