Page MenuHomePhabricator

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

Authored by jfurtek on Jan 11 2022, 8:25 AM.

Details

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

Diff Detail

Event Timeline

jfurtek created this revision.Jan 11 2022, 8:25 AM
jfurtek requested review of this revision.Jan 11 2022, 8:25 AM

Nice

mlir/tools/mlir-tblgen/EnumsGen.cpp
197

Where else is this verified on the attribute?

rriddle added inline comments.Jan 13 2022, 10:28 AM
mlir/docs/OpDefinitions.md
1415–1416

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

mlir/include/mlir/IR/OpBase.td
1329–1331
1336–1337
1341–1345

Is this wrapped at 80?

Mogball added inline comments.Jan 13 2022, 3:10 PM
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 added inline comments.Jan 13 2022, 3:29 PM
mlir/docs/OpDefinitions.md
1354

nit: def Bit0

1417–1421

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

Same below.

jfurtek updated this revision to Diff 401021.Jan 18 2022, 3:46 PM

[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 updated this revision to Diff 401023.Jan 18 2022, 3:54 PM

[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 marked 7 inline comments as done.Jan 18 2022, 4:25 PM
jfurtek added inline comments.
mlir/docs/OpDefinitions.md
1415–1416

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

1417–1421

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
1341–1345

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 accepted this revision.Jan 19 2022, 3:54 PM

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
1328–1329

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

1341–1345

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

1342

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
208–209

Can you spell out what auto is here?

This revision is now accepted and ready to land.Jan 19 2022, 3:54 PM
jfurtek updated this revision to Diff 401771.Jan 20 2022, 1:41 PM
jfurtek marked 4 inline comments as done.

[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 marked 5 inline comments as done.Jan 20 2022, 1:43 PM

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

attribution: jfurtek@nvidia.com

This revision was automatically updated to reflect the committed changes.

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