[PATCH] D27454: Extract LaneBitmask into a separate type

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D27454#617500, @kparzysz wrote:

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


Ah that's why. It still feels wrong to put a codegen related class into llvm/Support. Maybe we should just not use the custom type in tablegen (you can still add safety checks like letting tablegen generate a `static_assert(std::is_same(LaneBitmask::Type, TypeUsedByTableGen));` to avoid accidental type mismatches between tablegen and libCodeGen. (A silly alternative would be to make sure LaneBitmask is a 100% header only implementation so we can include it into tablegen without needing to link libMC, but that seems brittle to me.)



================
Comment at: include/llvm/Support/LaneBitmask.h:42
+    typedef unsigned Type;
+    enum : unsigned { BitWidth = 8*sizeof(Type) };
+
----------------
kparzysz wrote:
> Just to explain the use of enum: using "static const unsigned" has in the past caused problems with Debug vs Release builds. Release builds treated the constant as literal, while Debug builds treated it as a constant object, thus requiring a definition (i.e. "const Type Class::Member = val"). Release builds, in turn, didn't like that definition, so there was no satisfactory way to keep both happy.
enums are fine (I think with `static const unsigned` we will get an extra entry in the rodata segment which seems superfluous for a purely compiletime constant).

The Release vs. Debug build issue rather sounds like a constant vs. non-constant expression issue to me. I would expect it to properly work in both as long as everything involved is marked constexpr or using basic types (also looking around there are certainly other headers in llvm using `static const type name =`.


================
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; }
----------------
kparzysz wrote:
> MatzeB wrote:
> > Do we actually need an ordering for bitmasks?
> Ordering is required for storing it in std::set, std::map, etc.
Sure I just couldn't come up with a sensible reason to put them into a set/map. Anyway I don't really care whether that operator exists or not.


================
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); }
----------------
kparzysz wrote:
> 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.
Oh indeed `enum : Type` was not my intention and `enum : LaneBitmask` of course doesn't work, of course we do not want implicit conversions.

Anyway if you mark the respective methods/constructors as `constexpr` I would expect something like this to work:
```
  static const LaneBitmask None = LaneBitmask(0);
  static const LaneBitmask All = ~None;
```


================
Comment at: include/llvm/Support/LaneBitmask.h:97
+  private:
+    explicit LaneBitmask(Type V) : Mask(V) {}
+
----------------
kparzysz wrote:
> 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.
I agree that we do not want implicit conversions here. I was thinking of an `explicit LaneBitmask(Type Mask) : Mask(Mask) {}` constructor.


================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:213
   // Any change at all?
-  if ((UsedLanes & ~PrevUsedLanes) == 0)
+  if ((UsedLanes & ~PrevUsedLanes).none())
     return;
----------------
kparzysz wrote:
> 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...  :)
I must have been tired writing the review. Of course it's not a union/intersection.

I would not like to overload operator- as it not clear whether that performs an integer subtraction with the underlying datatype or the "and not" operation.
The set theoretical name `difference()` feels unhelpful to me as well, so maybe a more colloquial `without()`.


Repository:
  rL LLVM

https://reviews.llvm.org/D27454