The uses of lane masks are not consistent in terms of what type they use: some use LaneBitmask (which is defined in more than one place), others use plain unsigned. This patch defines LaneBitmask as a class with the underlying type of unsigned, and changes all uses of lane masks to use this class. The class is intended to avoid any overhead compared to using an integral type directly. It specifically avoids implicit conversions from/to integral types to avoid potential errors when changing the underlying type. For example, a typical initialization of a "full" mask was "LaneMask = ~0u", which would result in a value of 0x00000000FFFFFFFF if the type was extended to uint64_t.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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)
include/llvm/Support/LaneBitmask.h | ||
---|---|---|
9 | Should use /// \file to tell doxygen about the file level comment. | |
36–37 | Maybe just move the print function up here, so you immediately see the matching format string. | |
40 | Just use a member initializer instead of a constructor. | |
43 | Do we actually need an ordering for bitmasks? | |
56–61 | 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. | |
71–84 | Same comment as operator<<, operator>> applies here | |
88–89 | Maybe use enum : Type { None = 0, All = ~(Type)0, } (or static const Type None = ...) | |
97 | Should we make this public and get rid of the get(Type V) function? | |
lib/CodeGen/DetectDeadLanes.cpp | ||
213 | (A & ~B) is a pretty common operation, maybe introduce: LaneBitmask::common(LaneBitmask Other) { return *this & ~Other; } (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. | |
lib/CodeGen/RegisterPressure.cpp | ||
43 | That sneaked in here and should probably be discussed in a separate patch (even though this appears to be a real problem). | |
lib/CodeGen/TargetRegisterInfo.cpp | ||
130–134 | Should probably move that function declaration/definition next to the LaneBitmask definition. | |
utils/TableGen/CodeGenRegisters.cpp | ||
54 | LaneMask() can be removed. | |
65 | dito. | |
682 | dito. |
I added a separate file because LaneBitmask is used in both TableGen and CodeGen. Otherwise I wouldn't bother.
include/llvm/Support/LaneBitmask.h | ||
---|---|---|
40 | I did it and got errors: "error: constructor for 'llvm::CodeGenRegisterClass' must explicitly initialize the member 'LaneMask' which does not have a default constructor". | |
43 | Ordering is required for storing it in std::set, std::map, etc. | |
88–89 | 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. | |
97 | 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. | |
lib/CodeGen/DetectDeadLanes.cpp | ||
213 | (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... :) |
include/llvm/Support/LaneBitmask.h | ||
---|---|---|
43 | 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. |
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.)
include/llvm/Support/LaneBitmask.h | ||
---|---|---|
43 | 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 =. | |
43 | 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. | |
88–89 | 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; | |
97 | I agree that we do not want implicit conversions here. I was thinking of an explicit LaneBitmask(Type Mask) : Mask(Mask) {} constructor. | |
lib/CodeGen/DetectDeadLanes.cpp | ||
213 | 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. |
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.
include/llvm/Support/LaneBitmask.h | ||
---|---|---|
88–89 | 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". | |
lib/CodeGen/DetectDeadLanes.cpp | ||
213 | I left it out from this patch. |
Removed LaneBitmask.h, merged it into MCRegisterInfo.h.
Used a separate type for lane masks in TableGen.
TableGen includes CodeGen/MachineValueType.h. Could we have LaneBitmask.h in CodeGen and include it here as well?
Ah I didn't notice that file before, but that looks like the "100% header only implementation" I mentioned as an alternative earlier. Maybe the idea isn't so silly and we should just do it with LaneBitmask as well, at least it is all implemented in the header at the moment anyway.
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.
include/llvm/MC/MCRegisterInfo.h | ||
---|---|---|
27 | Should use /// | |
50–53 | 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). | |
55 | The : Mask(0) is unnecessary. | |
61–62 | 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; }. | |
utils/TableGen/RegisterInfoEmitter.cpp | ||
616 | You could add a static_assert(sizeof(LaneBitmaskType) == 4) to be safe. |
include/llvm/MC/MCRegisterInfo.h | ||
---|---|---|
61–62 | I will add it in a subsequent patch. |
Should use ///