[PATCH] D27454: Extract LaneBitmask into a separate type

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz marked an inline comment as done.
kparzysz added a comment.

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

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


I have done that, but I had serious reservations about it.  I think it makes the tablegen code more error prone.  It may look unnatural to have LaneBitmask definition in Support, but it being used by two different layers justifies it, in my opinion.



================
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:
> 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;
> ```
I kept the get... functions in the end.  I wanted the None/All to be nested in the LaneBitmask namespace. Declaring them outside created a conflict with "None" from Optional, and declaring them inside of LaneBitmask was rejected by the compiler, since the LaneBitmask type was not yet complete (until the closing bracket).

I made all applicable members "constexpr".


================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:213
   // Any change at all?
-  if ((UsedLanes & ~PrevUsedLanes) == 0)
+  if ((UsedLanes & ~PrevUsedLanes).none())
     return;
----------------
MatzeB wrote:
> 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()`.
I left it out from this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D27454