[PATCH] D27454: Extract LaneBitmask into a separate type

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I'm not a big fan of all the boilerplate necessary when wrapping up something simple like an unsigned value.

However seeing the little places fixed here where we didn't correctly use the LaneBitmask typedef is convincing, so this is a good plan!

I have some comments though:

- Creating a new LaneBitmask.h header seems overkill (esp. in llvm/Support which is even imported into non-codegen parts of llvm). I would just add the definition at the beginning of MCRegisterInfo.h.
- Possibly add `constexpr` to all the members of LaneBitmask (though I'm not sure if we care about constexpr that much in llvm, it's used extremely rarely today)



================
Comment at: include/llvm/Support/LaneBitmask.h:9
+//===----------------------------------------------------------------------===//
+// A common definition of LaneBitmask for use in TableGen and CodeGen.
+//
----------------
Should use `/// \file` to tell doxygen about the file level comment.


================
Comment at: include/llvm/Support/LaneBitmask.h:36-37
+  struct LaneBitmask {
+    // When changing the underlying type, change the format string in
+    // the "porint" function as well.
+    typedef unsigned Type;
----------------
Maybe just move the print function up here, so you immediately see the matching format string.


================
Comment at: include/llvm/Support/LaneBitmask.h:40
+
+    LaneBitmask() : Mask(0) {}
+    bool operator== (LaneBitmask M) const { return Mask == M.Mask; }
----------------
Just use a member initializer instead of a constructor.


================
Comment at: include/llvm/Support/LaneBitmask.h:43
+    bool operator!= (LaneBitmask M) const { return Mask != M.Mask; }
+    bool operator< (LaneBitmask M)  const { return Mask < M.Mask; }
+    bool none() const { return Mask == 0; }
----------------
Do we actually need an ordering for bitmasks?


================
Comment at: include/llvm/Support/LaneBitmask.h:56-61
+    LaneBitmask operator<<(unsigned S) const {
+      return LaneBitmask(Mask << S);
+    }
+    LaneBitmask operator>>(unsigned S) const {
+      return LaneBitmask(Mask >> S);
+    }
----------------
Shifting seems like a very unnatural operator for a bitmask. It's only used as an encoding trick in the tablegen stuff, I don't think it makes any semantic sense as a lane bitmask. Therefore I'd rather force people to cast to LaneBitmask::Type and back for the shifting operations.


================
Comment at: include/llvm/Support/LaneBitmask.h:71-84
+    static LaneBitmask rol(LaneBitmask M, unsigned S) {
+      assert(S < 8*sizeof(Type));
+      if (S == 0)
+        return M;
+      Type V = M.getAsInteger();
+      return get((V << S) | (V >> (8*sizeof(Type) - S)));
+    }
----------------
Same comment as operator<<, operator>> applies here


================
Comment at: include/llvm/Support/LaneBitmask.h:88-89
+
+    static LaneBitmask getNone()    { return LaneBitmask(0); }
+    static LaneBitmask getAll()     { return ~LaneBitmask(0); }
+    static LaneBitmask get(Type V)  { return LaneBitmask(V); }
----------------
Maybe use

```
enum : Type {
  None = 0,
  All = ~(Type)0,
}
```
(or `static const Type None =` ...)
as that will make it obvious they are constants, will work in constant global initializers and is a few characters less typing.


================
Comment at: include/llvm/Support/LaneBitmask.h:97
+  private:
+    explicit LaneBitmask(Type V) : Mask(V) {}
+
----------------
Should we make this public and get rid of the `get(Type V)` function?


================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:213
   // Any change at all?
-  if ((UsedLanes & ~PrevUsedLanes) == 0)
+  if ((UsedLanes & ~PrevUsedLanes).none())
     return;
----------------
(A & ~B) is a pretty common operation, maybe introduce:

`LaneBitmask::common(LaneBitmask Other) { return *this & ~Other; }`
Possibly with an implicit bool cast operator so we can write:
`if (!UsedLanes.common(PrevUsedLanes)) return;`

(`union` may be another possible candidate for a name, but it feels a bit out of place as the type is called a mask and not a set)

We can leave this addition to a followup patch of course.



================
Comment at: lib/CodeGen/RegisterPressure.cpp:43
                                 LaneBitmask PrevMask, LaneBitmask NewMask) {
-  assert((NewMask & !PrevMask) == 0 && "Must not add bits");
-  if (NewMask != 0 || PrevMask == 0)
+  //assert((NewMask & !PrevMask) == 0 && "Must not add bits");
+  if (!NewMask.none() || PrevMask.none())
----------------
That sneaked in here and should probably be discussed in a separate patch (even though this appears to be a real problem).


================
Comment at: lib/CodeGen/TargetRegisterInfo.cpp:130-134
 Printable PrintLaneMask(LaneBitmask LaneMask) {
   return Printable([LaneMask](raw_ostream &OS) {
-    OS << format("%08X", LaneMask);
+    LaneMask.print(OS);
   });
 }
----------------
Should probably move that function declaration/definition next to the LaneBitmask definition.


================
Comment at: utils/TableGen/CodeGenRegisters.cpp:54
 CodeGenSubRegIndex::CodeGenSubRegIndex(Record *R, unsigned Enum)
-  : TheDef(R), EnumValue(Enum), LaneMask(0), AllSuperRegsCovered(true) {
+  : TheDef(R), EnumValue(Enum), LaneMask(), AllSuperRegsCovered(true) {
   Name = R->getName();
----------------
`LaneMask()` can be removed.


================
Comment at: utils/TableGen/CodeGenRegisters.cpp:65
   : TheDef(nullptr), Name(N), Namespace(Nspace), Size(-1), Offset(-1),
-    EnumValue(Enum), LaneMask(0), AllSuperRegsCovered(true) {
+    EnumValue(Enum), LaneMask(), AllSuperRegsCovered(true) {
 }
----------------
dito.


================
Comment at: utils/TableGen/CodeGenRegisters.cpp:682
     EnumValue(-1),
-    LaneMask(0) {
+    LaneMask() {
   // Rename anonymous register classes.
----------------
dito.


Repository:
  rL LLVM

https://reviews.llvm.org/D27454