[PATCH] D27454: Extract LaneBitmask into a separate type

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz marked 6 inline comments as done.
kparzysz added a comment.

In https://reviews.llvm.org/D27454#616756, @MatzeB wrote:

> - 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.


I added a separate file because LaneBitmask is used in both TableGen and CodeGen.  Otherwise I wouldn't bother.



================
Comment at: include/llvm/Support/LaneBitmask.h:40
+
+    LaneBitmask() : Mask(0) {}
+    bool operator== (LaneBitmask M) const { return Mask == M.Mask; }
----------------
MatzeB wrote:
> Just use a member initializer instead of a constructor.
I did it and got errors: "error: constructor for 'llvm::CodeGenRegisterClass' must explicitly initialize the member 'LaneMask' which does not have a default 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; }
----------------
MatzeB wrote:
> Do we actually need an ordering for bitmasks?
Ordering is required for storing it in std::set, std::map, etc.


================
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); }
----------------
MatzeB wrote:
> 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.
This would require allowing implicit conversion from Type, at least if we want to save on the number of characters.  I specifically wanted to avoid such conversions, because it makes it easy to make a hard-to-find mistake in the code.  As per the example from the main comment, a person may write "LaneBitmask M = ~0u" by the force of habit, and that code will compile and run correctly at the moment.  However it may fail if the lane masks later on become 64-bit integers, since ~0u will only bit-negate a 32-bit value.


================
Comment at: include/llvm/Support/LaneBitmask.h:97
+  private:
+    explicit LaneBitmask(Type V) : Mask(V) {}
+
----------------
MatzeB wrote:
> Should we make this public and get rid of the `get(Type V)` function?
This is related to the comment above regarding the implicit conversion from Type.  Having to write "get" may serve as a reminder that constructing lane masks needs to be done with "type switchability" in mind.


================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:213
   // Any change at all?
-  if ((UsedLanes & ~PrevUsedLanes) == 0)
+  if ((UsedLanes & ~PrevUsedLanes).none())
     return;
----------------
MatzeB wrote:
> (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.
> 
(A & ~B) is actually more like A - B (remove B's bits from A).  It does occur a few times, so adding a function for it may be a good idea.

Also, `union` may not work as a name, it being a keyword and all...  :)


Repository:
  rL LLVM

https://reviews.llvm.org/D27454