[mlir] Add ODS support for enum attributes with grouped bit cases

[jfurtek]

Summary

This diff modifies the tablegen specification and code generation for
BitEnumAttr attributes in MLIR Operation Definition Specification (ODS) files.
Specifically:

• there is a new tablegen class for "none" values (i.e. no bits set)
• single-bit enum cases are specified via bit index (i.e. [0, 31]) instead of the resulting enum integer value
• there is a new tablegen class to represent a "grouped" bitwise OR of other enum values

This diff is intended as an initial step towards improving "fastmath"
optimization support in MLIR, to allow more precise control of whether certain
floating point optimizations are applied in MLIR passes. "Fast" math options
for floating point MLIR operations would (following subsequent RFC and
discussion) be specified by using the improved enum bit support in this diff.
For example, a "fast" enum value would act as an alias for a group of other
cases (e.g. finite-math-only, no-signed-zeros, etc.), in a way that is similar
to support in C/C++ compilers (clang, gcc).

[jpienaar]

Nice

mlir/tools/mlir-tblgen/EnumsGen.cpp

197

Where else is this verified on the attribute?

[rriddle]

mlir/docs/OpDefinitions.md

1405–1406

Why an empty string and not something like an assert? Do we need to support an invalid enum?

mlir/include/mlir/IR/OpBase.td

1332–1334

1339–1340

1344–1348

Is this wrapped at 80?

[Mogball]

mlir/include/mlir/IR/OpBase.td

1325–1326

I would prefer leaving the name as BitEnumAttrCase, since Base feels more like an implementation detail (which it is here, but enum attributes will need a list<BitEnumAttrCaseBase> which feels weird).

[rdzhabarov]

mlir/docs/OpDefinitions.md

1344

nit: def Bit0

1407–1411

curious, why there is a change from
'1u & val' to 1u == (1u & val).

Same below.

[jfurtek]

[mlir] Add ODS support for enum attributes with grouped bit cases (update)

• Update handling of invalid bits in bit enum attributes
• Change base class name for bit enum attribute cases
• Apply formatting to tablegen (.td) files

[jfurtek]

[mlir] Add ODS support for enum attributes with grouped bit cases

This diff modifies the tablegen specification and code generation for
BitEnumAttr attributes in MLIR Operation Definition Specification (ODS) files.
Specifically:

• there is a new tablegen class for "none" values (i.e. no bits set)
• single-bit enum cases are specified via bit index (i.e. [0, 31]) instead of the resulting enum integer value
• there is a new tablegen class to represent a "grouped" bitwise OR of other enum values

This diff is intended as an initial step towards improving "fastmath"
optimization support in MLIR, to allow more precise control of whether certain
floating point optimizations are applied in MLIR passes. "Fast" math options
for floating point MLIR operations would (following subsequent RFC and
discussion) be specified by using the improved enum bit support in this diff.
For example, a "fast" enum value would act as an alias for a group of other
cases (e.g. finite-math-only, no-signed-zeros, etc.), in a way that is similar
to support in C/C++ compilers (clang, gcc).

[jfurtek]

mlir/docs/OpDefinitions.md

1405–1406

I removed the "empty string" case, and added an assert.

1407–1411

The previous implementation assumed mutually exclusive bit enum cases, of a single bit, so a simple bitwise and operation was adequate. In addition, the stringify() function cleared the bits, one by one, while building the list of cases.

The new implementation allows "groups" of bits, which effectively alias one or more of the individual bit cases. For this reason:

• an enum case (with it associated string representation) can require more than one bit,
• the stringify() function can't clear bits while building the string list, and
• the check for each enum case needs to determine whether all bits in the case are present - not just "any" of the bits.

mlir/include/mlir/IR/OpBase.td

1344–1348

My mistake - I thought that clang-format was taking care of this automatically.

It turns out that neither git clang-format nor clang-format-diff.py have .td files in the default list for file extensions. On top of that, clang-format support for line ranges seems to be broken for tablegen files (but not C/C++ files), so manually providing the .td file extension to git clang-format actually applies formatting changes to other lines (that I didn't change in this revision). (For example, there are other lines on OpBase.td that are more than 80 lines.) I've manually made the changes that clang-format made in the updated revision, but only to the lines that I changed. I will see if I can file a bug report for clang-format in the mean time. Let me know if there is different formatting I should apply here.

mlir/tools/mlir-tblgen/EnumsGen.cpp

197

The BitEnumAttrBase (tablegen) class has a predicate that checks the bitwise or of the attribute value against the cases, and this ends up being used by the implementation of the classof() function for the attribute. I can verify that an assertion does occur when initialized with an invalid bit - when assertions are enabled. I'm not sure if this is something that should have a test - in my (brief) experiments, initializing a 32-bit integer attribute with larger (e.g. 64-bit) integer - which seem like might be considered a similar scenario - doesn't trigger an assert.

[rriddle]

Did a scan over and looks like a general improvement over the current state (which should really get its own cleanup anyways).

mlir/include/mlir/IR/OpBase.td

1331–1332

Kind of nit, but you don't really need the braces,

1344–1348

There is no good formatting tool for Tablegen that I know of (which is why a lot of it can be weirdly formatted differently). I always just eyeball format it and try to stay consistent (keyword being "try").

1345

Not sure if we have an established style for .td asserts, but those in C++ generally start with lower case.

mlir/tools/mlir-tblgen/EnumsGen.cpp

209–210

Can you spell out what auto is here?

[jfurtek]

[mlir] Add ODS support for enum attributes with grouped bit cases

This diff modifies the tablegen specification and code generation for
BitEnumAttr attributes in MLIR Operation Definition Specification (ODS) files.
Specifically:

• there is a new tablegen class for "none" values (i.e. no bits set)
• single-bit enum cases are specified via bit index (i.e. [0, 31]) instead of the resulting enum integer value
• there is a new tablegen class to represent a "grouped" bitwise OR of other enum values

This diff is intended as an initial step towards improving "fastmath"
optimization support in MLIR, to allow more precise control of whether certain
floating point optimizations are applied in MLIR passes. "Fast" math options
for floating point MLIR operations would (following subsequent RFC and
discussion) be specified by using the improved enum bit support in this diff.
For example, a "fast" enum value would act as an alias for a group of other
cases (e.g. finite-math-only, no-signed-zeros, etc.), in a way that is similar
to support in C/C++ compilers (clang, gcc).

Revisions based on feedback:

• removed support for invalid bits in stringify, replacing with assert
• fixed formatting of tablegen changes
• changed name of BitEnumAttrCase base class
• fix typo in documentation for OpDefinitions.md
• fix assert string to be lower case
• remove 'auto', explicitly declaring enumerant value type for clarity
• remove unnecessary tablegen braces

[jfurtek]

Could someone with commit privileges land these changes (when convenient)? Thanks!

attribution: [e-mail]

[Mogball]

I gotchu. Sorry it took so long -- it slipped from my mind over the weekend.

mlir/docs/OpDefinitions.md

 } // namespace Inner
 } // namespace Outer
 ```
 
 Similarly for the following `BitEnumAttr` definition:
 
 ```tablegen
 def None: BitEnumAttrCaseNone<"None">;
 def Bit1: BitEnumAttrCaseBit<"Bit0", 0>;
 def Bit1: BitEnumAttrCaseBit<"Bit1", 1>;
 def Bit2: BitEnumAttrCaseBit<"Bit2", 2>;
 def Bit3: BitEnumAttrCaseBit<"Bit3", 3>;
 
 def MyBitEnum: BitEnumAttr<"MyBitEnum", "An example bit enum",
                            [None, Bit0, Bit1, Bit2, Bit3]>;
 ```
 
static bool isEqual(const ::MyBitEnum &lhs, const ::MyBitEnum &rhs) {
     return lhs == rhs;
   }
 };
 ```
 
 ```c++
 std::string stringifyMyBitEnum(MyBitEnum symbol) {
   auto val = static_cast<uint32_t>(symbol);
+  assert(15u == (15u | val) && "invalid bits set in bit enum");
   // Special case for all bits unset.
   if (val == 0) return "None";
-  // Return an empty string if any invalid bits are present
-  if (15 != (15 | val)) return "";
   llvm::SmallVector<llvm::StringRef, 2> strs;
   if (1u == (1u & val)) { strs.push_back("Bit0"); }
   if (2u == (2u & val)) { strs.push_back("Bit1"); }
   if (4u == (4u & val)) { strs.push_back("Bit2"); }
   if (8u == (8u & val)) { strs.push_back("Bit3"); }
   
   return llvm::join(strs, "|");
 }
 
 llvm::Optional<MyBitEnum> symbolizeMyBitEnum(llvm::StringRef str) {
   // Special case for all bits unset.
   if (str == "None") return MyBitEnum::None;
 
   llvm::SmallVector<llvm::StringRef, 2> symbols;

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVBase.td

 // A predicate that checks whether `$_self` is a known enum case for the
 // enum class with `name`.
 class SPV_IsKnownEnumCaseFor<string name> :
     CPred<"::mlir::spirv::symbolize" # name # "("
           "$_self.cast<IntegerAttr>().getValue().getZExtValue()).hasValue()">;
 
 // Wrapper over base BitEnumAttr to set common fields.
 class SPV_BitEnumAttr<string name, string description,
-                      list<BitEnumAttrCaseBase> cases> :
+                      list<BitEnumAttrCase> cases> :
     BitEnumAttr<name, description, cases> {
   let predicate = And<[
     I32Attr.predicate,
     SPV_IsKnownEnumCaseFor<name>,
   ]>;
   let cppNamespace = "::mlir::spirv";
 }
 

mlir/include/mlir/IR/OpBase.td

 // representation is the same as the C++ symbol name.
 class I32EnumAttrCase<string sym, int val, string str = sym>
     : IntEnumAttrCaseBase<I32, sym, str, val>;
 class I64EnumAttrCase<string sym, int val, string str = sym>
     : IntEnumAttrCaseBase<I64, sym, str, val>;
 
 // A bit enum case stored with 32-bit IntegerAttr. `val` here is *not* the
 // ordinal number of the bit that is set. It is the 32-bit integer with only
 // one bit set.
-class BitEnumAttrCaseBase<string sym, int val, string str = sym> :
-    EnumAttrCaseInfo<sym, val, str>,
-    SignlessIntegerAttrBase<I32, "case " # str> {
+class BitEnumAttrCase<string sym, int val, string str = sym>
+    : EnumAttrCaseInfo<sym, val, str>,
+      SignlessIntegerAttrBase<I32, "case " #str> {
 }
 
 // The special bit enum case for no bits set (i.e. value = 0).
-class BitEnumAttrCaseNone<string sym, string str = sym> :
-    BitEnumAttrCaseBase<sym, 0, str> {
+class BitEnumAttrCaseNone<string sym, string str = sym>
+    : BitEnumAttrCase<sym, 0, str> {
 }
 
 // The bit enum case for a single bit, specified by the bit position.
 // The pos argument refers to the index of the bit, and is currently
 // limited to be in the range [0, 31].
-class BitEnumAttrCaseBit<string sym, int pos, string str = sym> :
-    BitEnumAttrCaseBase<sym, !shl(1, pos), str> {
-    assert !and(!ge(pos, 0), !le(pos, 31)), "Bit position must be between 0 and 31"; 
+class BitEnumAttrCaseBit<string sym, int pos, string str = sym>
+    : BitEnumAttrCase<sym, !shl(1, pos), str> {
+  assert !and(!ge(pos, 0), !le(pos, 31)),
+      "Bit position must be between 0 and 31";
 }
 
 // A bit enum case for a group/list of previously declared single bits,
 // providing a convenient alias for that group.
-class BitEnumAttrCaseGroup<string sym, list<BitEnumAttrCaseBit> cases, string str = sym> :
-    BitEnumAttrCaseBase<sym, !foldl(0, cases, value, bitcase, !or(value, bitcase.value)), str> {
+class BitEnumAttrCaseGroup<string sym, list<BitEnumAttrCaseBit> cases,
+                           string str = sym>
+    : BitEnumAttrCase<
+          sym, !foldl(0, cases, value, bitcase, !or(value, bitcase.value)),
+          str> {
 }
 
 // Additional information for an enum attribute.
 class EnumAttrInfo<
     string name, list<EnumAttrCaseInfo> cases, Attr baseClass> :
       Attr<baseClass.predicate, baseClass.summary> {
   // The C++ enum class name
   string className = name;

 }
 
 // A bit enum stored with 32-bit IntegerAttr.
 //
 // Op attributes of this kind are stored as IntegerAttr. Extra verification will
 // be generated on the integer to make sure only allowed bits are set. Besides,
 // helper methods are generated to parse a string separated with a specified
 // delimiter to a symbol and vice versa.
-class BitEnumAttrBase<list<BitEnumAttrCaseBase> cases, string summary> :
+class BitEnumAttrBase<list<BitEnumAttrCase> cases, string summary> :
     SignlessIntegerAttrBase<I32, summary> {
   let predicate = And<[
     I32Attr.predicate,
     // Make sure we don't have unknown bit set.
     CPred<"!($_self.cast<::mlir::IntegerAttr>().getValue().getZExtValue() & (~("
           # !interleave(!foreach(case, cases, case.value # "u"), "|") #
           ")))">
   ]>;
 }
 
-class BitEnumAttr<string name, string summary, list<BitEnumAttrCaseBase> cases> :
+class BitEnumAttr<string name, string summary, list<BitEnumAttrCase> cases> :
     EnumAttrInfo<name, cases, BitEnumAttrBase<cases, summary>> {
   let underlyingType = "uint32_t";
 
   // Determine "valid" bits from enum cases for error checking
   int validBits = !foldl(0, cases, value, bitcase, !or(value, bitcase.value));
 
   // We need to return a string because we may concatenate symbols for multiple
   // bits together.

mlir/tools/mlir-tblgen/EnumsGen.cpp

static void emitSymToStrFnForBitEnum(const Record &enumDef, raw_ostream &os) {
   auto allBitsUnsetCase = getAllBitsUnsetCase(enumerants);
 
   os << formatv("{2} {1}({0} symbol) {{\n", enumName, symToStrFnName,
                 symToStrFnRetType);
 
   os << formatv("  auto val = static_cast<{0}>(symbol);\n",
                 enumAttr.getUnderlyingType());
   // If we have unknown bit set, return an empty string to signal errors.
   int64_t validBits = enumDef.getValueAsInt("validBits");
-  os << formatv("  if ({0}u != ({0}u | val)) return \"\";\n", validBits);
+  os << formatv("  assert({0}u == ({0}u | val) && \"invalid bits set in bit "
+                "enum\");\n",
+                validBits);
   if (allBitsUnsetCase) {
     os << "  // Special case for all bits unset.\n";
     os << formatv("  if (val == 0) return \"{0}\";\n\n",
                   allBitsUnsetCase->getSymbol());
   }
   os << "  ::llvm::SmallVector<::llvm::StringRef, 2> strs;\n";
   for (const auto &enumerant : enumerants) {
     // Skip the special enumerant for None.
     if (auto val = enumerant.getValue())
       os << formatv(
           "  if ({0}u == ({0}u & val)) {{ strs.push_back(\"{1}\"); }\n ", val,
           enumerant.getStr());
   }
   os << formatv("  return ::llvm::join(strs, \"{0}\");\n", separator);
 
   os << "}\n\n";
 }